-
Notifications
You must be signed in to change notification settings - Fork 379
Conversation
Thanks @pscott for help with the APDUs 😄 |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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); |
There was a problem hiding this comment.
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?
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. |
Small changes required to support the new transaction formats for the new transaction types.
To be merged with changes here: LedgerHQ/app-ethereum#157