chainapsis / keplr-wallet

The most powerful wallet for the Cosmos ecosystem and the Interchain
https://www.keplr.app
Other
763 stars 455 forks source link

Keplr 2.0 signEthereum always errors #736

Closed garrettparris closed 1 year ago

garrettparris commented 1 year ago

Describe the bug I am using an upgraded version of keplr, keplr 2.0. I am trying to sign an eip1559 using signEthereum with the EthSignType 'transaction'. On a browser with the previous version of keplr, this method works great and I can sign and submit to a JSON RPC. In keplr 2.0 I just get an error saying TODO.

z497688734 commented 1 year ago

me too

Thunnini commented 1 year ago

TBH, I thought there was no place using this feature. So, the feature was omitted from the first version of 2.0 because it was pushed out of priority. I'll add it by early next week.

Thunnini commented 1 year ago

signEthereum API is restored in version v0.12.7. However, this version is still in pending review on chrome webstore. Probably, it will be published tomorrow.

Thunnini commented 1 year ago

It's published now. Remember that EthSignType.TRANSACTION returns not signed tx but signature from now. https://github.com/chainapsis/keplr-wallet/pull/747

mhagel commented 1 year ago

Thanks for the quick turnaround. For EthSignType.MESSAGE I am now seeing a mismatch between the signature address and the actual address.

This wasn't happening before 2.0. Is there a change in how the signature address gets generated?

Thunnini commented 1 year ago

@mhagel What error are you getting specifically? Can you share some code to reproduce this issue?

mhagel commented 1 year ago

Sure. There's no specific keplr error, just something we noticed when validating the signature.

We use ethereumjs-util to parse and validate the signature address returned from keplr.

In this case, signatureString is from Keplr. addressModel.address is the evmos... bech32 address of the signer.

    // import * as ethUtil from 'ethereumjs-util';

    const msgBuffer = Buffer.from(sortedStringify(canvasMessage));

    const msgHash = ethUtil.hashPersonalMessage(msgBuffer);
    const ethSignatureParams = ethUtil.fromRpcSig(signatureString.trim());
    const publicKey = ethUtil.ecrecover(
      msgHash,
      ethSignatureParams.v,
      ethSignatureParams.r,
      ethSignatureParams.s
    );

    const addressBuffer = ethUtil.publicToAddress(publicKey);
    const lowercaseAddress = ethUtil.bufferToHex(addressBuffer);
    try {
      const bech32AddrBuf = ethUtil.Address.fromString(
        lowercaseAddress.toString()
      ).toBuffer();
      const bech32Address = bech32.encode(
        chain.bech32_prefix,
        bech32.toWords(bech32AddrBuf)
      );
      if (addressModel.address === bech32Address) isValid = true;

Before Keplr 2.0, the signer address matched the address parsed from the public key derived from the signature string. Now, these don't match. This leads me to believe that the new keplr signEthereum implementation may function different from the previous one.

mhagel commented 1 year ago

Previous signEthereum implementation, for reference: https://github.com/chainapsis/keplr-wallet/blob/97e77b314c58e0c7379f48a880972dfbb481df26/packages/provider/src/core.ts#L223-L244

Thunnini commented 1 year ago

@mhagel Apologies my bad, https://github.com/chainapsis/keplr-wallet/commit/0e6b7169400f6c253d1ba69751a8b5b59cc97efc this commut solves the issue, this would be included in the next version

mhagel commented 1 year ago

Thank you @Thunnini, that fixed it!

Thunnini commented 1 year ago

We believe this issue has been resolved. If you have any additional issues, please create a new issue.