Skip to content
This repository has been archived by the owner on Nov 23, 2023. It is now read-only.

Add support for Ethereum EIP 1559 #871

Closed
wants to merge 21 commits into from

Conversation

FrederikBolding
Copy link
Contributor

@FrederikBolding FrederikBolding commented Aug 10, 2021

  • Updated Trezor Common to include new EIP 1559 message
  • Add new method for signing EIP 1559 transactions

@FrederikBolding
Copy link
Contributor Author

Heya! Hoping to start a conversation about how to implement trezor/trezor-firmware#1653 in the JS library.

@FrederikBolding FrederikBolding marked this pull request as ready for review August 10, 2021 11:56
@FrederikBolding FrederikBolding marked this pull request as draft August 10, 2021 11:56
@FrederikBolding
Copy link
Contributor Author

@szymonlesisz Sorry for the ping, but looking to start the conversation about implementing this.

I haven't figured out how to test with the new firmware build, but I think the code here should work with that.

Curious to hear your thoughts on whether you would rather use the same function for both the old and new transaction types.

I can also add more test cases once we get some progress on this.

@matejkriz
Copy link
Member

matejkriz commented Aug 10, 2021

Hi @FrederikBolding , thanks for the draft!

We were thinking more of keeping only EthereumSignTx method, the difference is really small and would be nice to handle the selection of protobuf message here inside trezor-connect.

What mechanism are you planning to use in your app for detection if firmware of connected device supports the new message? For Suite we would like to define new unavailable capability here https://github.com/trezor/connect/blob/develop/src/data/config.json#L106 . So the condition for protobuf message selection could look something like !device.unavailableCapabilities.eip1559 && !!max_priority_fee ? EthereumSignTxEIP1559 : EthereumSignTx.

I could help you to run the new firmware in emulator, if you are interested. Starting point is https://github.com/trezor/trezor-firmware/blob/master/docs/core/build/emulator.md

@FrederikBolding
Copy link
Contributor Author

Hi @FrederikBolding , thanks for the draft!

We were thinking more of keeping only EthereumSignTx method, the difference is really small and would be nice to handle the selection of protobuf message here inside trezor-connect.

Sounds good to me, this is my preferred solution as well. I will make appropriate changes for this!

What mechanism are you planning to use in your app for detection if firmware of connected device supports the new message? For Suite we would like to define new unavailable capability here https://github.com/trezor/connect/blob/develop/src/data/config.json#L106 . So the condition for protobuf message selection could look something like !device.unavailableCapabilities.eip1559 && !!max_priority_fee ? EthereumSignTxEIP1559 : EthereumSignTx.

We haven't thought to much about that to be honest, but if you have a good way of doing that I don't have problem with it! What would be the format of adding that capability?

I could help you to run the new firmware in emulator, if you are interested. Starting point is https://github.com/trezor/trezor-firmware/blob/master/docs/core/build/emulator.md

I have the new firmware running in an emulator from when I wrote the firmware PR. But I am unsure of how this repos tests with it 😄

@FrederikBolding
Copy link
Contributor Author

@matejkriz I have simplified to use the same method for both calls. Still awaiting some instructions on tests as well as unavailableCapabilities.

@matejkriz
Copy link
Member

@FrederikBolding regarding the testing, when you run this repo locally (yarn run dev), you need to visit https://localhost:8088/ first in your browser to confirm the certificate and then you could use it in your app or use connect-explorer: https://trezor.github.io/connect-explorer/?trezor-connect-src=https://localhost:8088/#/method/ethereumSignTransaction (it needs page reload on any change in trezor-connect)

@matejkriz
Copy link
Member

Format for the new unavailableCapability in https://github.com/trezor/connect/blob/develop/src/data/config.json#L146 could be

,
{
  "excludedMethods": ["eip1559"],
  "min": ["0", "2.4.2"],
  "comment": ["new eth transaction pricing mechanism (EIP1559) since 2.4.2"]
}

@FrederikBolding
Copy link
Contributor Author

FrederikBolding commented Aug 12, 2021

@FrederikBolding regarding the testing, when you run this repo locally (yarn run dev), you need to visit https://localhost:8088/ first in your browser to confirm the certificate and then you could use it in your app or use connect-explorer: https://trezor.github.io/connect-explorer/?trezor-connect-src=https://localhost:8088/#/method/ethereumSignTransaction (it needs page reload on any change in trezor-connect)

I've gotten this far, but how do I force Trezor Connect to talk to the emulator instead of a normal device and how do I run the tests on the emulator?

@szymonlesisz
Copy link
Contributor

Hi @FrederikBolding ,

First of all: WOW big thanks for the integration!
I will have some small nit-picks later but it looks really nice at first glance :)

you can run tests with emulator ./tests/run.sh -i ethereumSignTransaction (see tests docs)

or if you wish to work with the emulator during integration im recommending https://github.com/trezor/trezor-user-env docker image - it contains multiple firmwares including 2.4.2 (2-master)

@FrederikBolding FrederikBolding changed the title [WIP] Add support for Ethereum EIP 1559 Add support for Ethereum EIP 1559 Aug 12, 2021
@FrederikBolding FrederikBolding marked this pull request as ready for review August 12, 2021 22:10
@FrederikBolding
Copy link
Contributor Author

What I have now works and passes tests with fixtures copy-pasted from the test cases I created in the firmware. Good to start an actual review.

Let me know if you have any questions 😄

@szymonlesisz
Copy link
Contributor

@FrederikBolding ,

Sorry for taking so long but i've found a problem with unavailableCapabilities so i needed to fix it here before merging this.
TLDR: eip1559 unavailableCapability added here was not shown properly

I've rebased your branch on top of my PR with the fix, squashed all your commits into one and added some fixups from me:

https://github.com/trezor/connect/pull/874/commits

in f94aecf
i've changed field name in config.json (related to PR mentioned above) + added few unit tests

in 1dcdeaa
i've changed flowtype/typescript definitions a litte bit + added few types tests

in e216b0e
i've updated the code to validate firmware range before running method (to prevent signing eip1559 with T1 - which is not supported) + minor changes to make flowtype happy

in 1b56b9e
i've moved eip1559 fixtures into standalone file + added 2 missing cases from firmware repo (erc20 transfer)

Now all tests are passing
Let me know if you are ok with my fixups so i can squash them

@szymonlesisz szymonlesisz mentioned this pull request Aug 18, 2021
@FrederikBolding
Copy link
Contributor Author

FrederikBolding commented Aug 18, 2021

@szymonlesisz

@FrederikBolding ,

Sorry for taking so long but i've found a problem with unavailableCapabilities so i needed to fix it here before merging this.
TLDR: eip1559 unavailableCapability added here was not shown properly

I've rebased your branch on top of my PR with the fix, squashed all your commits into one and added some fixups from me:

https://github.com/trezor/connect/pull/874/commits

No worries about the wait!

in e216b0e
i've updated the code to validate firmware range before running method (to prevent signing eip1559 with T1 - which is not supported) + minor changes to make flowtype happy

It seems you removed this.device.unavailableCapabilities.eip1559, how is it then detected if a user tries to send a type 2 tx without having the proper firmware? I might be misunderstanding how the firmwareRange code works?

Now all tests are passing
Let me know if you are ok with my fixups so i can squash them

Other than the small question above, everything else looks good to me. You can push your squashed commits to my branch if you want. I don't know what you prefer.

Thanks in advance!!

@szymonlesisz
Copy link
Contributor

I might be misunderstanding how the firmwareRange code works?

validation of capabilities in EthereumSignTranaction.run is done "too late", from this place you can either return success or failure, there is no "interaction with user" to tell him what is going on.

firmwareRange check is used before EthereumSignTranaction.run and user will be noticed in popup about required update

Screenshot from 2021-08-18 15-15-34

checking for i can use it + i have eip1559 params gives wrong result when i CANT use it + but i have eip1559 params example: running ethereumSignTransaction with eip1559 params on 2.4.1 where support is disabled by config.json will fall to the legacy signing block and will use eip1559 params in helper.ethereumSignTx

@FrederikBolding
Copy link
Contributor Author

I might be misunderstanding how the firmwareRange code works?

validation of capabilities in EthereumSignTranaction.run is done "too late", from this place you can either return success or failure, there is no "interaction with user" to tell him what is going on.

firmwareRange check is used before EthereumSignTranaction.run and user will be noticed in popup about required update

Screenshot from 2021-08-18 15-15-34

checking for i can use it + i have eip1559 params gives wrong result when i CANT use it + but i have eip1559 params example: running ethereumSignTransaction with eip1559 params on 2.4.1 where support is disabled by config.json will fall to the legacy signing block and will use eip1559 params in helper.ethereumSignTx

Sounds good!

@FrederikBolding
Copy link
Contributor Author

Closing in favor of #874

@puruoni
Copy link

puruoni commented Aug 29, 2021

Hi, thanks for the EIP-1559 support!
I'd like to use it with Trezor One, but it's not possible?

⚠️ Supported only by Trezor T with Firmware 2.4.2 or higher!

@FrederikBolding
Copy link
Contributor Author

Hi, thanks for the EIP-1559 support!
I'd like to use it with Trezor One, but it's not possible?

⚠️ Supported only by Trezor T with Firmware 2.4.2 or higher!

Unfortunately it is not supported!

@FrederikBolding
Copy link
Contributor Author

@szymonlesisz Sorry for the ping, but not sure where else to contact. Is this supposed to be releasing today? I saw the library was updated, but using the latest version throws Parameter \"gasPrice\" is missing., so I assume not everything is updated. How/when is the update coming out?

@shroom-dev
Copy link

Hi, will EIP-1559 support be added to Trezor One at some point soon?

@greenlucid
Copy link

greenlucid commented Oct 21, 2021

Is it never going to be supported for Trezor One?
That would be so lame. You know there's an offer from Eth core multisig for this, right?

@0xJatto
Copy link

0xJatto commented Oct 31, 2021

No comment?

@liszper
Copy link

liszper commented Nov 8, 2021

@FrederikBolding I upgraded to 2.4.2, latest MetaMask too, but my Trezor T still doesn't support direct "write contract" on etherscan.io, getting "Invalid transaction params: params specify an EIP-1559 transaction but the current network does not support EIP-1559" with the latest Metamask version or very high gas costs (indicating there is an error) with older versions of Metamask.

I've spent months trying to solve this with dozens of MetaMask and Trezor firmware versions and now It's becoming a significant problem for me. Could you help me please?

@FrederikBolding
Copy link
Contributor Author

@FrederikBolding I upgraded to 2.4.2, latest MetaMask too, but my Trezor T still doesn't support direct "write contract" on etherscan.io, getting "Invalid transaction params: params specify an EIP-1559 transaction but the current network does not support EIP-1559" with the latest Metamask version or very high gas costs (indicating there is an error) with older versions of Metamask.

I've spent months trying to solve this with dozens of MetaMask and Trezor firmware versions and now It's becoming a significant problem for me. Could you help me please?

I'm not quite sure whether MetaMask has rolled out 1559 support for Trezor at this point.

But if it is not working for you, you can use https://app.mycrypto.com/interact-with-contracts to interact with contracts using Trezor Model T. If you have any issues feel free to reach out on our Discord or to our support team!

@guff-se
Copy link

guff-se commented Nov 27, 2021

Wow. Just bought a Trezor One for the specific purpose of managing Ethereum contracts. Am I understanding it correctly that there is no plan for EIP-1559 support for the One? So I can basically trash it and buy something else? :'(

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

Successfully merging this pull request may close these issues.

None yet

9 participants