Add support for Ethereum EIP 1559 #1653
Conversation
|
This PR looks to be in a working state after a bit of back and worth. I unfortunately hadn't read up on your commit history policy before starting the work, so feel free to let me know if you want anything changed. I look forward to starting the review process and hopefully we can get this merged before EIP 1559 goes live on the Ethereum mainnet. I assume that adding support to Trezor Connect would be the next step after that as well 🤔 |
|
@matejcik How would you prefer I handle the conflicts? Should I wait until you finish the review to do rebasing etc? |
yes, please do not rebase or force-push while the review is ongoing. |
Yeah, that's what I thought. Just wanted to confirm. Let me know when you want me to handle the conflicts then. |
There was a problem hiding this comment.
I finally did a detailed review and proposed some fundamental changes to the protobuf. If you agree that the changes make sense, please implement them.
Now would be a good time to rebase, but you might run into a problem with the layout function (see comment), so I leave it up to you if you want to implement that now, or if you want to hold off rebasing until we implement the confirm_amount function on our side.
|
in any case, thanks for working on this so far! |
No worries! Thanks for reviewing! I did a few commits to simplify and fix some of the issues you pointed out. For a few of your comments I suggested a different approach or disagreed with your suggestion. Looking forward to your responses on my comments and a re-review. Will also rebase when you guys have the layout stuff merged, let me know when that is. |
|
just a note, do not mark my comments resolved, instead comment with "fixed" or commit hash in which the comment is resolved. only the person who started the comment should resolve it |
Sure thing! My bad. |
Hope you had a nice vacation, welcome back! 😄 I looked through your comments and addressed them all today. Good for another round of review and perhaps some discussion in a few of the comments! |
FrederikBolding
force-pushed
the
eth-eip-1559
branch
from
2fd1b76
to
fd56d3e
Compare
|
@matejcik I have rebased today and moved the UI to the new file and included the new access_list code. This has somehow broken my dev environment. I get |
|
is it possible that you still have |
|
@matejcik With the rebase I have fixed the access_list RLP comments and moved the UI layout to the same place as the legacy components. In my optics the only blocking things are now if you want to wait for the new layout function and the possibility of using another message type for EIP 1559 transactions. I have commented on both these open discussions. |
There was a problem hiding this comment.
Fínal (hopefully) round of comments for the core code.
One thing i'm wondering about is the chain_id calculation in sign_digest, which you removed. I don't think we actually need it, as it can always be always done on the client. But still wondering.
Also maybe worth considering setting chain_id to bytes in protobuf right now? (see #1741)
|
I pushed f60c48c to your branch that fixes build on legacy. |
🤞
As explained in another comment, this is not needed for EIP 1559 transactions!
I would prefer not to at this point, because it might add extra complexity and require even more back and forth, reviews, etc. If that is okay with you 😄 |
There was a problem hiding this comment.
squashed & rebased as 204d31e
pipeline running at https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/pipelines/350729690
i'll still need to add a changelog entry
Initial EIP1559 implementation Fix a few small issues Progress on Python lib implementation and firmware Fix RLP length Start fixing tests Fix legacy transactions Simplify API and logic Add EIP1559 tests Fix access list formatting Fix UI visiblity issue Fix commented out code fix: correct linting issues Fix access_list protobuf formatting Remove unneeded code Remove dead code Check tx_type bounds for EIP 2718 Reduce code duplication Prefer eip2718_type over re-using tx_type Add more tests Simplify format_access_list Simplify sign_tx slightly Change Access List format and add logic to encode it Fix a bunch of small PR comments Fix a linting issue Move tests out of class and regenerate Remove copy-pasted comments Add access list to CLI Simplify _parse_access_list_item Fix small mistakes following rebase Fix linting Refactor to use a separate message for EIP 1559 tx Simplify changed legacy code Fix a few small PR comments Fix linting fix(legacy): recognize SignTxEIP1559 on legacy build Fix PR comments
[no changelog]
Cool! What needs to be done for trezor-connect? Just regen the protobuf and add params for EIP1559? Is that something you guys are doing or not? |
It's on the roadmap but nobody is working on it currently. PR will be appreciated :) I am not sure what's needed honestly. There's some tomfoolery about old versions of protobuf in Connect, so YMMV. |
Okay 🤔 It looks like all tests passed except for the changelog thing and something related to the new message? |
|
should be fixed now as a398704, pipeline at https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/pipelines/350749141 |
Go ahead 😄 |
|
All green! 😄 Does my changes automatically reflect in https://github.com/trezor/trezor-common? |
|
not automatically but I just pushed it out |
|
so this should be all on firmware side! wonderful. thanks again for your contribution and work on this |
Indeed! Thanks so much for all the reviews 😄 |
Fixed a breaking change with Trezor Connect v8. This commit does NOT enable EIP-1559, but is ready to be activated once the new firmware is available. Ref: trezor/trezor-firmware#1653
|
Issue found: |
Fixes #1604
The following PR adds support for typed Ethereum transactions (EIP 2718) with a focus on EIP 1559. This changes the format of transactions to include the transaction type, new gas parameters as well as an optional access list.
It also adds a new UI screen to show the new gas parameters.
I have attempted to re-use as much of the existing transaction signing code as well as added support in the Python lib.
I have repurposed some of the existing tests to cover most of the same cases with the new format. On top of that the PR can be (and has been) tested on the Calaveras testnet (as well as more networks coming later this month).
Resources
https://eips.ethereum.org/EIPS/eip-2718
https://eips.ethereum.org/EIPS/eip-1559
https://eips.ethereum.org/EIPS/eip-2930
https://github.com/ethereum/eth1.0-specs/blob/master/network-upgrades/client-integration-testnets/calaveras.md