-
Notifications
You must be signed in to change notification settings - Fork 146
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
Jlm update for ethereum pair addition #969
Conversation
packages/keyring/src/pair/index.ts
Outdated
if (p.length === 20) { | ||
return (p); | ||
} else { | ||
return hexToU8a(ethereumEncode(keccakAsU8a(secp256k1Expand(p)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Ethereum addresses are with the checksum encoding, I would not change this. It adds value in the ecosystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you refering to the use of ethereumEncode? Or the conversion? I'm not sure what you mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The encoding. So we updated all address outputs here to be non-checksummed. In the Ethereum ecosystem the checksums do add value. If we want non checksum addresses (which we certainly don't), the correct place would be to encode it without. In this case however, we don't want to do this.
- the address output should output the U8a - only a simple
p.length === 20 ? p : keccakAsU8a(secp256k1Expand(p))
- the encodeaddress should use ethereumEncode
So effectively we will probably only have the line change in 1 as code-changes (for length 20, return as-is, else keep the status quo) and then your new tests to ensure it is working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I pushed a commit in that sens
Co-authored-by: Jaco <jacogr@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just 2 small comments around reverting to the previous versions as indicated. Happy to merge after that, thank you.
Co-authored-by: Jaco <jacogr@gmail.com>
Co-authored-by: Jaco <jacogr@gmail.com>
Sounds good, updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
When injecting accounts, we only have the address and not the publicKey.
Therefore, we need to support adding a key from address for ethereum addresses