Skip to content

fix: breaking change from @metamask/transaction-controller regarding Ledger transactions#8932

Merged
gantunesr merged 14 commits into
mainfrom
fix/ledger-transactions-issue-after-txController-upgrade
Mar 19, 2024
Merged

fix: breaking change from @metamask/transaction-controller regarding Ledger transactions#8932
gantunesr merged 14 commits into
mainfrom
fix/ledger-transactions-issue-after-txController-upgrade

Conversation

@dawnseeker8

@dawnseeker8 dawnseeker8 commented Mar 14, 2024

Copy link
Copy Markdown
Contributor

Description

After the TransactionController upgrade to 8.0.1, it use `@ethereumjs/tx: 4.2.0 version, this library have break change on tx.common class which drop function chainIdBN(), and the old version @ethereumjs/tx: 3.5.0 used in @consensys/ledgerhq-metamask-kering still use the old version of tx.common and deprecated chainIdBN() function in BaseTransaction.ts.

So to fix the issue in main branch, we need to upgrade @consensys/ledgerhq-metamask-kering to use @ethereumjs/tx: 4.2.0. and then upgrade main branch to use latest @consensys/ledgerhq-metamask-kering.

Related issues

Fixes: #8917

Manual testing steps

send funds screen ( please make sure PR #8930 merged to main branch first)

  1. Connect Ledger device
  2. Switch network to Linea Testnet
  3. Send transaction from Ledger account to Account 2 (normal account)
  4. Confirm transaction on Ledger device
  5. Transaction should successfully submitted.

test-dapp send coin transacation

  1. Connect Ledger device
  2. Switch network to Linea Testnet
  3. open test-dapp https://metamask.github.io/test-dapp
  4. connect the dapp with your ledger wallet.
  5. click Send EIP 1559 Transaction.
  6. Confirm in ledger
  7. your transaction should be submitted successfully and complete successfully.

Screenshots/Recordings

Before

8917_iPhone.MP4

After

Screen_Recording_20240314_133247_MetaMask.mp4

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • 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.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@dawnseeker8 dawnseeker8 requested a review from a team as a code owner March 14, 2024 09:26
@github-actions

github-actions Bot commented Mar 14, 2024

Copy link
Copy Markdown
Contributor

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.

@dawnseeker8 dawnseeker8 added team-hardware-wallets-deprecated DEPRECATED: please use "team-accounts-framework" instead Run Smoke E2E labels Mar 14, 2024
@dawnseeker8 dawnseeker8 self-assigned this Mar 14, 2024
@dawnseeker8 dawnseeker8 added Ledger Ledger hardware wallet related issue or development needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing labels Mar 14, 2024
@github-actions

github-actions Bot commented Mar 14, 2024 •
edited by metamaskbot
Loading

Copy link
Copy Markdown
Contributor

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: bddb4b2
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ae55294b-24d2-4b99-abd0-8a92bfe0bcce

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@socket-security

socket-security Bot commented Mar 14, 2024 •
edited
Loading

Copy link
Copy Markdown

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@dawnseeker8
Merge branch 'main' into fix/ledger-transactions-issue-after-txContro…
a83cafa 
…ller-upgrade
@dawnseeker8
feat: upgrade ledgerhq-metamask-keyring library to latest which have …
7de59f7 
…a lot of internal library upgrade

This upgrade will see the impact to socket-security any improvement about the list
@dawnseeker8
Merge branch 'main' into fix/ledger-transactions-issue-after-txContro…
468bd1d 
…ller-upgrade
@codecov-commenter

codecov-commenter commented Mar 15, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 43.74%. Comparing base (4fb3968) to head (7b3ed60).

Files Patch % Lines
...ents/Views/confirmations/SendFlow/Confirm/index.js 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #8932   +/-   ##
=======================================
  Coverage   43.74%   43.74%           
=======================================
  Files        1273     1273           
  Lines       31103    31103           
  Branches     3161     3161           
=======================================
  Hits        13606    13606           
  Misses      16684    16684           
  Partials      813      813

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gantunesr gantunesr changed the title fix: Fix the ledger transaction all broken after TransactionController upgrade. fix: breaking change from @metamask/transaction-controller regarding Ledger transactions Mar 15, 2024
@dawnseeker8 dawnseeker8 marked this pull request as draft March 16, 2024 00:50
@sonarqubecloud

sonarqubecloud Bot commented Mar 18, 2024

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@dawnseeker8 dawnseeker8 marked this pull request as ready for review March 18, 2024 09:35
@github-actions

github-actions Bot commented Mar 18, 2024 •
edited by metamaskbot
Loading

Copy link
Copy Markdown
Contributor

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 7b3ed60
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8ee5b9e2-e456-4ebb-ba9c-35464e2e62ec

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@dawnseeker8

dawnseeker8 commented Mar 18, 2024

Copy link
Copy Markdown
Contributor Author

Smoke E2E tests passd in this link https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ae55294b-24d2-4b99-abd0-8a92bfe0bcce

dawnseeker8
dawnseeker8 commented Mar 18, 2024
View reviewed changes
Comment thread app/components/Views/confirmations/SendFlow/Confirm/index.js
@gantunesr

gantunesr commented Mar 19, 2024

Copy link
Copy Markdown
Member

Lets have this QAed before merging @dawnseeker8 and @vivek-consensys

@angelcheung22 angelcheung22 mentioned this pull request Mar 19, 2024
Merged
13 tasks
@angelcheung22

angelcheung22 commented Mar 19, 2024

Copy link
Copy Markdown

@vivek-consensys please help to QA this one.

@vivek-consensys

vivek-consensys commented Mar 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Tested on iPhone 13 on iOS 17.3.1 and Samsung S21 on Android 13 using Bitrise build:- https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/f2ffaaef-ee08-49db-822d-e29e96c3640b

Swap Video

Send Txn Video

@vivek-consensys vivek-consensys added QA in Progress QA has started on the feature. QA Passed QA testing has been completed and passed and removed QA in Progress QA has started on the feature. labels Mar 19, 2024
@gantunesr gantunesr merged commit 1748856 into main Mar 19, 2024
@gantunesr gantunesr deleted the fix/ledger-transactions-issue-after-txController-upgrade branch March 19, 2024 12:37
@github-actions github-actions Bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 19, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 19, 2024
@metamaskbot metamaskbot added the release-7.20.0 Issue or pull request that will be included in release 7.20.0 label Mar 19, 2024
@sethkfman sethkfman added release-7.19.0 Issue or pull request that will be included in release 7.19.0 and removed release-7.20.0 Issue or pull request that will be included in release 7.20.0 labels Mar 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@gantunesr gantunesr gantunesr approved these changes

Assignees

@dawnseeker8 dawnseeker8

Labels

Ledger Ledger hardware wallet related issue or development QA Passed QA testing has been completed and passed release-7.19.0 Issue or pull request that will be included in release 7.19.0 Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing team-hardware-wallets-deprecated DEPRECATED: please use "team-accounts-framework" instead

Projects

PR review queue
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Unable to send transaction using Ledger device

7 participants

@dawnseeker8 @codecov-commenter @gantunesr @angelcheung22 @vivek-consensys @sethkfman @metamaskbot