Add `frame_support::crypto::ecdsa::Public.to_eth_address()` (`k256`-based) and use it in pallets by agryaznov · Pull Request #11087 · paritytech/substrate

agryaznov · 2022-03-22T11:13:38Z

As a follow-up to #10883 and a pre-requisite to ink#1068, this PR:

adds pub fn to_eth_address(&self) -> Result<[u8; 20], ()> to ~~sp_core::ecdsa::Public~~frame_support::crypto::ecdsa
updates existing fn convert(a: beefy_primitives::crypto::AuthorityId) -> Vec<u8> in pallet-beefy-mmr to use it
adds seal_to_eth_address to pallet-contracts API which uses it

…it, contracts Ext interface

athei

First round. Didn't look at the contracts changes, yet.

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

…cts to use it

agryaznov · 2022-03-30T16:54:48Z

Had to put this new function into frame_support, to keep crypto calculations out of runtime (wasm performs weakly on those), while still avoiding creation of a new host function.

@shawntabrizi please advise if the place for it is chosen correctly.

athei

Please add some tests in the new ecdsa file.

athei

For the CI try to merge master and see if that helps.

bkchr

Didn't check the benchmarking code in detail.

Some general points, please never use unwrap in the runtime! Either expect with a proof or remove. In your case here you need to remove all of them.

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

bkchr · 2022-04-15T06:12:01Z

+ <[u8; 20]>::try_from(
+ sp_io::hashing::keccak_256(&uncompressed.as_bytes()[1..])[12..].as_ref(),
+ )
+ .expect("failed to take last 20 bytes of a keccak_256 hash")


Why do you still us an expect here? I already gave you a solution where you would not need this anymore.

your version didn't compile

Yeah it failed, but the solution wasn't that complicated. I pushed a commit to fix this.

…ased) and use it in pallets (paritytech#11087) * `ecdsa::Public::to_eth_address` + test, beefy-mmr `convert()` to use it, contracts Ext interface * `seal_ecdsa_to_eth_address` all but benchmark done * `seal_ecdsa_to_eth_address` + wasm test * `seal_ecdsa_to_eth_address` + benchmark * fixed dependencies * Apply suggestions from code review Co-authored-by: Alexander Theißen <alex.theissen@me.com> * fixes from review paritytech#1 * ecdsa::Public(*pk).to_eth_address() moved to frame_support and contracts to use it * beefy-mmr to use newly added frame_support function for convertion * a doc fix * import fix * benchmark fix-1 (still fails) * benchmark fixed * Apply suggestions from code review Co-authored-by: Alexander Theißen <alex.theissen@me.com> * fixes on Alex T feedback * to_eth_address() put into extension trait for sp-core::ecdsa::Public * Update frame/support/src/crypto/ecdsa.rs Co-authored-by: Alexander Theißen <alex.theissen@me.com> * Update frame/contracts/src/wasm/mod.rs Co-authored-by: Alexander Theißen <alex.theissen@me.com> * fixes on issues pointed out in review * benchmark errors fixed * fmt fix * EcdsaRecoverFailed err docs updated * Apply suggestions from code review Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * make applied suggestions compile * get rid of unwrap() in runtime * Remove expect Co-authored-by: Alexander Theißen <alex.theissen@me.com> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> Co-authored-by: Bastian Köcher <info@kchr.de>

…ased) and use it in pallets (paritytech#11087) * `ecdsa::Public::to_eth_address` + test, beefy-mmr `convert()` to use it, contracts Ext interface * `seal_ecdsa_to_eth_address` all but benchmark done * `seal_ecdsa_to_eth_address` + wasm test * `seal_ecdsa_to_eth_address` + benchmark * fixed dependencies * Apply suggestions from code review Co-authored-by: Alexander Theißen <alex.theissen@me.com> * fixes from review #1 * ecdsa::Public(*pk).to_eth_address() moved to frame_support and contracts to use it * beefy-mmr to use newly added frame_support function for convertion * a doc fix * import fix * benchmark fix-1 (still fails) * benchmark fixed * Apply suggestions from code review Co-authored-by: Alexander Theißen <alex.theissen@me.com> * fixes on Alex T feedback * to_eth_address() put into extension trait for sp-core::ecdsa::Public * Update frame/support/src/crypto/ecdsa.rs Co-authored-by: Alexander Theißen <alex.theissen@me.com> * Update frame/contracts/src/wasm/mod.rs Co-authored-by: Alexander Theißen <alex.theissen@me.com> * fixes on issues pointed out in review * benchmark errors fixed * fmt fix * EcdsaRecoverFailed err docs updated * Apply suggestions from code review Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * make applied suggestions compile * get rid of unwrap() in runtime * Remove expect Co-authored-by: Alexander Theißen <alex.theissen@me.com> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> Co-authored-by: Bastian Köcher <info@kchr.de>

agryaznov added 6 commits March 21, 2022 17:06

ecdsa::Public::to_eth_address + test, beefy-mmr convert() to use …
4c5f9fe
…it, contracts Ext interface

seal_ecdsa_to_eth_address all but benchmark done
414c40e

seal_ecdsa_to_eth_address + wasm test
a7453f4

seal_ecdsa_to_eth_address + benchmark
7e040fd

Merge branch 'master' into ag-seal-to-eth-addr
f843bc8

fixed dependencies
d3090c7

agryaznov requested review from acatangiu and athei as code owners March 22, 2022 11:13

athei reviewed Mar 22, 2022
View reviewed changes

Comment threadprimitives/core/src/lib.rs
Comment threadprimitives/core/Cargo.toml Outdated
Comment threadprimitives/core/src/ecdsa.rs Outdated
Comment threadframe/beefy-mmr/src/lib.rs
Comment threadframe/beefy-mmr/src/lib.rs

agryaznovand others added 6 commits March 22, 2022 19:15

Apply suggestions from code review
869cf84
Co-authored-by: Alexander Theißen <alex.theissen@me.com>

fixes from review#1
c4b6a29

Merge branch 'master' into ag-seal-to-eth-addr
3dbda9f

ecdsa::Public(*pk).to_eth_address() moved to frame_support and contra…
f5feb46
…cts to use it

beefy-mmr to use newly added frame_support function for convertion
b801972

a doc fix
be3e8a9

agryaznov changed the title ~~Add sp_core::ecdsa::Public.to_eth_address() (k256-based) and use it in pallets~~Add frame_support::crypto::ecdsa::Public.to_eth_address() (k256-based) and use it in palletsMar 30, 2022

import fix
4037170

agryaznov requested a review from shawntabrizi March 30, 2022 16:54

agryaznov requested a review from athei March 30, 2022 16:54

agryaznov added 3 commits March 30, 2022 20:23

benchmark fix-1 (still fails)
c8ff0ef

benchmark fixed
976ed7e

Merge branch 'master' into ag-seal-to-eth-addr
a26858b

athei reviewed Apr 1, 2022
View reviewed changes

bkchr reviewed Apr 4, 2022
View reviewed changes

Comment threadframe/support/src/crypto/ecdsa.rs Outdated

agryaznov added 5 commits April 11, 2022 21:03

fixes on issues pointed out in review
cdd6bdb

benchmark errors fixed
b51a7b0

Merge branch 'master' into ag-seal-to-eth-addr
fb1fcc4

fmt fix
41ba268

EcdsaRecoverFailed err docs updated
34779f8

agryaznov force-pushed the ag-seal-to-eth-addr branch from a70d756 to 34779f8Compare April 11, 2022 19:54

athei approved these changes Apr 12, 2022
View reviewed changes

Merge branch 'master' into ag-seal-to-eth-addr
10af685

github-actionsBot added the A7-needspolkadotpr label Apr 12, 2022

athei requested a review from bkchr April 13, 2022 14:37

bkchr approved these changes Apr 14, 2022
View reviewed changes

agryaznovand others added 4 commits April 14, 2022 19:59

Apply suggestions from code review
7c5fa91
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

make applied suggestions compile
ec3a2af

get rid of unwrap() in runtime
dffce61

Merge branch 'master' into ag-seal-to-eth-addr
55c2d06

agryaznov requested a review from athei April 14, 2022 21:36

bkchr suggested changes Apr 15, 2022
View reviewed changes

agryaznov requested a review from bkchr April 15, 2022 07:16

Remove expect
2997b7e

bkchr approved these changes Apr 16, 2022
View reviewed changes

agryaznov merged commit a5cd385 into masterApr 16, 2022

agryaznov deleted the ag-seal-to-eth-addr branch April 16, 2022 12:51

agryaznov mentioned this pull request Apr 24, 2022
Implement ecdsa_to_eth_address() and remove eth_compatibility crate use-ink/ink#1233
Merged

github-actionsBot mentioned this pull request May 31, 2022
Update substrate/polkadot/cumulus from v0.9.20 to v0.9.22 moonbeam-foundation/moonbeam#1557
Closed

ntn-x2 mentioned this pull request Jun 7, 2022
feat: support linking ethereum accounts KILTprotocol/kilt-node#355
Merged
6 tasks

github-actionsBot mentioned this pull request Jun 22, 2022
Update substrate/polkadot/cumulus from v0.9.20 to v0.9.23 moonbeam-foundation/moonbeam#1623
Closed

Dengjianping mentioned this pull request Jul 7, 2022
Bump deps to v0.9.22 Manta-Network/Manta#571
Merged
10 tasks

Uh oh!

Conversation

agryaznov commented Mar 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agryaznov commented Mar 30, 2022

Uh oh!

athei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

athei left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchrApr 15, 2022

Choose a reason for hiding this comment

Uh oh!

agryaznovApr 15, 2022

Choose a reason for hiding this comment

Uh oh!

bkchrApr 16, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

agryaznov commented Mar 22, 2022•
edited
Loading