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(24732): fix error when sending txn from dapp and editing the gas #24810

Merged
merged 1 commit into from
May 29, 2024

Conversation

DDDDDanica
Copy link
Contributor

@DDDDDanica DDDDDanica commented May 28, 2024

Description

There is a racing condition captured in MV3 build. When sending transaction from dapp directly, if user chooses to edit the gas fee, and when suggestedGasFees request is processing slow, gasFeeEstimates could be undefined within hook useCustomTimeEstimate.js. The fix is to add a condition for accessing this object before forwarding to UI.

Open in GitHub Codespaces

Related issues

Fixes: #24732

Manual testing steps

  1. Build in MV3
  2. Trigger a txn from dapp
  3. Open network panel, and trigger edit gas button before suggestedGasFees is finished fetching
  4. You should see the network edition panel

Screenshots/Recordings

Before

by @seaona

network-api-reqs.mp4

After

Pre-merge author checklist

mv3-gas-edition.mov
  • I’ve followed MetaMask 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.

@DDDDDanica DDDDDanica self-assigned this May 28, 2024
@DDDDDanica DDDDDanica requested review from a team as code owners May 28, 2024 08:28
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.

@metamaskbot
Copy link
Collaborator

Builds ready [a4da8b3]
Page Load Metrics (295 ± 322 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6611689157
domContentLoaded9341253
load542420295670322
domInteractive9341253
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 24 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@DDDDDanica DDDDDanica changed the title Draft: fix(24732): fix error when sending txn from dapp and editing the gas fix(24732): fix error when sending txn from dapp and editing the gas May 28, 2024
waitTimeEstimate = high.minWaitTimeEstimate;
} else {
waitTimeEstimate = low.maxWaitTimeEstimate;
waitTimeEstimate = gasFeeEstimates.high?.minWaitTimeEstimate;
Copy link
Contributor

Choose a reason for hiding this comment

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

does this and the next condition not need gasFeeEstimates && as well? Alternatively, perhaps gasFeeEstimates?. could work on all of them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good suggestion ! added optional chaining in the equation

@metamaskbot
Copy link
Collaborator

Builds ready [2eb1e66]
Page Load Metrics (695 ± 515 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint632411063818
domContentLoaded85517136
load5230156951073515
domInteractive85517136
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 177 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented May 28, 2024

Codecov Report

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

Project coverage is 65.67%. Comparing base (7eb2f2b) to head (81e245a).
Report is 11 commits behind head on develop.

Files Patch % Lines
...fee-popover/edit-gas-item/useCustomTimeEstimate.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #24810      +/-   ##
===========================================
- Coverage    65.67%   65.67%   -0.00%     
===========================================
  Files         1360     1360              
  Lines        54105    54104       -1     
  Branches     14066    14065       -1     
===========================================
- Hits         35533    35532       -1     
  Misses       18572    18572              

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

@metamaskbot
Copy link
Collaborator

Builds ready [81e245a]
Page Load Metrics (414 ± 389 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6212388178
domContentLoaded94714115
load522393414811389
domInteractive94714115
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 137 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@DDDDDanica DDDDDanica merged commit 0df1d57 into develop May 29, 2024
71 of 72 checks passed
@DDDDDanica DDDDDanica deleted the fix/24732 branch May 29, 2024 09:55
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2024
@metamaskbot metamaskbot added release-11.18.0 release-11.16.6 Issue or pull request that will be included in release 11.16.6 and removed release-11.18.0 labels May 29, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-11.16.6 on PR. Adding release label release-11.16.6 on PR and removing other release labels(release-11.18.0), as PR was cherry-picked in branch 11.16.6.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.16.6 Issue or pull request that will be included in release 11.16.6 team-extension-platform
Projects
Archived in project
4 participants