search

mikeshultz / ledger-eth-lib

Library to interface with Ethereum app on Ledger hardware wallets
https://ledgereth.readthedocs.io/
MIT License
19 stars 9 forks source link

EIP-1559 Transactions #9

Closed antazoey closed 2 years ago

antazoey commented 2 years ago

Hello!

This is a feature request for EIP-1559 support.

Thank you for this repo, it has been a nice resource to look at.

I have been trying implement this myself using the eth-account library in another project but am running into issues

mikeshultz commented 2 years ago

Looks like it should be possible as ledger app-ethereum appears to added support some months ago. I'll poke around, maybe this weekend.

antazoey commented 2 years ago

Yes I can get it to work when I already have the encoded transaction. It is just the encoding of the transaction I am struggling with

Edit:

https://github.com/LedgerHQ/app-ethereum/pull/157/files#r815388944

^ The link you posted included an example of an already-encoded 1559 transaction. That is great! And eth-account lib has an example of encoding one but I don't think it is working correctly.

mikeshultz commented 2 years ago

Just some notes as I'm going through this.

Encoding seems to be a bit different. EIP-2718 defines typed transactions. There's a handful of different unsigned encoded transaction types:

The encoding of type 2 does not seem to appear in EIP-1559, so I'm still digging that up. I'll probably want to add Type 1 transactions to this as well, presuming they work with Ledger's app-ethereum.

ETA: Added a logical guess from EIP-1559 for encoding type 2 transactions.

mikeshultz commented 2 years ago

The linked page for EIP-1559 is kind of out of date(oops). Actual encoding should be like this for type 2 transactions:

0x02 || rlp([chain_id, nonce, max_priority_fee_per_gas, max_fee_per_gas, gas_limit, destination, amount, data, access_list])

Testing this encoding out, but I'm having some issues getting this to work. Ledger seems to think the data is out of order. It does look like it expects TransactionType to be packed with typed transactions which I'm doing:

https://github.com/LedgerHQ/app-ethereum/blob/12ebced7bf65f0aa808f3afec73aa40ad80aeff9/src_features/signTx/cmd_signTx.c#L47

I'm not super great with C though and have no idea what this is:

https://github.com/LedgerHQ/app-ethereum/blob/12ebced7bf65f0aa808f3afec73aa40ad80aeff9/src_features/signTx/cmd_signTx.c#L51

Looking through how it processes fields, they seem to be in the order of this enum (since it increments a field index as it processes the bytes:

https://github.com/LedgerHQ/app-ethereum/blob/f0f47e4250430edce99967742d9a16c9ee8bd29c/src_common/ethUstream.h#L81-L95

Which looks like it matches the definition in the EIP. So, I have something I think should work, but it doesn't. Need to verify my code a bit and will try some things out. Was hoping this would be a quick update but guess not. Will update if I learn more.

mikeshultz commented 2 years ago

Success!

I was able to decode the tx they're using in their test case (which I think is actually invalid, anyway because the bribe and max fee were swapped) and use that to figure out what was wrong with my implementation. Looks like I wasn't RLP encoding access_list properly. I now have a successful implementation and I've been able to sign a valid transaction. I'm kind of crashing however, and probably won't be wrapping it up tonight. I still want to do a bit more testing to be sure this works and then clean up my quick & dirty implementation. Should probably be able to take care of that and probably release tomorrow night.

antazoey commented 2 years ago

Thank you so much for figuring this out; it's a good teaching moment for me, as you documented your journey.

mikeshultz commented 2 years ago

Done and released in 0.3.0. Thanks for the feature request.

https://pypi.org/project/ledgereth/0.3.0/