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

feat: display native values returned from decoding api #28374

Merged
merged 43 commits into from
Nov 22, 2024

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Nov 8, 2024

Description

display native values returned from decoding api

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3617

Manual testing steps

  1. Enable signature decoding api
  2. On test dapp submit permit request
  3. Check simulation section

Screenshots/Recordings

Screenshot 2024-11-08 at 10 21 09 AM

For NFT collection:
Screenshot 2024-11-11 at 6 33 54 PM

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.

@jpuri jpuri requested a review from a team as a code owner November 8, 2024 05:13
Copy link
Contributor

github-actions bot commented Nov 8, 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 Nov 8, 2024
@jpuri jpuri requested a review from a team as a code owner November 8, 2024 09:53
@metamaskbot
Copy link
Collaborator

Builds ready [8de0e98]
Page Load Metrics (1928 ± 95 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint46521881770459220
domContentLoaded16952432189719192
load17032461192819795
domInteractive29105552311
backgroundConnect8119372914
firstReactRender521801103115
getState45319178
initialActions00000
loadScripts11921966141518488
setupStore56312157
uiStartup190526712145208100
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -103.91 KiB (-2.28%)
  • ui: 12.2 KiB (0.16%)
  • common: 107.1 KiB (1.30%)

@metamaskbot
Copy link
Collaborator

Builds ready [81503b3]
Page Load Metrics (2800 ± 213 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39733812272975468
domContentLoaded207036302762446214
load210336462800444213
domInteractive32135783417
backgroundConnect9142503316
firstReactRender784001408842
getState592991194823
initialActions01000
loadScripts154828922098369177
setupStore6461394
uiStartup242742193276544261
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 5.24 KiB (0.07%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

minor suggestion to remove the snapshot test. I think the snapshot tests have diminishing returns. With a minor change, there is a lot of change in this PR. I suggest we avoid snapshot tests for smaller, reusable components as they will be tested together in parent snapshots

);

expect(await findByText('<0.000001')).toBeInTheDocument();
expect(container).toMatchSnapshot();
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing how many snapshots were affected in this PR, I favor avoiding snapshots on smaller, reusable components. We can observe nuanced style changes from the parent component(s) that render these smaller components and test specific attributes such as text, behavior, or direct styles.

Suggested change
expect(container).toMatchSnapshot();

jpuri and others added 3 commits November 21, 2024 14:55
…it-simulation/native-value-display/native-value-display.tsx

Co-authored-by: digiwand <20778143+digiwand@users.noreply.github.com>
…it-simulation/value-display/value-display.tsx

Co-authored-by: digiwand <20778143+digiwand@users.noreply.github.com>
@jpuri jpuri requested a review from digiwand November 21, 2024 09:29
@metamaskbot
Copy link
Collaborator

Builds ready [455f4b0]
Page Load Metrics (2017 ± 74 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint45123651859492236
domContentLoaded17772311198214369
load17862365201715574
domInteractive18108532311
backgroundConnect9109362613
firstReactRender911731292412
getState55917168
initialActions01000
loadScripts12951758147913063
setupStore66014157
uiStartup20052618225717986
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 5.25 KiB (0.07%)
  • common: 0 Bytes (0.00%)

@jpuri jpuri added this pull request to the merge queue Nov 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 21, 2024
@jpuri jpuri added this pull request to the merge queue Nov 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 21, 2024
@jpuri jpuri enabled auto-merge November 21, 2024 15:10
@metamaskbot
Copy link
Collaborator

Builds ready [6a1b4b4]
Page Load Metrics (2030 ± 113 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29626381950450216
domContentLoaded161225561978216104
load162226502030236113
domInteractive17211504019
backgroundConnect8106532914
firstReactRender6939921211756
getState591283014
initialActions01000
loadScripts11351982144519493
setupStore686232613
uiStartup182630062432355171
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 5.25 KiB (0.07%)
  • common: 0 Bytes (0.00%)

@jpuri jpuri added this pull request to the merge queue Nov 22, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [266a002]
Page Load Metrics (1953 ± 62 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17892342195612761
domContentLoaded17302249190811857
load17902358195312962
domInteractive248137147
backgroundConnect14106452914
firstReactRender641411032211
getState66117157
initialActions01000
loadScripts12411667138510048
setupStore75719178
uiStartup20032668216114871
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 5.25 KiB (0.07%)
  • common: 0 Bytes (0.00%)

Merged via the queue into develop with commit 095e2eb Nov 22, 2024
75 checks passed
@jpuri jpuri deleted the native_value_display branch November 22, 2024 00:50
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2024
@metamaskbot metamaskbot added the release-12.9.0 Issue or pull request that will be included in release 12.9.0 label Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.9.0 Issue or pull request that will be included in release 12.9.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants