Skip to content
This repository was archived by the owner on Jun 27, 2022. It is now read-only.

Add EIP1559 and EIP2930 support #637

Merged
merged 5 commits into from
Aug 1, 2021
Merged

Conversation

FrederikBolding
Copy link
Contributor

@FrederikBolding FrederikBolding commented Jul 31, 2021

Small changes required to support the new transaction formats for the new transaction types.

To be merged with changes here: LedgerHQ/app-ethereum#157

@FrederikBolding FrederikBolding changed the title Add EIP 1559 and EIP2930 support Add EIP1559 and EIP2930 support Jul 31, 2021
@FrederikBolding
Copy link
Contributor Author

Thanks @pscott for help with the APDUs 😄

@codecov
Copy link

codecov bot commented Jul 31, 2021

Codecov Report

Merging #637 (9f8126f) into master (0772c5e) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #637      +/-   ##
==========================================
+ Coverage   42.22%   42.31%   +0.08%     
==========================================
  Files          74       74              
  Lines        4497     4504       +7     
  Branches      787      791       +4     
==========================================
+ Hits         1899     1906       +7     
  Misses       2551     2551              
  Partials       47       47              
Impacted Files Coverage Δ
packages/hw-app-eth/src/Eth.ts 86.09% <100.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0772c5e...9f8126f. Read the comment docs.

Copy link
Contributor

@gre gre left a comment

Choose a reason for hiding this comment

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

Thanks for contribution!

Internal note: It's curious that we were not future proof via usage of ethers lib but I feel we use a too low level utility from it. 🤔

In any case this should be good to go, I asked internally what was the reason of the previous 0x prefix but as test are passing here it should be fine 🙂

@@ -262,12 +267,28 @@ export default class Eth {
offset += chunkSize;
}

rlpTx = ethers.utils.RLP.decode("0x" + rawTxHex);
Copy link
Contributor

Choose a reason for hiding this comment

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

@machard : do you know why we needed to prefix 0x in the past? Also do you know why we had to call it twice. I wonder if we could use a higher level function that would have avoided having to do the logic of that pr on our side. Because ethers must have somewhere the logic to extract the rlp part of a tx?

@gre
Copy link
Contributor

gre commented Aug 1, 2021

I'm going to merge this work as it seems to pass CI. thanks a lot for also adding some tests for this scenario, very useful.
I will issue a ledgerjs version straightaway so we can already anticipate the incoming app.

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