Skip to content

Conversation

rellfy
Copy link

@rellfy rellfy commented Sep 17, 2021

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
Copy link
Author

rellfy commented Sep 17, 2021

I've just realised #1971 does the same.

@maraoz
Copy link

maraoz commented Oct 8, 2021

Tested locally, and it worked for me.

@tynes
Copy link

tynes commented Oct 21, 2021

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

@0xGabi
Copy link

0xGabi commented Nov 16, 2021

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
Copy link

0xGabi commented Nov 16, 2021

@cryppadotta
Copy link

Tested this locally and it worked for me too

@megasyl
Copy link

megasyl commented Aug 1, 2022

Please, can you merge this if it works ?

@Brando753
Copy link

@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.

@ricmoo
Copy link
Member

ricmoo commented Nov 2, 2022

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.

@Brando753
Copy link

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?

@jqnote
Copy link

jqnote commented Dec 27, 2022

it's really a good feature for our hardhat users

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

Successfully merging this pull request may close these issues.

10 participants