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: remove reliance on transaction decode in confirmations #29341

Merged
merged 11 commits into from
Dec 20, 2024

Conversation

matthewwalsh0
Copy link
Member

@matthewwalsh0 matthewwalsh0 commented Dec 19, 2024

Description

Disable all advanced transaction data decoding using Sourcify, 4Byte and Uniswap, if the Decode smart contracts toggle is disabled.

Remove all reliance on the advanced decoding excluding the Data section.

Specifically:

  • Create useTokenTransactionData hook to decode all token transactions locally using the ABIs.
  • Replace all usages of useDecodedTransactionData with the new hook, except for the TransactionData component.
  • Update related unit and integration tests to decode valid test data rather than rely on mocking hooks.

Open in GitHub Codespaces

Related issues

Manual testing steps

Regression of redesigned transaction confirmations.

Specifically:

  • Token Transfer Recipient
  • Token Transfer Amount
  • Approval Spender
  • Approval Spending Cap

Screenshots/Recordings

Before

After

New Toggle Description

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.

Add useTokenTransactionData hook.
Remove all core usages of decode data.
@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label Dec 19, 2024
@matthewwalsh0 matthewwalsh0 marked this pull request as ready for review December 19, 2024 12:45
@matthewwalsh0 matthewwalsh0 requested a review from a team as a code owner December 19, 2024 12:45
OGPoyraz
OGPoyraz previously approved these changes Dec 19, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [9678571]
Page Load Metrics (1626 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14471899163413063
domContentLoaded14241814160311957
load14471888162612560
domInteractive2186422010
backgroundConnect87123189
firstReactRender16105493216
getState56417199
initialActions01000
loadScripts10531413119811254
setupStore681232813
uiStartup16562399198919795
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -996 Bytes (-0.01%)
  • common: 0 Bytes (0.00%)

@@ -23,6 +26,7 @@ export function useDecodedTransactionData(

return useAsyncResult(async () => {
if (
!isDecodeEnabled ||
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of immediately resolving the privacy regression, is there anything more needed than just the above two lines?

Copy link
Member Author

@matthewwalsh0 matthewwalsh0 Dec 19, 2024

Choose a reason for hiding this comment

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

Afraid so, the redesigned confirmations are incorrectly reliant on the advanced decoding meaning they would lose core functionality if it were disabled. Hence the additional changes to use local decoding.

Copy link
Contributor

Choose a reason for hiding this comment

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

would they lose functionality that was available on the old designs, if decoding was disabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

Afraid so, fundamental data such as token transfer recipient and approve spending cap.

@metamaskbot
Copy link
Collaborator

Builds ready [3644b3e]
Page Load Metrics (1898 ± 76 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15332149189716077
domContentLoaded14842082186415373
load14922151189815976
domInteractive2910443178
backgroundConnect983352512
firstReactRender1674362311
getState764232010
initialActions01000
loadScripts11071617140013766
setupStore76013126
uiStartup17222433216517283
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -996 Bytes (-0.01%)
  • common: -155 Bytes (-0.00%)

jpuri
jpuri previously approved these changes Dec 20, 2024
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

Great work

@metamaskbot
Copy link
Collaborator

Builds ready [3c6dcba]
Page Load Metrics (1578 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14171832158210651
domContentLoaded14061764155610550
load14111787157810249
domInteractive23563094
backgroundConnect75723189
firstReactRender1575282110
getState45612157
initialActions01000
loadScripts9511366114610450
setupStore65612147
uiStartup15692070179511857
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -1014 Bytes (-0.01%)
  • common: -161 Bytes (-0.00%)

@matthewwalsh0 matthewwalsh0 added this pull request to the merge queue Dec 20, 2024
Merged via the queue into main with commit 1c6391c Dec 20, 2024
80 checks passed
@matthewwalsh0 matthewwalsh0 deleted the fix/remove-confirmation-decode-reliance branch December 20, 2024 13:52
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2024
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Dec 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.11.0 Issue or pull request that will be included in release 12.11.0 team-confirmations Push issues to confirmations team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants