fix: ledger vrs values + add support for type 0 and type 2 transactions #27966

seaona · 2024-10-18T15:58:54Z

Description

Note: the jobs will fail in this branch, as we need to run lavamoat:auto, but I leave this for the base branch feat/9544 to do.
In case you want to try it locally you'll need to update the policies locally (see steps below).

Legacy Transactions support:

legacy-ledger.mp4

EIP1559 Transactions support:

eip1559-ledger.mp4

Related issues

Fixes:

Manual testing steps

To run locally do:

yarn lavamoat:auto -- this will update the policies (now this is failing in this PR, but I leave this to be done in the original branch from @Akaryatrh )
yarn start:test
yarn test:e2e:single test/e2e/tests/hardware-wallets/ledger.spec.ts --browser=chrome --leave-running=true

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-10-18T15:59:06Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Gudahtt · 2024-10-21T13:58:52Z

test/stub/keyring-bridge.js

-    return Transaction.fromSerializedTx(txBuffer, {
+    const VALID_TYPES = [1, 2];
+    const txType = VALID_TYPES.includes(rawTx[0]) ? rawTx[0] : null;
+    const rlpData = txType === null ? txBuffer : txBuffer.slice(1, tx.length);


Interesting, so why are we removing the transaction type of supported transactions?

What would happen here with a pre-EIP-1559 transaction, or a non-supported transaction type?

hi @Gudahtt , so here we chose to implement this piece as close as what Ledger does

https://npmfs.com/package/@ledgerhq/hw-app-eth/6.12.2/src/Eth.ts#L206

mmm so with pre-EIP-1559's this piece of logic seems to work, since we are using a legacy tx type 0 in the spec 🤔

EDIT - I guess for the non-supported types, this would eventually fail somewhere? I'll try now manually to see what happens in a real device vs in this setup, and report back

Interesting, thanks for sharing that link. It looks like Ledger uses the type to inform what it uses as its representation of the transaction to sign, here: https://npmfs.com/package/@ledgerhq/hw-app-eth/6.12.2/src/Eth.ts#L222

While it would be nice to just do what Ledger does, it's not entirely clear how to do that here because the linked code doesn't include the full picture. Reading further, it appears that Ledger sends the entire raw transaction to the device, only using the decoded data for fetching contract related metadata to send to the device alongside the transaction. Meaning that what we're doing here isn't really that similar after all.

It looks like Transaction.fromValuesArray will only work for legacy transactions (the Transaction class we're using here is this one: https://github.com/ethereumjs/ethereumjs-monorepo/blob/%40ethereumjs/tx%404.2.0/packages/tx/src/legacyTransaction.ts#L33)

This change looks OK if that's all we want this mock to be used for. But it seems like it might fail for transactions of type 1 or 2. I'd recommend that we throw a more useful error for those types in the interim, or ensure they work correctly as well.

ohh thank you for these insights, and indeed the test is failing with type 2. I think I see what you mean, so possibly we'll need to handle each case differently in our logic 🤔
What would you think on doing: one case for type 0 (the current one that is working) one for type 2, and the rest of cases just handle it as failure? We could leave for the future the case of type 1 with access list? I'm not sure we currently have any test for type 0x1 nor support in the test dapp for testing it, so maybe adding 'testability' around this tx type first, would be better? 🤔

Leaving type 1 for later makes sense to me!

@Gudahtt @seaona can we merge this one and make new PRs for other cases? (this would allow me to clean up the destination branch)
Except if you see some blockers?

I've added support for type 0 and type 2, and 2 specs, one for type 0 and another one for type2, which are now passing (see vids in the description).
Note: the jobs fail in ci, since this branch also needs to run lavamoat auto -> I leave this out of scope, so it can be done in the base branch from @Akaryatrh

test/stub/keyring-bridge.js

seaona · 2024-11-19T09:13:54Z

test/e2e/tests/hardware-wallets/ledger.spec.ts

-          KNOWN_PUBLIC_KEY_ADDRESSES[0].address,
-          convertETHToHexGwei(2),
-        );
-        await driver.delay(2000);


ℹ️ we can remove this delay if we seed the account from the beginning, using the ganache set balance method

seaona · 2024-11-19T09:14:45Z

test/e2e/tests/hardware-wallets/ledger.spec.ts

+        fixtures: new FixtureBuilder().build(),
+        ganacheOptions: {
+          ...defaultGanacheOptions,
+          hardfork: 'london',


ℹ️ this makes our tx an EIP1559 (type 2 tx)

seaona · 2024-11-19T09:15:11Z

test/stub/keyring-bridge.js

+    } else if (txType === 2) {
+      const rlpData = txBuffer.slice(1, tx.length);
+      const rlpTx = rlp.decode(rlpData);
+      return FeeMarketEIP1559Transaction.fromValuesArray(rlpTx, {


for type 2 tx's, we need to use this other tx type (EIP1559)

vrs values

33cb530

seaona requested a review from a team as a code owner October 18, 2024 15:58

Akaryatrh approved these changes Oct 21, 2024

View reviewed changes

Akaryatrh added the team-hardware-wallets label Oct 21, 2024

Gudahtt reviewed Oct 21, 2024

View reviewed changes

test/stub/keyring-bridge.js Outdated Show resolved Hide resolved

seaona added 3 commits November 5, 2024 12:42

add comments and remove old var

74b5cb3

add support for type 2

a16cee7

ledger name

d96042f

seaona commented Nov 19, 2024

View reviewed changes

seaona self-assigned this Nov 19, 2024

seaona added area-qa Relating to QA work (Quality Assurance) team-extension-platform Extension Platform team labels Nov 19, 2024

seaona changed the title ~~fix: ledger vrs values~~ fix: ledger vrs values + add support for type 0 and type 2 transactions Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ledger vrs values + add support for type 0 and type 2 transactions #27966

fix: ledger vrs values + add support for type 0 and type 2 transactions #27966

seaona commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024

Gudahtt Oct 21, 2024

seaona Nov 5, 2024 •

edited

Loading

Gudahtt Nov 5, 2024

Gudahtt Nov 5, 2024

seaona Nov 5, 2024

Gudahtt Nov 5, 2024

Akaryatrh Nov 12, 2024

seaona Nov 19, 2024 •

edited

Loading

seaona Nov 19, 2024

seaona Nov 19, 2024

seaona Nov 19, 2024

fix: ledger vrs values + add support for type 0 and type 2 transactions #27966

Are you sure you want to change the base?

fix: ledger vrs values + add support for type 0 and type 2 transactions #27966

Conversation

seaona commented Oct 18, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 18, 2024

Choose a reason for hiding this comment

seaona Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaona Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaona commented Oct 18, 2024 •

edited

Loading

seaona Nov 5, 2024 •

edited

Loading

seaona Nov 19, 2024 •

edited

Loading