ethers.js icon indicating copy to clipboard operation
ethers.js copied to clipboard

Fix LedgerSigner for EIP1559

Open rellfy opened this issue 2 years ago • 7 comments

The current LedgerSigner does not pass the correct TX parameters for EIP1559, which results in the gas price being zero and therefore the transaction never being sent.

rellfy avatar Sep 17 '21 11:09 rellfy

I've just realised #1971 does the same.

rellfy avatar Sep 17 '21 11:09 rellfy

Tested locally, and it worked for me.

maraoz avatar Oct 08 '21 05:10 maraoz

This PR is very useful, anything that I can do to help get it over the line?

tynes avatar Oct 21 '21 21:10 tynes

Hi, @ricmoo how can we help to get this one merged and released? Since the London fork on the xDai network, a few days ago, it is not possible to send a tx using a ledger wallet 🤕

0xGabi avatar Nov 16 '21 04:11 0xGabi

My problem was related to this article. In case someone else experience a similar problem: https://www.ledger.com/blog/metamask-and-ledger-integration-fixed-new-partnership-between-metamask-and-ledger-to-ensure-ease-of-use-and-security-for-our-shared-customers

0xGabi avatar Nov 16 '21 20:11 0xGabi

Tested this locally and it worked for me too

cryppadotta avatar Jan 08 '22 16:01 cryppadotta

Please, can you merge this if it works ?

megasyl avatar Aug 01 '22 20:08 megasyl

@ricmoo I have tested this PR, and it appears to work. Others have also reported testing this, and it appears ready to merge. Can we merge this for the next release? This bug is causing trouble with my hardhat scripts requiring me to use the old transaction format awaiting this PR. This PR has been ready for over a year, and I would like to know if you're awaiting anything before merging this in. If you need help or need to see something more before merging in, I would gladly offer it to get this code in the next release.

Brando753 avatar Nov 02 '22 19:11 Brando753

I was not planing to re-add this into the main package in v5, as I'm trying to minimize changes in preparation for v6 (in v6 this is a separate package, which makes things much simpler). It should be fairly simple to break out the files into a new package, or even just include them directly in specific projects as an additional single file.

The CI gets complicated for this, as the needed USB/HID libraries kept changing, which required brew install-ing a bunch of development headers, kernel libraries, etc. on MacOS targets and apt-get install-ing packages on Linux. And depending on the environment, changing group ownership of /dev/ items to make it work in user space.

By keeping it a separate package in v6, it will be easier to isolate it and reduce the number of dependencies needed for the core.

ricmoo avatar Nov 02 '22 20:11 ricmoo

Hey @ricmoo, thanks for replying. I am looking forward to the V6 release and greatly appreciate the work you have put into this project. The EthersJS library is such a nicer alternative to the Web3 library. I can understand wanting to minimize changes before a big release, but this is a minimal change and ensures the V5 has uniform EIP-1559 support. This does a version bump on two dependencies and then passes the missing params to the base transaction for Ledger. Without this change, ledger support is effectively neutered as a Signer in Ethersjs. If the concern is integration testing and needing to update tests to give full coverage here, I would be happy to look at helping. Still, I would appreciate it if this change was made available as soon as possible, as those of us who use Ethers with hardhat cannot take advantage of EIP-1559. Regardless if the LedgerSigner is moved into a separate package for V6, this work will still have to be done, right?

Brando753 avatar Nov 02 '22 23:11 Brando753

it's really a good feature for our hardhat users

jqnote avatar Dec 27 '22 14:12 jqnote