Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

Upgrade ethereumjs libraries #88

Merged
merged 1 commit into from
Jun 10, 2021
Merged

Upgrade ethereumjs libraries #88

merged 1 commit into from
Jun 10, 2021

Conversation

brad-decker
Copy link
Contributor

@brad-decker brad-decker commented May 27, 2021

Adds support for new versions of ethereumjs/tx to the trezor implementation.

@brad-decker brad-decker requested a review from darkwing May 27, 2021 02:18
@brad-decker
Copy link
Contributor Author

Note: https://consensys.slack.com/archives/G1L7H42BT/p1621894614057900 is relevant for how to test this.

@darkwing darkwing changed the title wip Upgrade ethereumjs libraries. Jun 4, 2021
@darkwing darkwing changed the title Upgrade ethereumjs libraries. Upgrade ethereumjs libraries Jun 4, 2021
@darkwing
Copy link
Contributor

darkwing commented Jun 4, 2021

I've tested this using my Trezor device using the test dapp and all of my testing of signing worked as they should.

When you get back, let's briefly sync on this and choose the best path forward. Awesome work!

@darkwing
Copy link
Contributor

darkwing commented Jun 8, 2021

Retested this PR with current develop branch of metamask-extension and the test dapp:

  • Sign works as expected
  • Personal sign works as expected
  • Send fails with Forbidden key path, but it also fails for me on current production MM (9.5.9), so I don't believe it is related

Restested this PR with current poc (MetaMask/metamask-extension#11194) and the test dapp:

  • Sign works as expected
  • Personal sign works as expected
  • Send - Trezor window pops up and I get an infinite spinner -- never asks me to send or bomb out with the Forbidden key path error I saw on develop.

I should also say that I restored a Ledger mneumonic on this Trezor, though I don't know that that matters.

@brad-decker brad-decker marked this pull request as ready for review June 9, 2021 21:57
@brad-decker brad-decker requested a review from a team as a code owner June 9, 2021 21:57
@brad-decker brad-decker force-pushed the update-ethereumjs-tx branch 2 times, most recently from 42dff70 to 5c690ec Compare June 9, 2021 22:23
.stub(newFakeTx, 'getSenderAddress')
.callsFake(() => fakeAccounts[0]);
sinon.stub(newFakeTx, 'verifySignature').callsFake(() => true);
// sinon.stub(newFakeTx, 'from').get(() => fakeAccounts[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove this (?)

@brad-decker brad-decker force-pushed the update-ethereumjs-tx branch from 5c690ec to e134430 Compare June 10, 2021 16:05
Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works on both POC branch and develop. Nice!

@brad-decker brad-decker merged commit 5030dd7 into main Jun 10, 2021
@brad-decker brad-decker deleted the update-ethereumjs-tx branch June 10, 2021 16:49
aloisklink added a commit to aloisklink/eth-trezor-keyring that referenced this pull request Sep 20, 2021
Creating an unfrozen transaction (added in MetaMask#88) seems to be a change
that was only required in eth-ledger-keyring, not in Trezor,
and is fixed by @ethereumjs/tx: v3.1.4 anyway.

I've removed this part,
since it was causing issues with EIP-1559 transactions, and does
not seem necessary for non-EIP-1559 transactions either.
aloisklink added a commit to aloisklink/eth-trezor-keyring that referenced this pull request Sep 20, 2021
Creating an unfrozen transaction (added in MetaMask#88) seems to be a change
that was only required in eth-ledger-keyring, not in Trezor,
and is fixed by @ethereumjs/tx: v3.1.4 anyway.

I've removed this part,
since it was causing issues with EIP-1559 transactions, and does
not seem necessary for non-EIP-1559 transactions either.
aloisklink added a commit to aloisklink/eth-trezor-keyring that referenced this pull request Oct 17, 2021
Creating an unfrozen transaction (added in MetaMask#88) seems to be a change
that was only required in eth-ledger-keyring, not in Trezor,
and is fixed by @ethereumjs/tx: v3.1.4 anyway.

I've removed this part,
since it was causing issues with EIP-1559 transactions, and does
not seem necessary for non-EIP-1559 transactions either.
danjm pushed a commit that referenced this pull request Nov 17, 2021
Creating an unfrozen transaction (added in #88) seems to be a change
that was only required in eth-ledger-keyring, not in Trezor,
and is fixed by @ethereumjs/tx: v3.1.4 anyway.

I've removed this part,
since it was causing issues with EIP-1559 transactions, and does
not seem necessary for non-EIP-1559 transactions either.
danjm added a commit that referenced this pull request Nov 18, 2021
* Upgrade trezor-connect to 8.2.1

This adds support for EIP-1559 for the Trezor T,
once we get all this library to work with it as well.

We use version "8.2.1-extended" exactly,
as we need non-browser support for local tests.

* Update @ethereumjs/common to 2.4.0 for 1559 tests

* Upgrade @ethereumjs/tx dependency to ^3.2.1

Although 3.3.0 is out, I've selected ^3.2.1 just
to match the current version of MetaMask's package.json file.

Version 3.2.0 adds support for EIP-1559 transactions.

* Test EIP-1559 transaction support

Test case copied over from
MetaMask/eth-ledger-bridge-keyring#99

* Set ESLint ecmaVersion to 2018

This adds support for object-rest-spread (e.g. {...x, ...y}),
without using the babel parser, which we use in tests.

This is different from what the Metamask extension has, which
is 2017, but they're using the babel parser.

* Add EIP-1559 transaction support

Creating an unfrozen transaction (added in #88) seems to be a change
that was only required in eth-ledger-keyring, not in Trezor,
and is fixed by @ethereumjs/tx: v3.1.4 anyway.

I've removed this part,
since it was causing issues with EIP-1559 transactions, and does
not seem necessary for non-EIP-1559 transactions either.

* Document EIP1559 support for Model T on README.md

* Add method to expose trezor model

* Lint fix

* Update index.js

Co-authored-by: Alois Klink <[email protected]>

* Use event listener instead of getFeatures call to set model

* Lint fix

* Revert getEIP1559Support to getModel

* Move event listener first and trigger event via init

* Lint fix

* Remove error handle in getModel method

Co-authored-by: Alois Klink <[email protected]>
Co-authored-by: David Walsh <[email protected]>
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.

2 participants