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

added ethereum address20 support #2216

Merged
merged 9 commits into from
Nov 15, 2024
Merged

Conversation

aramikm
Copy link
Collaborator

@aramikm aramikm commented Nov 12, 2024

Goal

The goal of this PR is to allow sending transactions using ethereum signatures and ethereum addresses

Closes #2206

Changes

  • Replaced MultiSignature with UnifiedSignature in the runtime which supports ethereum signatures for ecdsa
  • Replaced AccountIdLookup with EthereumCompatibleAccountIdLookup in runtime which support 20 bytes accounts in MultiAddress
  • Showing runtime logs for start-frequency-instant mode (this would be helpful in debugging)

Warning

  • For e2e test we should use getUnifiedAddress(keyPair) function instead of keypair.address directly. Thisb would ensure we are using correct ethereum address for ethereum keys.

Checklist

  • Updated js/api-augment for Custom RPC APIs?
  • Unit Tests added?
  • e2e Tests added?
  • Spec version incremented?

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 57.77778% with 76 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
common/primitives/src/signatures.rs 57.77% 76 Missing ⚠️
Files with missing lines Coverage Δ
common/primitives/src/lib.rs 100.00% <ø> (ø)
common/primitives/src/signatures.rs 57.77% <57.77%> (ø)

@@ -103,6 +103,7 @@ allow = [
"OpenSSL",
"Unicode-DFS-2016",
"Zlib",
"Unicode-3.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wilwade This appears to be compatible with our license but would be good to double check.

@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Nov 12, 2024
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Nov 12, 2024
@aramikm aramikm requested a review from demisx as a code owner November 12, 2024 19:19
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Nov 12, 2024
@@ -0,0 +1,379 @@
#![cfg_attr(not(feature = "std"), no_std)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copied from sp_runtime and only modified the Ecdsa Path

Copy link
Collaborator

@JoeCap08055 JoeCap08055 left a comment

Choose a reason for hiding this comment

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

  • Reviewed code. Left a couple comments in the e2e tests & JS code, non-blocking, could be in another PR
  • Ran unit tests
  • Ran e2e tests locally

throw new Error(`ecdsa type is not supported!`);
}
return pair.address;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this might be required at quite a few places at some point/utility, should be documented?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I Agree With This Comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I already cleaned up and added more documentation on my other PR which focuses on e2e tests. Going to keep it as is so that it does not cause conflicts with my second PR

Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

Looks great... also checked out & ran e2e tests as well as reviewing code.

common/primitives/src/signatures.rs Show resolved Hide resolved
const passkeyPayload = await createPasskeyPayload(passKeyPrivateKey, passKeyPublicKey, passkeyCall, false);
const passkeyProxy = ExtrinsicHelper.executePassKeyProxy(fundedKeys, passkeyPayload);
assert.doesNotReject(passkeyProxy.fundAndSendUnsigned(fundingSource));
await assert.doesNotReject(passkeyProxy.fundAndSendUnsigned(fundingSource));
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 thanks

throw new Error(`ecdsa type is not supported!`);
}
return pair.address;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I Agree With This Comment

e2e/scaffolding/ethereum.ts Outdated Show resolved Hide resolved
e2e/scaffolding/ethereum.ts Outdated Show resolved Hide resolved
)
);
} catch (e) {
if ((e as any).name === 'RpcError') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎖️

Copy link
Collaborator

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

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

Great work 💯

@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels Nov 15, 2024
@aramikm aramikm merged commit deac3bf into main Nov 15, 2024
29 checks passed
@aramikm aramikm deleted the ethereum_style_account_support branch November 15, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metadata-changed Metadata has changed since the latest full release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants