ethers-io / ext-signer-ledger

Ethers extension for LedgerSigner for Ledger Hardware Wallet support.
https://ethers.org
MIT License
7 stars 1 forks source link

hardware-wallets: "Error: transaction.chainId/signature.v mismatch" #6

Closed mudgen closed 1 year ago

mudgen commented 3 years ago

I am getting this error: Error: transaction.chainId/signature.v mismatch

From this code:


const { LedgerSigner } = require('@ethersproject/hardware-wallets')

async function main () {
  const signer = new LedgerSigner(ethers.provider)
  const usdcAddress = '0x2791Bca1f2de4661ED88A30C99A7a9449Aa84174'
  // account 3
  const account3 = 'myaddress'
  const erc20 = (await ethers.getContractAt('IERC20', usdcAddress)).connect(signer)
  const tx = await erc20.transfer(account3, ethers.utils.parseUnits('0.1', 6))
  const receipt = await tx.wait()
  if (receipt.status) {
    console.log('Successful')
  } else {
    console.log('Failed')
  }
}

The v value for the signature is 54.

I wonder if this error could be related to the chain id math change that was made in ledgerjs: https://github.com/LedgerHQ/ledgerjs/commit/a1f21d25f3c216d4b2ec475c0802c56223e9a9e0

This code is using Hardhat connected to Polygon, which uses chainId 137.

Please help me get this fixed. Thanks!

FrederikBolding commented 3 years ago

Yeah, the Ledger code was doing the math wrong for higher chain ids before. The Ledger library needs to be bumped for Ethers to fix this.

mudgen commented 3 years ago

Yeah, the Ledger code was doing the math wrong for higher chain ids before. The Ledger library needs to be bumped for Ethers to fix this.

I confirmed this is true. I cloned ethers.js and updated the ledger libraries and now it works.

ricmoo commented 3 years ago

Good to know. Were there any other changes needed other than changing the package name?

mudgen commented 3 years ago

No, only change was using the latest versions of the ledgerjs libraries. I made a pull request if that is helpful: https://github.com/ethers-io/ethers.js/pull/1370

ricmoo commented 3 years ago

Oh, I see. Pulling in a deprecated version, which I assume is one they maintain for backwards compatibility.

I can probable merge that change sooner then. I need to finish off Typed Transactions first, then I should be able to pull that in.

mudgen commented 3 years ago

Great.

mudgen commented 3 years ago

Can this be merged soon? I have someone else that needs this to work with Polygon. Thanks

mudgen commented 3 years ago

Is this coming soon?

ricmoo commented 3 years ago

I’ve been working on it straight for the last few days, the changes are not trivial to get things properly up to date. Please a bit more patience. :)

mudgen commented 3 years ago

Oh, that is really great, good job, and so much very appreciated! Sorry about being demanding.

I am curious, with this merge request it has been working for me. Of course I like and appreciate it being better and streamlined. I am curious, what you are improving?

ricmoo commented 3 years ago

The main thing is to pull the ancillary packages from the monorepo, especially things like the hardware wallets package, which have incredibly complex dependencies (it needs an OS with the dev libusb, and/or hid headers, and C extensions). They are causing havoc with the CI environments, and even testing locally is non-trivial.

I’m trying to upgrade the LedgerSigner to use modern methods of communicating from both node and the browser instead of the hacky u2f hijacking they used to do, which they have deprecated.

Right now I’m still fighting the dist file generation, as the Ledger libraries don’t seem to play nice with bundlers, but I think a module that can be dropped into a web browser will simplify the lives for many (including myself ;)).

mudgen commented 3 years ago

Yes, makes sense. I think it is really great you are doing this. I think LedgerSigner is critically important software so it thrills me to see you doing this.

A module that can be dropped into a web browser will simplify the lives for many (including myself ;)).

This sounds really great.

How would developers and users use this module in a web browser? What would be the user flow? Would it work like this:

  1. A user connects their Ledger to their computer.
  2. Then the user clicks on a button to tell the website to use their Ledger for transactions.
  3. Then the user clicks on a button to send a transaction to their Ledger.
  4. The users' Ledger signs the transaction and sends it to the website.
  5. Then what? How does the website send the transaction to the blockchain? Can MetaMask be used for that?

Right now people can use Ledger with MetaMask. But it isn't so great because there is a really annoying persistent security popup that comes up anytime someone tries to make a transaction with Ledger on Windows 10. Can the new Ledger support you are working on be a good alternative solution?

mudgen commented 3 years ago

How is the new Ledger support coming along?

mudgen commented 3 years ago

@ricmoo Is it possible to get this done/fixed soon?

mudgen commented 3 years ago

@ricmoo I am not use ledger in the browser. I just need ledger to work with ethersjs on nodejs. It does work but not with Polygon.

davidiola commented 3 years ago

ran into this same issue -- thx for providing a workaround @mudgen

tkporter commented 3 years ago

thanks for saving the day @mudgen! For future readers looking for the quickest way to get things workinng before https://github.com/ethers-io/ethers.js/pull/1370 is merged, my ugly approach was to just add

"resolutions": {
  "@ledgerhq/hw-app-eth": "5.47.1",
  "@ledgerhq/hw-transport": "5.46.0",
  "@ledgerhq/hw-transport-u2f": "5.36.0-deprecated"
}

to my package.json (which is essentially https://github.com/ethers-io/ethers.js/pull/1370)

anders-torbjornsen commented 2 years ago

I made a package for this @anders-t/ethers-ledger https://www.npmjs.com/package/@anders-t/ethers-ledger

ricmoo commented 2 years ago

Awesome! Thanks @anders-torbjornsen ; I'll refer people to this package until I get the v6 ancillary package working. :)

zemse commented 2 years ago

Argument of type 'JsonRpcProvider' is not assignable to parameter of type 'Provider' on 'LedgerSigner'(@ethersproject/hardware-wallets)

@noobshow that should work, can you share code snippet at the point where you get this error with the LedgerSigner?

zemse commented 2 years ago

JsonRpcProvider extends BaseProvider and BaseProvider extends ethers.providers.Provider. So a type that accepts ethers.providers.Provider should be happy with JsonRpcProvider.

It is possible that there is an ethers version mismatch (between ethers.providers.Provider coming from @ethersproject/hardware-wallets and ethers.providers.Provider coming from normal ethers package), can you once try removing package-lock.json or yarn.lock, removing node_modules, and reinstalling if this fixes the problem?

ItsShadowl commented 2 years ago

JsonRpcProvider extends BaseProvider and BaseProvider extends ethers.providers.Provider. So a type that accepts ethers.providers.Provider should be happy with JsonRpcProvider.

It is possible that there is an ethers version mismatch (between ethers.providers.Provider coming from @ethersproject/hardware-wallets and ethers.providers.Provider coming from normal ethers package), can you once try removing package-lock.json or yarn.lock, removing node_modules, and reinstalling if this fixes the problem?

Thanks, its truly a version mismatch

tynes commented 2 years ago

Is there a list of tasks that can be delegated to help out on this issue? This functionality is critical and I'm willing to help out on getting this over the line if possible :)

ItsShadowl commented 2 years ago

I can't even use it, it gives error after few reads of account info

spectre1989 commented 2 years ago

@tynes I just used this https://www.npmjs.com/package/@anders-t/ethers-ledger

LaGregance commented 2 years ago

thanks for saving the day @mudgen! For future readers looking for the quickest way to get things workinng before ethers-io/ethers.js#1370 is merged, my ugly approach was to just add

"resolutions": {
  "@ledgerhq/hw-app-eth": "5.47.1",
  "@ledgerhq/hw-transport": "5.46.0",
  "@ledgerhq/hw-transport-u2f": "5.36.0-deprecated"
}

to my package.json (which is essentially ethers-io/ethers.js#1370)

You save my day thanks. I hope this will be resolved soon in the ethers.js lib.

IanDorion commented 2 years ago

Is the signature valid if V is set to 27 ?

Sh11thead commented 1 year ago

still relevant. any better solutions?

spectre1989 commented 1 year ago

@Sh11thead I use frame

tonisives commented 1 year ago

thanks for saving the day @mudgen! For future readers looking for the quickest way to get things workinng before ethers-io/ethers.js#1370 is merged, my ugly approach was to just add

"resolutions": {
  "@ledgerhq/hw-app-eth": "5.47.1",
  "@ledgerhq/hw-transport": "5.46.0",
  "@ledgerhq/hw-transport-u2f": "5.36.0-deprecated"
}

to my package.json (which is essentially ethers-io/ethers.js#1370)

~This works. Just call yarn/npm install after adding it to package.json as well. Then yarn will resolve the newer ledger dependencies.~

Even if tx gets sent with this, the maxFeePerGas is always reset to 0: issue. Only working option for me was @anders-t/ethers-ledger v1.04

ricmoo commented 1 year ago

The new @ethers-ext/signer-ledger (this package) should now work. It uses the most recent stable Ledger libraries. :)

Let me know if you have any issues with it though.

Thanks! :)