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: Do not break application if no token details are found using getTokenStandardAndDetails #26324

Merged

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Aug 2, 2024

Description

Upon further observation and while work on test cases with varying mock data, I found code improvements and, I believe, fixes related to the Permit code.

  1. If getTokenStandardAndDetails does not return token details, the app should not break. The PR handles this in 2 locations of the application.
  2. We should serialize the behavior in useEffect to run set states so that it does not run async

Open in GitHub Codespaces

Related issues

Related to: #25410
Related to: #26107

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

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 requested a review from a team as a code owner August 2, 2024 15:55
Copy link
Contributor

github-actions bot commented Aug 2, 2024

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 metamaskbot added the team-confirmations Push issues to confirmations team label Aug 2, 2024
@digiwand digiwand changed the title Fix: Do not break application if no token details are found using getTokenStandardAndDetails fix: Do not break application if no token details are found using getTokenStandardAndDetails Aug 2, 2024
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

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

Project coverage is 69.97%. Comparing base (bef5b8f) to head (2e94356).

Files Patch % Lines
.../confirmations/components/confirm/row/dataTree.tsx 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #26324   +/-   ##
========================================
  Coverage    69.97%   69.97%           
========================================
  Files         1422     1422           
  Lines        49938    49937    -1     
  Branches     13861    13861           
========================================
  Hits         34943    34943           
+ Misses       14995    14994    -1     

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

});
};

getDecimals();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since PR is refactoring this useeffect, it may be useful to use this hook I think to avoid race conditions:

https://github.com/MetaMask/metamask-extension/blob/develop/ui/hooks/useAsyncResult.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

conversation continued below #26324 (comment)

@metamaskbot
Copy link
Collaborator

Builds ready [edf1542]
Page Load Metrics (524 ± 366 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint653441407737
domContentLoaded10181394522
load422052524762366
domInteractive10181394522
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 67 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

matthewwalsh0
matthewwalsh0 previously approved these changes Aug 2, 2024
ui/pages/confirmations/components/confirm/row/dataTree.tsx Outdated Show resolved Hide resolved
@digiwand digiwand requested a review from jpuri August 2, 2024 17:03
@metamaskbot
Copy link
Collaborator

Builds ready [40e1cd1]
Page Load Metrics (371 ± 299 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint593311156933
domContentLoaded96623168
load381721371622299
domInteractive86623168
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 185 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

matthewwalsh0
matthewwalsh0 previously approved these changes Aug 12, 2024
@@ -11,6 +11,7 @@ import { getTokenStandardAndDetails } from '../../../../../store/actions';

import { Box } from '../../../../../components/component-library';
import { BlockSize } from '../../../../../helpers/constants/design-system';
import * as hookModule from '../../../../../hooks/useAsyncResult';
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but why the as import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Missed this when I was copying over the import from another test file. Updated d4e60ac

vinistevam
vinistevam previously approved these changes Aug 13, 2024
@digiwand digiwand dismissed stale reviews from vinistevam and matthewwalsh0 via 8a411d0 August 13, 2024 13:12
@digiwand
Copy link
Contributor Author

Thanks for the reviews! Made an update and reverted an unnecessary snapshot update. May I get re-approvals here please? @matthewwalsh0 @vinistevam @jpuri

Copy link

sonarcloud bot commented Aug 14, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [2e94356]
Page Load Metrics (317 ± 306 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint793141325929
domContentLoaded11191364120
load562290317637306
domInteractive11191364120
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 172 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@digiwand digiwand merged commit 03a6569 into develop Aug 14, 2024
78 checks passed
@digiwand digiwand deleted the fix-permit-dataTree-should-not-break-with-no-decimals branch August 14, 2024 16:16
@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2024
@metamaskbot metamaskbot added the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 14, 2024
@gauthierpetetin gauthierpetetin added release-12.3.0 Issue or pull request that will be included in release 12.3.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.3.0 Issue or pull request that will be included in release 12.3.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants