QPQ-AG/gmbytecode
10
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases 9 Wiki Activity

[PT-167805291] Add opcode for ecrecover #181

Merged
zxq9 merged 6 commits from newby/ecrecover into master 2019-08-14 23:08:45 +09:00
Conversation 11 Commits 6 Files Changed 3 +15 -11
zxq9 commented 2019-08-09 23:33:01 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: johnsnewby

Ref PT-16780529

Merge order:

  1. this PR
  2. PR for aesophia: https://github.com/aeternity/aesophia/pull/122
  3. PR for aeternity: https://github.com/aeternity/aeternity/pull/2638
*Created by: johnsnewby* Ref [PT-16780529](https://www.pivotaltracker.com/story/show/167805291) Merge order: 1. this PR 2. PR for aesophia: https://github.com/aeternity/aesophia/pull/122 3. PR for aeternity: https://github.com/aeternity/aeternity/pull/2638
zxq9 commented 2019-08-12 17:21:32 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

-define(PRIM_CALL_CRYPTO_ECRECOVER_SECP256K1,          420).
*Created by: hanssv* ```suggestion:-0+0 -define(PRIM_CALL_CRYPTO_ECRECOVER_SECP256K1, 420). ```
zxq9 commented 2019-08-12 17:21:42 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

Would it not make sense to also add the FATE instruction in this PR?

*Created by: UlfNorell* Would it not make sense to also add the FATE instruction in this PR?
zxq9 commented 2019-08-12 17:54:13 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

Review: Dismissed

OK, but as Ulf mentioned - you should have an opcode for FATE as well?

*Created by: hanssv* **Review:** Dismissed OK, but as Ulf mentioned - you should have an opcode for FATE as well?
zxq9 commented 2019-08-12 20:43:56 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

Put it in op code order please (or the next person adding an instruction will choose the same op code).

Also shouldn't it only take a hash and a signature? So [a,a,a] and {bytes, bytes}.

*Created by: UlfNorell* Put it in op code order please (or the next person adding an instruction will choose the same op code). Also shouldn't it only take a hash and a signature? So `[a,a,a]` and `{bytes, bytes}`.
zxq9 commented 2019-08-12 20:50:49 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: johnsnewby

I copied the signature from solidity, ecrecover(msgHash, v, r, s) but it could be just hash and sig. I really have no strong feelings about it. The ethereum implementation takes one argument and splits the variables out from it:

		let hash = H256::from_slice(&input[0..32]);
		let v = H256::from_slice(&input[32..64]);
		let r = H256::from_slice(&input[64..96]);
		let s = H256::from_slice(&input[96..128]);

I am happy to change the function signature if it's desired.

*Created by: johnsnewby* I copied the signature from solidity, `ecrecover(msgHash, v, r, s)` but it could be just hash and sig. I really have no strong feelings about it. The ethereum implementation takes one argument and splits the variables out from it: ``` let hash = H256::from_slice(&input[0..32]); let v = H256::from_slice(&input[32..64]); let r = H256::from_slice(&input[64..96]); let s = H256::from_slice(&input[96..128]); ``` I am happy to change the function signature if it's desired.
zxq9 commented 2019-08-12 21:32:09 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

Given that ecverify takes a single 65 byte signature it would be very strange to have ecrecover take the parts separately.

*Created by: UlfNorell* Given that ecverify takes a single 65 byte signature it would be very strange to have ecrecover take the parts separately.
zxq9 commented 2019-08-13 17:24:31 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

    , { 'ECRECOVER_SECP256K1', 16#7d,  false,    true,   true,  1300,   [a,a,a], ecrecover_secp256k1,            {bytes, bytes},   bytes, "Arg0 := ecrecover_secp256k1(Hash, Signature)"}
*Created by: UlfNorell* ```suggestion:-0+0 , { 'ECRECOVER_SECP256K1', 16#7d, false, true, true, 1300, [a,a,a], ecrecover_secp256k1, {bytes, bytes}, bytes, "Arg0 := ecrecover_secp256k1(Hash, Signature)"} ```
zxq9 commented 2019-08-13 23:45:48 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: UlfNorell

Review: Approved

*Created by: UlfNorell* **Review:** Approved
zxq9 commented 2019-08-14 15:57:08 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: hanssv

Review: Approved

*Created by: hanssv* **Review:** Approved
zxq9 commented 2019-08-14 18:08:45 +09:00 (Migrated from gitlab.com)
Copy Link

Created by: radrow

Review: Approved

*Created by: radrow* **Review:** Approved
zxq9 commented 2019-08-14 23:08:45 +09:00 (Migrated from gitlab.com)
Copy Link

Merged by: tolbrino at 2019-08-14 14:08:45 UTC

*Merged by: tolbrino at 2019-08-14 14:08:45 UTC*
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
WIP
bug
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
bug
Something is not working
duplicate
This issue or pull request already exists
enhancement
New feature
help wanted
Need some help
invalid
Something is wrong
pig lipstick
Muggle-facing enhancements
question
More information is needed
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: QPQ-AG/gmbytecode#181
No description provided.
Delete Branch "newby/ecrecover"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?