-
Notifications
You must be signed in to change notification settings - Fork 404
Feat/support cosmos evm #1814
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
base: main
Are you sure you want to change the base?
Feat/support cosmos evm #1814
Conversation
- Implemented encoding for EthSecp256k1 public keys in amino format. - Added functions to encode and decode EthSecp256k1 signatures. - Updated pubkey type definitions to include EthSecp256k1. - Enhanced signing clients to handle EthSecp256k1 accounts. - Created DirectEthSecp256k1HdWallet for managing EthSecp256k1 keypairs. - Added tests for DirectEthSecp256k1HdWallet and DirectEthSecp256k1Wallet functionalities.
6e044f9
to
ce7e8b9
Compare
} | ||
const pubkey = encodePubkey(encodeSecp256k1Pubkey(accountFromSigner.pubkey)); | ||
let pubkey; | ||
if (accountFromSigner.algo == "eth_secp256k1" || accountFromSigner.algo == "ethsecp256k1" ) { |
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.
It would be good to add a comment here to explain why you need eth_secp256k1
and ethsecp256k1
.
…nd update pubkey encoding in clients
} | ||
|
||
export function isEthSecp256k1Pubkey(pubkey: Pubkey): pubkey is EthSecp256k1Pubkey { | ||
return (pubkey as EthSecp256k1Pubkey).type === "os/PubKeyEthSecp256k1"; |
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.
Is this a typo? os/PubKeyEthSecp256k1
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.
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.
Thanks.
What does "os/" mean in this context?
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.
Maybe a legacy from evmOS?
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.
Seems so. That is the earliest example of it I can see publicly within github.
packages/amino/src/signerutils.ts
Outdated
* @param account The account data from a signer | ||
* @returns The amino-encoded pubkey (EthSecp256k1Pubkey or Secp256k1Pubkey) | ||
*/ | ||
export function getAminoPubkey(account: AccountData): any { |
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.
There's no good reason for the return type here to be any
.
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.
I have updated the code. Please have a look.
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.
This function isn't updated:
cosmjs/packages/amino/src/pubkeys.ts
Lines 55 to 58 in 67234a8
export function isSinglePubkey(pubkey: Pubkey): pubkey is SinglePubkey { | |
const singPubkeyTypes: string[] = [pubkeyType.ed25519, pubkeyType.secp256k1, pubkeyType.sr25519]; | |
return singPubkeyTypes.includes(pubkey.type); | |
} |
…-usages Fix/add missing ethsecp256k1 pubkey usages
I have updated the code. Please have a look. |
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.
Cool, looks pretty nice. Glad to see that the quality of evm signing imrpoved a lot on the chain side of things which makes it smooth to integrate.
return ripemd160(sha256(pubkeyData)); | ||
} | ||
|
||
export function rawEthSecp256k1PubkeyToRawAddress(pubkeyData: Uint8Array): Uint8Array { |
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.
An "raw Eth Secp256k1 Pubkey" is uncompressed by convention. All Ethereum ecosystem uses uncompressed pubkeys. So either the implementation is wrong but maybe it is a question of findung a better name for the function
I am wrong I guess and compressed pubkeys are used consostently in Cosmos, also evm code. Any thoughts on that matter?
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.
I found the codes in cosmos/evmethsecp256k1.go
, the PubKey() method returns a 33-byte compressed pubkey.
But when it derives an address, the Address() method first decompresses the pubkey, then uses the uncompressed pubkey to derive an address.
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.
Thanks, that helps. Sounds like a reasonable approach for the Cosmos Stack
// Prefixes listed here: https://github.com/tendermint/tendermint/blob/d419fffe18531317c28c29a292ad7d253f6cafdf/docs/spec/blockchain/encoding.md#public-key-cryptography | ||
// Last bytes is varint-encoded length prefix | ||
const pubkeyAminoPrefixSecp256k1 = fromHex("eb5ae987" + "21" /* fixed length */); | ||
const pubkeyAminoPrefixEthSecp256k1 = fromHex("5D7423DF" + "21" /* fixed length */); |
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.
Oh sweet, we now got a new Amino prefix for that? Could you add a reference to any documentation or source code where this is coming from?
This was a major blocker in earlier attempts to add EVM support to CosmJS
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.
Prove pubkeyAminoPrefixEthSecp256k1
is correct
1. Reproduce the existing prefix to prove my method is correct
According to the Amino document, I compute the below result, which matches the existing pubkeyAminoPrefixSecp256k1
.
2. Use the same method to compute pubkeyAminoPrefixEthSecp256k1

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.
Wow this document you found there is amazing. I did not know that there was an algorithm for calculating those prefixes. Super nice, thanks!
} | ||
|
||
export function isEthSecp256k1Pubkey(pubkey: Pubkey): pubkey is EthSecp256k1Pubkey { | ||
return (pubkey as EthSecp256k1Pubkey).type === "os/PubKeyEthSecp256k1"; |
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.
Thanks.
What does "os/" mean in this context?
|
||
export interface EthSecp256k1Pubkey extends SinglePubkey { | ||
readonly type: "os/PubKeyEthSecp256k1"; | ||
readonly value: string; |
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.
Question: is this a compressed or uncompressed encoding of the pubkey? In earlier versions of Ethermint this was uncompressed (but barely documented). Now looking at the code above it seems to be compressed.
packages/amino/src/pubkeys.ts
Outdated
secp256k1: "tendermint/PubKeySecp256k1" as const, | ||
/** @see https://github.com/tendermint/tendermint/blob/v0.33.0/crypto/secp256k1/secp256k1.go#L23 */ | ||
secp256k1: "tendermint/PubKeySecp256k1" as const, | ||
/** @see https://github.com/cosmos/evm/blob/main/crypto/ethsecp256k1/ethsecp256k1.go#L36 */ |
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.
/** @see https://github.com/cosmos/evm/blob/main/crypto/ethsecp256k1/ethsecp256k1.go#L36 */ | |
/** @see https://github.com/cosmos/evm/blob/v1.0.0-rc2/crypto/ethsecp256k1/ethsecp256k1.go#L35-L36 */ |
data: toBase64(encryptedData), | ||
}; | ||
return JSON.stringify(out); | ||
} |
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.
Could we please remove all references to kdf/serialize/deserialize from the newly added wallet? I know it is there for consistency but we will remove that functionality sooninsh and don't want new users to start with it. See #1796
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.
I checked, and the serialize/deserialize functions are only referenced in test codes.
/** | ||
* A wallet that holds a single secp256k1 keypair. | ||
* | ||
* If you want to work with BIP39 mnemonics and multiple accounts, use DirectSecp256k1HdWallet. |
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.
* If you want to work with BIP39 mnemonics and multiple accounts, use DirectSecp256k1HdWallet. | |
* If you want to work with BIP39 mnemonics and multiple accounts, use DirectEthSecp256k1HdWallet. |
value: Uint8Array.from(CosmosCryptoSecp256k1Pubkey.encode(pubkeyProto).finish()), | ||
}); | ||
} else if (isEthSecp256k1Pubkey(pubkey)) { | ||
const pubkeyProto = CosmosCryptoSecp256k1Pubkey.fromPartial({ |
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.
const pubkeyProto = CosmosCryptoSecp256k1Pubkey.fromPartial({ | |
// Note: This code block is hacky because we should use the correct EVM proto type | |
// https://github.com/cosmos/evm/blob/v1.0.0-rc2/proto/cosmos/evm/crypto/v1/ethsecp256k1/keys.proto#L12-L17. | |
// However, we do not have that available here or in cosmjs-types yet and the classic secp256k1 pubkey has the same structure except for documentation and annotations: | |
// https://github.com/cosmos/cosmos-sdk/blob/v0.53.4/proto/cosmos/crypto/secp256k1/keys.proto#L14-L30 | |
// | |
// Actually this type is so simple we should just have an Anybuf for TS instead of code generation (https://github.com/webmaster128/anybuf) | |
const pubkeyProto = CosmosCryptoSecp256k1Pubkey.fromPartial({ |
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.
This just adds a comment, no code change
…-update-from-comments
…om-comments Fix/add missing codes and update from comments
} | ||
|
||
export function isEthSecp256k1Pubkey(pubkey: Pubkey): pubkey is EthSecp256k1Pubkey { | ||
return (pubkey as EthSecp256k1Pubkey).type === "os/PubKeyEthSecp256k1"; |
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.
You should just use pubkey.type
here, there's no reason to cast it before the check.
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.
Should we also remove type casting in this line? https://github.com/cosmos/cosmjs/blob/main/packages/amino/src/pubkeys.ts#L24
return (pubkey as Secp256k1Pubkey).type === "tendermint/PubKeySecp256k1";
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.
You're right! #1847
Add Ethereum secp256k1 address support
Summary
This PR adds support for Ethereum-style secp256k1 addresses in CosmJS, enabling compatibility with Ethereum-derived address formats in Cosmos chains.
Changes
rawEthSecp256k1PubkeyToRawAddress()
function to convert compressed Ethereum secp256k1 public keys to raw addressespubkeyToRawAddress()
to handleEthSecp256k1Pubkey
typekeccak256
,Secp256k1
) from@cosmjs/crypto
Technical Details
Breaking Changes
None - this is a purely additive change that maintains backward compatibility.