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

feat(signer): ethereum implementation #1501

Merged
merged 19 commits into from
Apr 15, 2024
Merged

feat(signer): ethereum implementation #1501

merged 19 commits into from
Apr 15, 2024

Conversation

ryanleecode
Copy link
Contributor

@ryanleecode ryanleecode commented Mar 25, 2024

Description

Fixes: #1467

Adds an ethereum signer implementation to the signer crate.

Notes

  • The signer shares the ecdsa signing method since the only thing that is different is how the message is hashed
  • Instead of using for _ in 0..20 to add fuzzing to the tests, I'm using proptest

Testing

You can test this against moonbeam using chopsticks with the gist provided in the original issue:

https://gist.github.com/jsdw/13240f9341e433ea639b89d0d4235c8b

npx @acala-network/chopsticks@latest --config=chopsticks.yml

const ALITH: (&str, &str) = (
    "b6c8ddd1748f5c360e7efaa9a2f45a7872cc67a79599a889d07f40aa68b6de75",
    "adcaa4dabe8d19461eea3380dee40d4ca334f89a099b269c1ce66afb8de69461",
);
const BALTHASAR: (&str, &str) = (
    "f61ca7bfb5ee9e710cd259e7bcd0723fbb0dfb204442701c0ccb82e9c2f03a2b",
    "4d691af46d07a9b5d72e3bd89fc5f4c949708a249a0386bab32552789b8258fe",
);

chopsticks.yml

endpoint: wss://wss.api.moonbeam.network/
mock-signature-host: true
block: 4784047
db: ./db.sqlite

import-storage:
  System:
    Account:
      -
        -
          - "0x1ba2efe6ea21ee83e8925cd00fa6f562893919aa"
        - providers: 1
          data:
            free: "100000000000000000000000"
        -
          - "0x2ea13d9f33c8bf0c6dcdba10c739fe92857058af"
        - providers: 1
          data:
            free: "100000000000000000000000"
        -
          - "0xf24FF3a9CF04c71Dbc94D0b566f7A27B94566cac"
        - providers: 1
          data:
            free: "100000000000000000000000"
  TechCommitteeCollective:
    Members: ["0xf24FF3a9CF04c71Dbc94D0b566f7A27B94566cac"]
  CouncilCollective:
    Members: ["0xf24FF3a9CF04c71Dbc94D0b566f7A27B94566cac"]
  TreasuryCouncilCollective:
    Members: ["0xf24FF3a9CF04c71Dbc94D0b566f7A27B94566cac"]
  AuthorFilter:
    EligibleRatio: 100
    EligibleCount: 100

@ryanleecode ryanleecode force-pushed the feat/signer/eth branch 10 times, most recently from 74f6beb to b394b36 Compare March 26, 2024 03:27
@ryanleecode ryanleecode marked this pull request as ready for review March 26, 2024 03:42
@ryanleecode ryanleecode requested a review from a team as a code owner March 26, 2024 03:42
@@ -15,7 +15,7 @@ description = "Sign extrinsics to be submitted by Subxt"
keywords = ["parity", "subxt", "extrinsic", "signer"]

[features]
default = ["sr25519", "ecdsa", "subxt", "std", "native"]
default = ["sr25519", "ecdsa", "eth", "subxt", "std", "native"]
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether this should be a default feature because I reckon "eth accounts/signer" is rather niche use case and most users won't need it.

Doesn't matter that much it's easy to opt-out anyway, @jsdw thoughts?

Copy link
Collaborator

@jsdw jsdw Mar 27, 2024

Choose a reason for hiding this comment

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

The only reason I tend to default to having all feature flags is for nicer IDE/docs support (otherwise loads of stuff is greyed out). So this is consistent with what I did with the other signers, but I'd be very open to figuring out an approach which has IDEs/docs working nicely even without feature flags enabled here; I just haven't given it any thought :)

signer/src/eth.rs Outdated Show resolved Hide resolved
signer/src/eth.rs Outdated Show resolved Hide resolved
signer/src/eth.rs Outdated Show resolved Hide resolved
signer/src/eth.rs Outdated Show resolved Hide resolved
@ryanleecode ryanleecode requested a review from jsdw March 26, 2024 17:38
signer/src/eth.rs Outdated Show resolved Hide resolved
signer/src/eth.rs Outdated Show resolved Hide resolved
signer/src/eth.rs Outdated Show resolved Hide resolved
@ryanleecode
Copy link
Contributor Author

ryanleecode commented Mar 27, 2024

So there is a difference in the derive implementation as well. For ethereum the derivation path uses soft junctions and it looks like this m/44'/60'/0'/0/0

But the ecdsa implementation does not allow soft junctions.

https://github.com/paritytech/subxt/blob/master/signer/src/ecdsa.rs#L144

pub fn derive<Js: IntoIterator<Item = DeriveJunction>>(
    &self,
    junctions: Js,
) -> Result<Self, Error> {
    let mut acc = self.0.secret_key().clone().secret_bytes();
    for junction in junctions {
        match junction {
            DeriveJunction::Soft(_) => return Err(Error::SoftJunction),
            DeriveJunction::Hard(junction_bytes) => {
                acc = ("Secp256k1HDKD", acc, junction_bytes)
                    .using_encoded(sp_crypto_hashing::blake2_256)
            }
        }
    }
    Self::from_seed(acc)
}

@ryanleecode
Copy link
Contributor Author

ryanleecode commented Mar 28, 2024

I've removed from_uri and derive because they don't make sense in the eth context.

In Ethereum you have two contenders for derivation paths:

Soft derivation: m/44'/60'/0'/0/{index}
Hard derivation: m/44'/60'/{index}'/0/0

See: https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki

So the from_phrase function now has a third parameter index, which denotes the "account number" and also the derivation type: soft or hard.

Maybe we should also bring back the private key constructor since I imagine it would be handy.


I've also updated the dev account to match moonbeam's and confirmed they also work on another EVM compatible parachain.

Cargo.toml Outdated Show resolved Hide resolved
@jsdw jsdw mentioned this pull request Apr 12, 2024
* Tidy DerivationPath, make no-std compatible, misc bits

* fmt

* Improve comments and hide bip32 lib from pub interface

* remove unstable-eth from defaults again

* from_seed => from_secret_key, and check bip39 compliance eg no derivation path mnemonics vs seeds

* from_seed to from_Secret_key in sr25519 one; all of them are actually taking secret key bytes
@ryanleecode ryanleecode requested a review from a team as a code owner April 12, 2024 16:54
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

as it stands now I'm happy to merge this once CI is happy; Great job @ryanleecode!

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Nice once @ryanleecode ! 🚀

@ryanleecode ryanleecode merged commit b527c85 into master Apr 15, 2024
13 checks passed
@ryanleecode ryanleecode deleted the feat/signer/eth branch April 15, 2024 12:56
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/paritytech-update-for-april/7646/1

@jsdw jsdw mentioned this pull request May 16, 2024
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.

Add ethereum compatible signer to subxt_signer?
5 participants