Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: add Move contract and tests for ecrecover to address #4543

Merged
merged 1 commit into from
Sep 12, 2022
Merged

Conversation

joyqvq
Copy link
Contributor

@joyqvq joyqvq commented Sep 9, 2022

Addressed comments from #4461

Published contract on local testnet and call ecrecover_to_eth_address method:

        // Test case from https://web3js.readthedocs.io/en/v1.7.5/web3-eth-accounts.html#recover
        // signature: 0xb91467e570a6466aa9e9876cbcd013baba02900b8979d43fe208a4a4f339f5fd6007e74cd82e037b800186422fc2da167c747ef045e5d18a5f5d4300f8e1a0291c
        // hashed_msg: 0x1da44b586eb0729ff70a73c326926f6ed5a25f5b056e7f47fbc6e58d86871655
        // address: 0x2c7536e3605d9c16a7a3d7b1898e529396a65c23

Publish package:

~/mysten/sui/target/debug/sui client publish --gas-budget 10000

----- Certificate ----
Transaction Hash: cXJ+JOVeXY+Gbobb8stxdNPkCpUR69FW+yYNWX68Bwk=
Transaction Signature: AA==@kAI32JGsZgfjPLRWeoiCzVA4LmkelcNZYDMJFr0fsaxwEmyFNB6Et7ob0BvIslRS+rsDEjFB3/v+ypPQniNtDg==@vbWbKQmIavIjyZ4cUPRy+sXA3Swm9VMxpL3SLXN36lA=
Signed Authorities Bitmap: RoaringBitmap<[1, 2, 3]>
Transaction Kind : Publish
----- Transaction Effects ----
Status : Success
Created Objects:
  - ID: 0x9a072a9dc736ba6882df39360f808261a5f74df8 , Owner: Immutable
Mutated Objects:
  - ID: 0x0577a17d7aba159628083943717a5ace2775be43 , Owner: Account Address ( 0xb8a7851c2f68182143f13299cd10d86ac0f51cbe )
----- Publish Results ----
The newly published package object ID: 0x9a072a9dc736ba6882df39360f808261a5f74df8

Updated Gas : Coin { id: 0x0577a17d7aba159628083943717a5ace2775be43, value: 49287 }

Call method:

~/mysten/sui/target/debug/sui client call --package 0x9a072a9dc736ba6882df39360f808261a5f74df8 --module ecdsa --function ecrecover_to_eth_address --gas-budget 10000 --args 0xb91467e570a6466aa9e9876cbcd013baba02900b8979d43fe208a4a4f339f5fd6007e74cd82e037b800186422fc2da167c747ef045e5d18a5f5d4300f8e1a0291c 0x1da44b586eb0729ff70a73c326926f6ed5a25f5b056e7f47fbc6e58d86871655 0xb8a7851c2f68182143f13299cd10d86ac0f51cbe

----- Certificate ----
Transaction Hash: 55w4BW08P0cFn5Qr+AFYqPksiK6QSmgIJYXRnPXPrII=
Transaction Signature: AA==@FHEWC/C4PMiD4ZPMOMzezvA8si9j9tOp+Y8OVr6g7sBeZcdKL5LKKiyyCKPcB4ohV3JDdr5xTMrwqIQpL5T6Cg==@vbWbKQmIavIjyZ4cUPRy+sXA3Swm9VMxpL3SLXN36lA=
Signed Authorities Bitmap: RoaringBitmap<[1, 2, 3]>
Transaction Kind : Call
Package ID : 0x9a072a9dc736ba6882df39360f808261a5f74df8
Module : ecdsa
Function : ecrecover_to_eth_address
Arguments : [[65,185,20,103,229,112,166,70,106,169,233,135,108,188,208,19,186,186,2,144,11,137,121,212,63,226,8,164,164,243,57,245,253,96,7,231,76,216,46,3,123,128,1,134,66,47,194,218,22,124,116,126,240,69,229,209,138,95,93,67,0,248,225,160,41,28], [32,29,164,75,88,110,176,114,159,247,10,115,195,38,146,111,110,213,162,95,91,5,110,127,71,251,198,229,141,134,135,22,85], "0xb8a7851c2f68182143f13299cd10d86ac0f51cbe"]
Type Arguments : []
----- Transaction Effects ----
Status : Success
Created Objects:
  - ID: 0x3734d3adb658ee1f04e281f24769b302ef8d70fc , Owner: Account Address ( 0xb8a7851c2f68182143f13299cd10d86ac0f51cbe )
Mutated Objects:
  - ID: 0x0577a17d7aba159628083943717a5ace2775be43 , Owner: Account Address ( 0xb8a7851c2f68182143f13299cd10d86ac0f51cbe )

Get the output object with the value of address:

~/mysten/sui/target/debug/sui client object --id 0x3734d3adb658ee1f04e281f24769b302ef8d70fc

----- Move Object (0x3734d3adb658ee1f04e281f24769b302ef8d70fc[1]) -----
Owner: Account Address ( 0xb8a7851c2f68182143f13299cd10d86ac0f51cbe )
Version: 1
Storage Rebate: 14
Previous Transaction: 55w4BW08P0cFn5Qr+AFYqPksiK6QSmgIJYXRnPXPrII=
----- Data -----
type: 0x9a072a9dc736ba6882df39360f808261a5f74df8::ecdsa::Output
id: 0x3734d3adb658ee1f04e281f24769b302ef8d70fc
value: [44, 117, 54, 227, 96, 93, 156, 22, 167, 163, 215, 177, 137, 142, 82, 147, 150, 166, 92, 35]

Verify the address:

>>> x = bytearray([44, 117, 54, 227, 96, 93, 156, 22, 167, 163, 215, 177, 137, 142, 82, 147, 150, 166, 92, 35])
>>> x.hex()
'2c7536e3605d9c16a7a3d7b1898e529396a65c23'

@joyqvq joyqvq marked this pull request as ready for review September 9, 2022 17:10
@joyqvq joyqvq requested a review from oxade September 9, 2022 17:10
@@ -1765,19 +1765,21 @@ B1:
4: Gt
5: BrTrue(7)
B2:
6: Branch(16)
6: Branch(18)
Copy link
Contributor Author

@joyqvq joyqvq Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cheking @oxade is this calibration related snapshot change expected? i added a new native move function

@@ -18,6 +18,11 @@ module sui::crypto {
/// applied to Secp256k1 signatures.
public native fun ecrecover(signature: vector<u8>, hashed_msg: vector<u8>): vector<u8>;

/// @param pubkey: A 33-bytes compressed public key, a prefix either 0x02 or 0x03 and a 256-bit integer.
///
/// If the compressed public key is valid, return the 65-bytes uncompressed public key.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should say what will happen if it is invalid.

/// @param pubkey: A 33-bytes compressed public key, a prefix either 0x02 or 0x03 and a 256-bit integer.
///
/// If the compressed public key is valid, return the 65-bytes uncompressed public key.
public native fun decompress_pubkey(pubkey: vector<u8>): vector<u8>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not to be addressed in this PR, but some ideas for improving this library)

  • We could use type wrappers to represent compressed and decompressed public keys, signatures, etc. instead of always using vector<u8>. This should make it easier to avoid common mistakes/misuse
  • Instead of having one big crypto library, we could split it into a few smaller ones partitioned by (e.g.) signature scheme. No strong priors on how this should be done, but I think it will be easier to add new schemes in the future if we can do so by adding a fresh module instead of upgrading an existing on-chain module

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, will follow up a PR: #4567

};

let pubkey = crypto::ecrecover(signature, hashed_msg);
let uncompressed = crypto::decompress_pubkey(pubkey);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! and good observation that in Eth they hash the uncompressed pub key.

Copy link
Collaborator

@kchalkias kchalkias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this to unblock Eth bridging devs experimenting with Sui<>Eth. Agree with Sam re distinct PubKey types.

@joyqvq joyqvq merged commit 8092c73 into main Sep 12, 2022
@joyqvq joyqvq deleted the ethsig-2 branch September 12, 2022 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants