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

EthSecp #1160

Closed
wants to merge 0 commits into from
Closed

EthSecp #1160

wants to merge 0 commits into from

Conversation

zakarialounes
Copy link
Contributor

Please also see: confio/cosmjs-types#30

@zakarialounes
Copy link
Contributor Author

cc: @fedekunze

@liangping
Copy link

Hope we can merge it ASAP.

@webmaster128
Copy link
Member

Is tendermint/PubKeyEthSecp256k1 a type that exists in Tendermint? At least it uses a Tendermint namespace. But I don't see it in the codebase. I would like to avoid adding code for forked Tendermint/Cosmos SDK tech.

@liangping
Copy link

liangping commented Jun 8, 2022

It's introduced By EVMOS, we need it if we want to integrate Evmos stuffs into Cosmjs.

For example:

export class EthereumLedgerSigner implements OfflineAminoSigner{
  app: Eth
  hdpath: string = "44'/60'/0'/0/0"

  public async getAccounts(): Promise<readonly AccountData[]> {

    return this.app.getAddress(this.hdpath).then(x => {
      const x1: AccountData = {
        pubkey: Secp256k1.compressPubkey(fromHex(x.publicKey)),
        address: x.address,
        algo: "secp256k1" // should set to 'ethsecp256k1'
      }
      const x2: AccountData = {
        pubkey: Secp256k1.compressPubkey(fromHex(x.publicKey)),
        address: ethToCosmos(x.address),
        algo: "secp256k1" // // should set to 'ethsecp256k1'
      }
      return [x1, x2]
    })
  };

There are many scenarios that we need to extend something like pubkey

@zakarialounes
Copy link
Contributor Author

Is tendermint/PubKeyEthSecp256k1 a type that exists in Tendermint? At least it uses a Tendermint namespace. But I don't see it in the codebase. I would like to avoid adding code for forked Tendermint/Cosmos SDK tech.

@webmaster128 So this PR will not be accepted?

@StregoFren
Copy link

I think the underlying problem is that comsjs currently can't be extended in Application Land to support this. In some way I think this is very special for the type of an Account. If it would be just messages, we could add them to the registry.

I think short term, it might make sense to support evmos accounts in cosmjs. If there are too many of those incidents, it might make more sense to allow users hooking into the account parsing.

@jolube
Copy link

jolube commented Jun 14, 2022

@StregoFren @webmaster128 In fact it is introduced by Ethermint: https://github.com/tharsis/ethermint

The open source library is used and maintained by the Evmos core team, in addition to being used in for a number of other chains; Cronos, Kava, GenesisL1, etc.

Supporting Ethermint types in cosmjs will ensure support for a growing number of chains in the Cosmos ecosystem that rely on Ethermint's EVM.

@@ -346,7 +351,11 @@ export class SigningStargateClient extends StargateClient {
if (!accountFromSigner) {
throw new Error("Failed to retrieve account from signer");
}
const pubkey = encodePubkey(encodeSecp256k1Pubkey(accountFromSigner.pubkey));
const pubkey = encodePubkey(
chainId.startsWith("evmos_9001-")

Choose a reason for hiding this comment

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

Is there a more general approach over the evmos chain id? Other chains may be using the EthSecp keys from an ethermint integration. It would be nice if they can also use the cosm library in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When this is the case then we can make the code more complex so that it is manageable. No need to make it more complex for the moment as long as it is not useful.

Choose a reason for hiding this comment

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

we already have other chains using the evm module from ethermint other than evmos. A few were mentioned here already (cronos, kava etc), and I am working on one myself. Given that I would love to see ethermint types supported in cosmjs in a way that these other chains could leverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I didn't know that. So I'm going to rework this so it's scalable and get back to you when it's right. Thanks for the heads up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be possible to get feedback from a maintainer, @webmaster128 per example? If I make the code compatible for all compatible chains, can the code be merged or do you refuse to have the type managed by cosmjs? Thanks in advance

@zakarialounes zakarialounes marked this pull request as draft June 17, 2022 14:30
@webmaster128
Copy link
Member

webmaster128 commented Jun 28, 2022

I have no issue adding support for evmos types where this is possible in a clean way. But looking at the diffs people send for evmos support, it feels like big time hackery in the deepest core of CosmJS. If evmos changes those things, it is no surprise Cosmos SDK client libraries don't work anymore.

We'll certainly not add chain ID checks to change internal logic. There should also be a different Amino prefix for the different pubkey type. AccountData does not support the ethermint pubkey type, but this could be changed.

Adding ethermint types in cosmjs-types seems useful. But not in src/cosmos/crypto/ethsecp256k1/keys.ts as there is a separate ethermint protobuf package already.

@dimiandre
Copy link
Member

So as dapp developers, how can we interact with evmos?

is there any hacky way to do that dAPP side instead of cosmjs side?

@zakarialounes
Copy link
Contributor Author

As soon as I have some time I will update the code to import the ethermint types directly into cosmjs (without having to go through the chain-id, but via the coinType). Before that I'll update the code with my last changes (chain-id updated) and I'll publish the package on npm so that it's possible to take advantage of it now (since there is absolutely no chance that's the Amino prefix will be changed imho). I let you know.

@@ -45,13 +61,15 @@ export function encodeEd25519Pubkey(pubkey: Uint8Array): Ed25519Pubkey {
// 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("eb5ae987" + "21" /* fixed length */);
Copy link
Member

Choose a reason for hiding this comment

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

Did you test this prefix? Is this the one used in address derivation?

This is wrong in two ways:

  1. eb5ae987 refers to tendermint/PubKeySecp256k1
  2. 0x21 means that data of length 33 bytes is following

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EthSecp256 use the same prefix and length (please see #1160 (comment)).
Works and is available for trial via https://ezstaking.tools/.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, good to know. Then this is broken in Ethermint. Possibly we could work around that by removing the reverse operation bytes -> Pubkey. Let's see ...

@webmaster128 webmaster128 mentioned this pull request Dec 13, 2022
9 tasks
@webmaster128
Copy link
Member

Thank you for this PR. It was a very helpful contribution to #1351. For now it is undecided if Ethermint is in scope of this project.

@vincentysc
Copy link

Hey~ Im interested in the ethermint support too. Im willing to contribute and push this forward. So the blockers is how to ideal with the same prefix of secp256k1 and ethsecp256k1. And the chain-id issue can be solved by using the coin_type=60. Those are the blockers. Am I right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants