search

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

gasPrice is set to 0 with LedgerSigner for EIP 1559 transactions #2

Closed bugduino closed 1 year ago

bugduino commented 3 years ago

Hi, I'm using "ethers": "5.4.7" with "@ethersproject/hardware-wallets": "5.4.0" in an hardhat project and I'm trying to sign a transaction with LedgerSigner but the transaction gas price is always set to 0.

After debugging a bit it seems that the problem lies here: https://github.com/ethers-io/ethers.js/blob/master/packages/hardware-wallets/src.ts/ledger.ts#L96-L108

the transaction argument and the subsequent tx variable are in this form for eip 1559 transactions:

{                                                                                                         
  data: '0x...',
  to: '0x...',
  from: '0x...',
  type: 2,
  maxFeePerGas: BigNumber { _hex: '0x1741c8ba38', _isBigNumber: true },
  maxPriorityFeePerGas: BigNumber { _hex: '0x3b9aca00', _isBigNumber: true },        
  nonce: 1,                                                                                             
  gasLimit: BigNumber { _hex: '0x8cc5', _isBigNumber: true },      
  chainId: 1 
}

but when building the baseTx object the only reference used for setting the gas price is the gasPrice key which is not present anymore so gasPrice is always undefined.

I've made a simple fix locally by changing how gasPrice is built so to be able to make it work

gasPrice: (tx.gasPrice || tx.maxFeePerGas || undefined)

but I'm sure that there a better way to manage this

maraoz commented 3 years ago

Found the same problem today migrating a hardhat deployment script to use LedgerSigner, and can confirm this solved the problem:

gasPrice: (tx.gasPrice || tx.maxFeePerGas || undefined),

I'm not entirely sure how easy it would be to make LedgerSigner support EIP 1559, but at least this allows sending old-style transactions using ethers.js + a Ledger hardware wallet.

maraoz commented 3 years ago

Ah, apparently someone already solved it here: https://github.com/ethers-io/ethers.js/pull/1971/

acryptosx commented 2 years ago

ethers-io/ethers.js#1971 is still not merged?

I worked around it by adding { type: 1 } to the overrides when creating the transaction.

smartcontracts commented 2 years ago

Found the same root cause after 2 hours of frustrating debugging. Solution that @maraoz described seems like a good short-term option and would probably save people lots of headache until proper EIP 1559 support can be added. Can confirm that as @acryptosx adding { type: 1 } to overrides works as a workaround for now.

asardon commented 1 year ago

ran into the same issue, others might want to use frame, worked like a charm: https://github.com/NomicFoundation/hardhat/issues/1159#issuecomment-789310120

ricmoo commented 1 year ago

The new @ethers-ext/signer-ledger (this package) should has been completely re-written. This issue should be addressed by the latest version, so I'm going to close this. If you still experience a problem, please re-open.

Thanks! :)

bugduino commented 1 year ago

Thanks for the update @ricmoo, is the package correctly published on npm / yarn registry? I tried installing it but it seems that is not present as npm i @ethers-ext/signer-ledger yields 

Not Found - GET https://registry.npmjs.org/@ethers-ext%2fsigner-ledger - Not found

'@ethers-ext/signer-ledger@*' is not in this registry.
ricmoo commented 1 year ago

Sorry! Looks like I hadn’t published it to npm. It is now published. :)