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

fix: Cannot destructure property 'balance' of 'K[Z]' as it is undefined fromAddress error when no transaction is loaded in ConfirmTransactionBase #25506

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Jun 25, 2024

Description

In confirm-transaction-base.container.js, while the transaction is switching, the component may be in a state without transaction data. In this case, we can handle the empty transaction data appropriately until the expected data comes in to allow the user flow to continue without breaking.

note: I was unable to reproduce this in develop after some time, but I successfully repro'd this in Version-v12.0.0 which allowed me to verify the fix works. Possibly the underlying empty transaction data state was fixed by other code in develop

This PR is planned to be cherry-picked for Version-v12.0.0

Related issues

Fixes: #25406

Manual testing steps

See Issue for repro steps

Screenshots/Recordings

Possible, occasional state in confirm-transaction-base.container.js

CleanShot 2024-06-25 at 15 58 13@2x

Before

See error in Issue screenshots

After

No error message is shown and app works as expected

Pre-merge author checklist

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.

@digiwand digiwand added the team-confirmations Push issues to confirmations team label Jun 25, 2024
@digiwand digiwand requested a review from a team as a code owner June 25, 2024 14:04
Copy link
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.

jpuri
jpuri previously approved these changes Jun 25, 2024
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.68%. Comparing base (d403213) to head (e422925).
Report is 2 commits behind head on develop.

Files Patch % Lines
...saction-base/confirm-transaction-base.container.js 66.67% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25506      +/-   ##
===========================================
- Coverage    69.69%   69.68%   -0.00%     
===========================================
  Files         1350     1350              
  Lines        47865    47867       +2     
  Branches     13199    13201       +2     
===========================================
+ Hits         33355    33356       +1     
- Misses       14510    14511       +1     

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

@metamaskbot
Copy link
Collaborator

Builds ready [2667021]
Page Load Metrics (135 ± 167 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6413290178
domContentLoaded9341463
load411648135347167
domInteractive9341463
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 4 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@sleepytanya
Copy link
Contributor

@digiwand
MetaMask now doesn't break with an error but the subsequent transaction is created on the same chain as the previous one.
First scenario from the bug ticket:

  • Connect to the test dapp with Sepolia
  • Connect to dapp with Ethereum
  • Trigger a swap in dapp
  • Without accepting/rejecting, go to the test dapp and trigger a signature - signature request from the test dapp is created on mainnet
swapSign.mov

Error also doesn't appear for this scenario, Connect request is lost, instead of blue numbers there is '...' on the MM icon.

  • Connect to test-dapp
  • Click button to start legacy send transaction; popup comes up, I see a "1" in the extension icon, I ignore the popup window
  • Open new tab, go to Uniswap
  • Click "Connect" button, click MetaMask; I see a "2" in the extension icon, no popup occurs
  • Click "Connect" button again, click MetaMask; I see a "3" in the extension icon, no popup occurs
  • After the first request is rejected and the subsequent request can be accessed MM can break with an error
Screen.Recording.2024-06-25.at.16.26.35.mov

Error also doesn't happen for these steps:

  • Start send+swap txs with low gas
  • Send another (approve) tx from a dapp
  • Confirmation is stuck on the spinner
  • Activity tab has two txs - Pending and Unapproved
  • Unapprovedtxs can't be rejected
  • If user rejects first tx MM displays an error Cannot destructure property 'balance' of 'Q[z]' as it is undefined.

@digiwand
Copy link
Contributor Author

Great catch @sleepytanya! Q: were you able to repro the bug on the develop branch? I was unable to do this so I wonder if it was fixed by develop and not added to v12.0.0

@digiwand
Copy link
Contributor Author

@sleepytanya I have repro'd the issue on develop. We will need to further investigate a fix for this

@sleepytanya
Copy link
Contributor

@digiwand @darkwing @bschorchit
This issue is now resolved across all scenarios we have identified. I will create a separate ticket for the issue with the signature generated on the wrong network.

@digiwand digiwand added the DO-NOT-MERGE Pull requests that should not be merged label Jun 26, 2024
@digiwand digiwand removed the DO-NOT-MERGE Pull requests that should not be merged label Jun 26, 2024
@digiwand digiwand requested a review from davidmurdoch June 27, 2024 12:42
@digiwand digiwand requested review from jpuri and darkwing June 27, 2024 13:15
@metamaskbot
Copy link
Collaborator

Builds ready [99d49c8]
Page Load Metrics (279 ± 316 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint652161054019
domContentLoaded94916105
load392526279659316
domInteractive94916105
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 298 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [e422925]
Page Load Metrics (52 ± 4 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6010686126
domContentLoaded9271242
load40695294
domInteractive9271242
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 298 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@digiwand digiwand merged commit a32cbb8 into develop Jun 27, 2024
70 checks passed
@digiwand digiwand deleted the fix-tx-missing-balance-fromAddress branch June 27, 2024 15:58
@github-actions github-actions bot locked and limited conversation to collaborators Jun 27, 2024
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.1.0 Issue or pull request that will be included in release 12.1.0 team-confirmations Push issues to confirmations team
Projects
Archived in project
7 participants