Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upgrade ethereumjs/tx support #68

Merged
merged 1 commit into from
Jun 10, 2021
Merged

Conversation

brad-decker
Copy link
Contributor

@brad-decker brad-decker commented Apr 16, 2021

This pull request enables consumers to use newer versions of @ethereumjs/tx (previously ethereumjs-tx). This enables support for typed transaction envelopes, and features built upon it such as optional access lists.

Support for the oldest supported version of ethereumjs-tx prior to this change is maintained. I have not confirmed if this will work for all versions of ethereumjs-tx / @ethereumjs/tx between those two points.

Challenges:

  1. @ethereumjs/tx defaults to immutable objects, which is great... however the previous version of the signTransaction method relied upon mutability. The immutability is opt-out but in an effort to allow for taking advantage of immutability at least in the user's domain (not in this repo), this converts immutable Transactions into mutable versions temporarily, then returns an object that is immutable, if the provided transaction object was immutable.
  2. @ethereumjs/tx has a new 'Common' utility that sets things like chain and hardfork support. This is passed in the options object for TransactionFactory.fromTxData. In order to not lock ourselves into having to pass this object around between the consumer and this package, the common options are lifted from the provided transaction object and reapplied to the copies created.
  3. ethereumjs-tx and @ethereumjs/tx have differing accessor methods and construction methods. to work through this I have split the implementation of these two paths up, keeping the most shared code as possible while also creating branches wherein we can rest assured we are dealing with the appropriate type of object.
  4. Validation of provided parameters is much more strict in @ethereumjs/tx as a result the test case uses a mutable object and stubs out many methods that would otherwise cause tests to fail. I will be following this up with a PR that changes the test structure to use a private key for the HDkey initialization and allows more appropriate levels of testing.

@brad-decker brad-decker marked this pull request as ready for review April 17, 2021 20:04
@brad-decker brad-decker requested a review from a team as a code owner April 17, 2021 20:04
@brad-decker brad-decker force-pushed the upgrade-ethereumjs-tx-support branch from 1d57172 to d156d0f Compare June 8, 2021 20:06
@darkwing
Copy link
Contributor

darkwing commented Jun 9, 2021

I've completed testing of this PR on both develop and poc of `metamask extension:

Develop

  • Sign: works!
  • Personal Sign: works!
  • Typed Data v4 Sign: works!
  • Send: works!

POC Branch (https://github.com/MetaMask/metamask-extension/pull/11194/files)

  • Sign: works!
  • Personal Sign: works!
  • Typed Data v4 Sign: works!
  • Send: Error :(. I tried sending 1 ETH to myself on Rinkby. I submitted the transaction, approved on my Ledger device, and the transaction immediately failed. The background process error was:
err here is the error [ethjs-query] while formatting outputs from RPC '{"value":{"code":-32000,"message":"invalid sender"}}'

Error in RPC response:
 
Object { id: "8e2f4d36-7ffb-46c5-a3a6-621b33c40fea", jsonrpc: "2.0", error: {…} }
​
error: Object { code: -32603, message: "Error: [ethjs-query] while formatting outputs from RPC '{\"value\":{\"code\":-32000,\"message\":\"invalid sender\"}}'", stack: "Error: Error: [ethjs-query] while formatting outputs from RPC '{\"value\":{\"code\":-32000,\"message\":\"invalid sender\"}}'" }
​
id: "8e2f4d36-7ffb-46c5-a3a6-621b33c40fea"
​
jsonrpc: "2.0"
​
<prototype>: Object { … }
createLoggerMiddleware.js:16:12
[ethjs-query] while formatting outputs from RPC '{"value":{"code":-32000,"message":"invalid sender"}}' backend.js:32
    n backend.js:32

It appears you were expecting a possible issue there (https://github.com/MetaMask/metamask-extension/pull/11194/files#diff-675c437262d906486cf5436c0b64535347286cc4d1081dbd863fb45b8a01b81aR624) =

@brad-decker
Copy link
Contributor Author

I was unable to recreate the error with sending on rinkeby, but could reliably recreate that error message sending from any other chain due to the hardcoded rinkeby common implementation in the POC.

@darkwing
Copy link
Contributor

Retested on POC and develop and both work great!

@brad-decker brad-decker merged commit d1cae99 into main Jun 10, 2021
@brad-decker brad-decker deleted the upgrade-ethereumjs-tx-support branch June 10, 2021 16:48
julianariel pushed a commit to block-wallet/eth-ledger-bridge-keyring that referenced this pull request Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants