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.

Sign up for GitHub

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

Jump to bottom

feat: add token verification source count and link to block explorer #27759

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from
Open

feat: add token verification source count and link to block explorer #27759

wants to merge 29 commits into from
+70 −19

Conversation

bfullam
Copy link

@bfullam bfullam commented Oct 10, 2024 •
edited
Loading

Description

In a previous redesign, the information about the number of sources a token has been verified on and the link to that token on the relevant block explorer was removed from the swap page. This returns that information.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to the swap page
  2. Select swap assets and amount
  3. See token verification info and block explorer link

Screenshots/Recordings

Before

Screenshot 2024-10-10 at 14 30 01

After

Screenshot 2024-10-10 at 14 30 21

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.

@bfullam
feat: add token verification source count and link to block explorer
6081700 
In a previous redesign, the information about the number of sources a token has been verified on and the link to that token on the relevant block explorer was removed from the swap page. This returns that information.
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 10, 2024 •
edited
Loading

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.

@bfullam
Copy link
Author

bfullam commented Oct 10, 2024

I have read the CLA Document and I hereby sign the CLA

@metamaskbot
Copy link
Collaborator

Builds ready [5b410dc]
Page Load Metrics (2061 ± 261 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29526661801560269
domContentLoaded158140282035536258
load159440532061543261
domInteractive26112512311
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 159 Bytes (0.00%)
  • common: 1 Bytes (0.00%)

@bfullam bfullam marked this pull request as ready for review October 10, 2024 13:23
@bfullam bfullam requested review from a team as code owners October 10, 2024 13:23
@vstern1
Copy link

vstern1 commented Oct 10, 2024

Looks good. Could we update the copy?

Verified on x sources. You can check on [block explorer].

Can we specify which block explorer? I think we did on Extension before it disappeared.
@coreyjanssen does this look ok?

@metamaskbot
Copy link
Collaborator

Builds ready [8523b5b]
Page Load Metrics (1844 ± 109 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint154323341839229110
domContentLoaded153523231817224108
load154423331844226109
domInteractive2196502311
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 159 Bytes (0.00%)
  • common: 1 Bytes (0.00%)

@jclancy93
Copy link
Contributor

jclancy93 commented Oct 10, 2024 •
edited
Loading

I noticed something similar to @vstern1

image

In the designs we are using human readable names (Etherscan, PolygonScan, LineaScan, etc.) but in the current implementation we are using human readable URLs instead (etherscan.io, lineascan.build, etc.). Hopefully we have a way to get human readable block explorer names instead

Update: looks like there's not directly in the BlockExplorer component as we have defined. But we could consider including a new enum that maps the chain id to block explorer readable name

ejwessel
ejwessel previously approved these changes Oct 10, 2024
View reviewed changes
@martahj
Copy link
Contributor

martahj commented Oct 10, 2024

We probably shouldn't show the message for the native token, as this makes a highly trustable token (ETH) look unreliable.
Screenshot 2024-10-10 at 10 50 54 AM

@coreyjanssen
Copy link
Contributor

Looks good to me. Approved by content design.

@bfullam
fix: update message copy
c7d20bb 
Create new message to handle updated copy for swap token verified sources.
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 16, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
66.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@metamaskbot
Copy link
Collaborator

Builds ready [5b010b0]
Page Load Metrics (2038 ± 147 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27328371935482231
domContentLoaded165327741972268129
load166928332038307147
domInteractive32249724521
backgroundConnect8319598642
firstReactRender462871135225
getState5389478441
initialActions00000
loadScripts120822561486242116
setupStore11188504622
uiStartup186333772352429206
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 254 Bytes (0.00%)
  • common: 736 Bytes (0.01%)

@metamaskbot
Copy link
Collaborator

Builds ready [8d00e80]
Page Load Metrics (1808 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16482391180316881
domContentLoaded16272227176513967
load16472414180817685
domInteractive16204504019
backgroundConnect9187444522
firstReactRender472041044120
getState4111162613
initialActions01000
loadScripts11821680132712057
setupStore11145433517
uiStartup182531472084329158
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 254 Bytes (0.00%)
  • common: 736 Bytes (0.01%)

@metamaskbot
Copy link
Collaborator

Builds ready [e1fdd68]
Page Load Metrics (1775 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22622601700365175
domContentLoaded16032084174111555
load16112160177512560
domInteractive198246199
backgroundConnect787332914
firstReactRender502921125527
getState45810116
initialActions01000
loadScripts1136152612789445
setupStore1195312713
uiStartup182127572009218104
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 254 Bytes (0.00%)
  • common: 736 Bytes (0.01%)

martahj
martahj previously approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@martahj martahj left a comment

Choose a reason for hiding this comment

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

LGTM!

infiniteflower
infiniteflower reviewed Oct 17, 2024
View reviewed changes
Copy link
Contributor

@infiniteflower infiniteflower left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I notice the sources message only appears after we've fetched a quote. Is that the intended UX? Portfolio shows it even before you enter in any amounts.

@bfullam
Copy link
Author

bfullam commented Oct 17, 2024

Ticket description said to display after fetching a quote, but now that you point it out, that does feel a little off.

@vstern1, did you want it to wait until after fetching for a specific reason?

@metamaskbot
Copy link
Collaborator

Builds ready [74ad24c]
Page Load Metrics (1643 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30219091589315151
domContentLoaded1504185416279445
load15131899164310450
domInteractive19190524321
backgroundConnect75018136
firstReactRender42209894421
getState470222512
initialActions00000
loadScripts1074139612037837
setupStore1093202110
uiStartup16572409184216278
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 262 Bytes (0.00%)
  • common: 736 Bytes (0.01%)

@metamaskbot
Copy link
Collaborator

Builds ready [dd670b9]
Page Load Metrics (1939 ± 171 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint45826941859434209
domContentLoaded156426381852251120
load156830741939357171
domInteractive2788482010
backgroundConnect67948416780
firstReactRender482041034823
getState4122273014
initialActions00000
loadScripts114722181389237114
setupStore1196292512
uiStartup175632862182379182
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 427 Bytes (0.01%)
  • common: 730 Bytes (0.01%)

infiniteflower
infiniteflower reviewed Oct 21, 2024
View reviewed changes
Copy link
Contributor

@infiniteflower infiniteflower left a comment

Choose a reason for hiding this comment

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

We probably shouldn't show anything if no To token has been selected

Screenshot 2024-10-21 at 1 54 43 PM

@bfullam
Copy link
Author

bfullam commented Oct 22, 2024

Absolutely right, good catch @infiniteflower! Fix made 👌

@metamaskbot
Copy link
Collaborator

Builds ready [8f8d708]
Page Load Metrics (1865 ± 109 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint156224721867229110
domContentLoaded15522315181318187
load156125151865227109
domInteractive25266595124
backgroundConnect9196505527
firstReactRender521991103818
getState4128313718
initialActions01000
loadScripts11321737134414469
setupStore108322199
uiStartup171030772139368177
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 451 Bytes (0.01%)
  • common: 730 Bytes (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martahj martahj martahj left review comments

@infiniteflower infiniteflower infiniteflower left review comments

@ejwessel ejwessel ejwessel left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
team-swaps
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@bfullam @metamaskbot @vstern1 @jclancy93 @martahj @coreyjanssen @ejwessel @infiniteflower