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: check UTL verifiedness of tokens before showing spoof warning banner #288

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

steveluscher
Copy link
Collaborator

@steveluscher steveluscher commented Aug 24, 2023

fix: check UTL verifiedness of tokens before showing spoof warning banner

Summary

The UTL provides a verfied property. Let's not show a token name/icon spoof warning banner unless that property is false.

Test Plan

image

http://localhost:3000/address/2VhjJ9WxaGC3EZFwJG9BDUs9KxKCAjQY4vgd1qxgYWVg/transfers

image

http://localhost:3000/address/DezXAZ8z7PnrnRJjz3wXBoRgixCa6xjnB7YaB1pPB263

image

http://localhost:3000/address/FASTAr1in6u53vykPbCuFTUZnGdiLJrXaTEQ7UAArdBm

Fixes: #229.

@vercel
Copy link

vercel bot commented Aug 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 24, 2023 10:18pm

return legacyCdnTokenInfo
? {
...legacyCdnTokenInfo,
verified: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mcintyre94, I'm presuming that everything that comes from the legacy token list CDN can be presumed to be ‘verified.’ Does that make sense?

tags
}
tags,
verified: !!(legacyCdnTokenInfo || sdkTokenInfo.verified),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This means ‘if you're on the legacy token list, or the UTL registry said you're verified, you're verified.’ Is that sound, @mcintyre94?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I missed this PR before! I think this is reasonable, but if the UTL registry contains the same token as unverified maybe that should take precedence?

So:

  • Not on the legacy token list at all, UTL verified determines it
  • On legacy token list and UTL, UTL verified determines it
  • On legacy token list and not UTL, verified

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, if I understand correctly, that basically implies that we should do this?

Suggested change
verified: !!(legacyCdnTokenInfo || sdkTokenInfo.verified),
verified: sdkTokenInfo.verified ?? false,
  1. By this line, we know that sdkTokenInfo is non-null.
  2. The ‘not on UTL but on legacy list’ case is handled above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Building a Vercel preview for that now: #289.

…nner

# Summary

The UTL provides a `verfied` property. Let's not show a token name/icon spoof warning banner unless that property is `false`.


# Test Plan
@steveluscher steveluscher added the automerge Merge this Pull Request automatically once CI passes label Aug 24, 2023
@steveluscher steveluscher merged commit 4b26de2 into master Aug 24, 2023
3 checks passed
@steveluscher steveluscher deleted the pr288 branch August 24, 2023 22:20
steveluscher added a commit that referenced this pull request Sep 14, 2023
This is a follow up on this comment: #288 (comment)
steveluscher added a commit that referenced this pull request Sep 14, 2023
This is a follow up on this comment:
#288 (comment)

Reran the test plan from #288.

Related to #229.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Warning message shown for a Token which misleads the audience
3 participants