Implemented ECDSA recover function. #914

xgreenx · 2021-09-10T12:20:25Z

Added support of seal_ecdsa_recover from the paritytech/substrate#9686.
This function allows to recover the ecdsa compressed public key from the signature and message hash. It will return ECDSAPublicKey which has two methods to_eth_address and to_account_id.

Added method `to_eth_address` and `to_account_id`. Added tests.

cmichi · 2021-09-10T14:07:57Z

@xgreenx Thanks for the PR! Could you add the two words ECDSA and Ethereum here? That's why the spellcheck CI stage fails.

The other stage seems to be a Rust nightly change to the way the compiler displays error messages again. We'll sort it out in another PR.

cmichi

Looks good overall, I've got some comments.

cmichi · 2021-09-14T06:28:18Z

crates/lang/src/traits.rs

+            .expect("Unable to parse the compressed ECDSA public key");
+        let uncompressed = pub_key.serialize();
+
+        // Hash the uncompressed public key without first byte by Keccak256 algorithm.


Could you add the reasoning why the first byte is skipped to the comment?

cmichi · 2021-09-14T06:29:22Z

crates/env/src/api.rs

@@ -604,3 +604,38 @@ where
        instance.hash_encoded::<H, T>(input, output)
    })
 }
+
+/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.


Suggested change

/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.

/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`,

/// and stores the result in `output`.

cmichi · 2021-09-14T06:29:57Z

crates/env/src/backend.rs

@@ -140,6 +140,14 @@ pub trait EnvBackend {
        H: CryptoHash,
        T: scale::Encode;

+    /// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.


Suggested change

/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.

/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`,

/// and stores the result in `output`.

cmichi · 2021-09-14T06:32:51Z

crates/lang/src/env_access.rs

@@ -1013,4 +1017,62 @@ where
        ink_env::hash_encoded::<H, V>(value, &mut output);
        output
    }
+
+    /// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.


Suggested change

/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.

/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`,

/// and stores the result in `output`.

cmichi · 2021-09-14T06:45:05Z

crates/engine/src/ext.rs

+            signature[64]
+        };
+        let message = Message::parse(message_hash);
+        let signature = Signature::parse_slice(&signature[0..64]).unwrap();


Could you replace the unwrap()'s in the PR with expect(…)? We try to avoid unwrap's when possible.

Even better is unwrap_or_else(|error| panic!("<error description>: {}", error)) which contains the root error message alongside a local error description provided by us.

crates/engine/src/ext.rs

cmichi · 2021-09-14T06:52:34Z

crates/engine/src/ext.rs

+            Signature,
+        };
+
+        let recovery_byte = if signature[64] > 27 {


Something that I'm missing is an explanation of this magic number. How about creating a const with a mnemonic name and adding a comment to that const?

Please turn the 27 into a const with proper documentation of where it comes. Ideally a link.

I added the comment before the if section. It is more clear than a comment near a constant. This value is used in several crates, so I can't create one global constant. Define it in every crate seems overkill and better to leave the comment because 27 is used in one place.

cmichi · 2021-09-14T06:53:24Z

crates/lang/tests/traits.rs

+    ];
+
+    assert_eq!(pub_key.to_account_id(), EXPECTED_ACCOUNT_ID.into());
+}


Could you add a test for the failure case, asserting EcdsaRecoverFailed?

cmichi · 2021-09-14T06:56:56Z

crates/lang/src/traits.rs

+
+/// The ECDSA compressed public key.
+#[derive(Debug, Copy, Clone)]
+pub struct ECDSAPublicKey(pub [u8; 33]);


Is there a reason why you've put this into traits.rs? From what I see there is no trait logic and my intuition would have been to put it into env_access.rs.

crates/lang/src/traits.rs

cmichi · 2021-09-17T02:24:17Z

crates/eth_compatibility/src/lib.rs

+}
+
+/// Address of Ethereum account
+pub type EthereumAddress = [u8; 20];


Could you make it a pub struct EthereumAddress([u8; 20]); with Deref and DerefMut implemented for it? So basically analog to what you did for ECDSAPublicKey.

crates/eth_compatibility/src/lib.rs

crates/eth_compatibility/tests/conversion.rs

cmichi

Thank you!

cmichi · 2021-09-17T08:34:03Z

@xgreenx Could you run cargo fmt --all? That's why the CI currently fails.

xgreenx · 2021-09-17T08:39:37Z

Now gitlab fmt fails=)

crates/eth_compatibility/LICENSE

Robbepop · 2021-09-17T08:44:53Z

crates/eth_compatibility/src/lib.rs

+        result
+    }
+
+    pub fn to_account_id(&self) -> <DefaultEnvironment as Environment>::AccountId {


It would be better if we did not only support the DefaultEnvironment if that's possible.

It can be only DefaultEnvironment because it hashes the ECDSA public key and the output is 32 bytes. I copied this logic from the substrate(it is how substrate generates AccountId from the ECDSA).

So the user can't use a custom type of AccountId here.

If that's the case I'd rather remove to_account_id and instead provide a way to extract the [u8; 32] so that users can themselves create an AccountId from them if necessary.

The user already has the ability to get an inner array(it is public).
The idea of this method is to simplify work with default AccountId. Because without this implementation the user must check the source code of substrate and find the way how to convert ECDSA public key into AccountId.

First of all this API is missing proper documentation.
Second I find it very confusing for users that this only works for the default environment.
It would be an improvement if it worked for any environment that uses the default environment account ID instead but that would still lead to confusion.
I'd personally prefer an API that returns something that can easily turned into the default account ID instead, e.g. [u8; 32] byte array.

crates/eth_compatibility/src/lib.rs

Robbepop · 2021-09-17T08:47:51Z

crates/lang/src/env_access.rs

+        let result = ink_env::ecdsa_recover(signature, message_hash, &mut output);
+
+        match result.is_ok() {
+            true => Ok(ECDSAPublicKey { 0: output }),
+            false => Err(Error::EcdsaRecoverFailed),
+        }


This could be rewritten using Result::map and Result::map_err.

Suggested change

let result = ink_env::ecdsa_recover(signature, message_hash, &mut output);

match result.is_ok() {

true => Ok(ECDSAPublicKey { 0: output }),

false => Err(Error::EcdsaRecoverFailed),

}

ink_env::ecdsa_recover(signature, message_hash, &mut output)

.map(|_| ECDSAPublicKey { 0: output })

.map_err(|_| Error::EcdsaRecoverFailed)

Maybe borrow checker makes problems due to &mut output. Not checked, yet.

USed symbolic links instead files.

codecov-commenter · 2021-09-17T09:20:45Z

Codecov Report

Merging #914 (35b5fcc) into master (6e7941a) will decrease coverage by 0.44%.
The diff coverage is 59.21%.

@@            Coverage Diff             @@
##           master     #914      +/-   ##
==========================================
- Coverage   83.00%   82.56%   -0.45%     
==========================================
  Files         190      191       +1     
  Lines        8160     8234      +74     
==========================================
+ Hits         6773     6798      +25     
- Misses       1387     1436      +49

Impacted Files	Coverage Δ
crates/engine/src/ext.rs	`61.18% <0.00%> (-6.70%)`	⬇️
crates/env/src/backend.rs	`0.00% <ø> (ø)`
crates/lang/src/env_access.rs	`27.27% <ø> (ø)`
...tes/env/src/engine/experimental_off_chain/impls.rs	`66.29% <68.75%> (+0.53%)`	⬆️
crates/env/src/engine/off_chain/impls.rs	`54.83% <73.33%> (+3.55%)`	⬆️
crates/eth_compatibility/src/lib.rs	`74.07% <74.07%> (ø)`
crates/env/src/api.rs	`47.05% <100.00%> (+3.30%)`	⬆️
crates/primitives/src/key_ptr.rs	`75.00% <0.00%> (-25.00%)`	⬇️
crates/primitives/src/key.rs	`65.00% <0.00%> (-17.50%)`	⬇️
...ates/storage/src/collections/hashmap/fuzz_tests.rs	`95.74% <0.00%> (-4.26%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e7941a...35b5fcc. Read the comment docs.

Robbepop · 2021-09-21T06:33:11Z

What is missing from this PR still to get a final review and merge it?

xgreenx · 2021-09-21T07:05:32Z

If you agree with this, then the change is ready=)

Robbepop · 2021-09-23T09:45:42Z

crates/env/src/engine/off_chain/impls.rs

@@ -195,6 +195,43 @@ impl EnvBackend for EnvInstance {
        self.hash_bytes::<H>(&encoded[..], output)
    }

+    fn ecdsa_recover(


I hope we can get rid of the huge code dupe once we eliminate the old off-chain engine.

crates/eth_compatibility/src/lib.rs

Robbepop

sorry for be belayed review. found some things. rest looks good. if fixed we can merge.

Robbepop · 2021-09-28T11:57:09Z

crates/eth_compatibility/src/lib.rs

+
+/// The ECDSA compressed public key.
+#[derive(Debug, Copy, Clone)]
+pub struct ECDSAPublicKey(pub [u8; 33]);


Suggested change

pub struct ECDSAPublicKey(pub [u8; 33]);

pub struct ECDSAPublicKey([u8; 33]);

is there a good reason why this should be pub?

The substrate had something like this structure and the field was public. I did the same=)
I've reworked it to be private and implemented the From<[u8; 33]> trait.

crates/eth_compatibility/src/lib.rs

Robbepop · 2021-09-28T11:59:47Z

crates/eth_compatibility/src/lib.rs

+impl core::ops::Deref for ECDSAPublicKey {
+    type Target = [u8; 33];
+
+    #[inline]
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl core::ops::DerefMut for ECDSAPublicKey {
+    #[inline]
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}


Deref and DerefMut should be used with great care and usually should only be applied on reference-like or pointer-like types.
Instead I'd propose to implement AsRef<[u8; 33]> and AsMut<[u8; 33]>.
This leads the same purpose but is way less intrusive.

Robbepop · 2021-09-28T12:00:10Z

crates/eth_compatibility/src/lib.rs

+impl core::ops::Deref for EthereumAddress {
+    type Target = [u8; 20];
+
+    #[inline]
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl core::ops::DerefMut for EthereumAddress {
+    #[inline]
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}


Deref and DerefMut should be used with great care and usually should only be applied on reference-like or pointer-like types.
Instead I'd propose to implement AsRef<[u8; 33]> and AsMut<[u8; 33]>.
This leads the same purpose but is way less intrusive.

crates/eth_compatibility/src/lib.rs

…ey` is private.

Robbepop

LGTM

Thanks a lot for the PR and sorry for taking so long.

xgreenx added 2 commits September 10, 2021 15:17

Implemented ecdsa recovery function.

95c94ed

Added method `to_eth_address` and `to_account_id`. Added tests.

Cargo fmt

3b1cf2a

xgreenx added 2 commits September 10, 2021 20:03

Added ECDSA and Ethereum to dictionary

c652162

Fixed comments according a new spellcheck

460441e

cmichi requested changes Sep 14, 2021

View reviewed changes

cmichi reviewed Sep 14, 2021

View reviewed changes

crates/lang/src/traits.rs Outdated Show resolved Hide resolved

xgreenx and others added 3 commits September 15, 2021 00:42

Fixes according comments in review.

e6637f6

Fixed build issue for wasm

57e3446

Merge branch 'paritytech:master' into feature/ink-ecdsa-recovery

a4a9491

cmichi reviewed Sep 17, 2021

View reviewed changes

crates/eth_compatibility/src/lib.rs Outdated Show resolved Hide resolved

cmichi reviewed Sep 17, 2021

View reviewed changes

crates/eth_compatibility/tests/conversion.rs Outdated Show resolved Hide resolved

Use struct instead of alias for EthereumAddress.

b0ba645

cmichi approved these changes Sep 17, 2021

View reviewed changes

cargo fmt --all

1758ef7

Robbepop reviewed Sep 17, 2021

View reviewed changes

crates/eth_compatibility/LICENSE Outdated Show resolved Hide resolved

Robbepop reviewed Sep 17, 2021

View reviewed changes

Simplified ecdsa_recover.

043a78d

USed symbolic links instead files.

Merge branch 'paritytech:master' into feature/ink-ecdsa-recovery

77baada

Added documentation for to_eth_address and to_account_id methods.

cde2bda

Robbepop reviewed Sep 23, 2021

View reviewed changes

xgreenx added 2 commits September 23, 2021 15:14

Renamed to_account_id into to_default_account_id

f5833c7

Cargo fmt

226cc55

Robbepop requested changes Sep 28, 2021

View reviewed changes

xgreenx added 2 commits September 28, 2021 19:42

Removed DeRef trait. Now field of EthereumAddress and `ECDSAPublicK…

43855b2

…ey` is private.

Fixed doc test for ecdsa_recover in EnvAccess

a1e6fcb

Robbepop approved these changes Oct 6, 2021

View reviewed changes

Merge branch 'paritytech:master' into feature/ink-ecdsa-recovery

35b5fcc

cmichi merged commit 284e4e5 into use-ink:master Oct 14, 2021

	/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`, and stores the result in `output`.
	/// Recovers the compressed ECDSA public key for given `signature` and `message_hash`,
	/// and stores the result in `output`.

	pub struct ECDSAPublicKey(pub [u8; 33]);
	pub struct ECDSAPublicKey([u8; 33]);

Implemented ECDSA recover function. #914

Implemented ECDSA recover function. #914

Conversation

xgreenx commented Sep 10, 2021

cmichi commented Sep 10, 2021

cmichi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmichi left a comment

Choose a reason for hiding this comment

cmichi commented Sep 17, 2021

xgreenx commented Sep 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 17, 2021 • edited Loading

Codecov Report

Robbepop commented Sep 21, 2021

xgreenx commented Sep 21, 2021

Choose a reason for hiding this comment

Robbepop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbepop left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Sep 17, 2021 •

edited

Loading

Robbepop left a comment •

edited

Loading