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

Update name of tokens on token list #1127

Merged
merged 20 commits into from
Jul 7, 2023

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Mar 14, 2023

PR Title

  • A brief description of changes. If the PR has breaking changes add BREAKING:
    to the start of the PR title.

Description
This PR aims to update the token name of the tokens on the token list.

  • BREAKING:
    It was added the name property on the follow function:
  • addToken - now it receives an object with optional params (name, image)
  • CHANING:
  • addTokens

It was created a new function called
updateTokensAttribute - It aims to update the tokens attribute to the tokens that are on the state with the fetched token list.

It was created a new ERC20 standard to get the token name
getTokenName
and the function to call it on Assets Controller
getERC20TokenName

Checklist

  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #???

@tommasini tommasini requested a review from a team as a code owner March 14, 2023 12:30
@tommasini tommasini changed the title update tokens with token name Update name of tokens on token list Mar 14, 2023
@tommasini
Copy link
Contributor Author

Thanks for the review @mcmire I will address your comments later today

@tommasini
Copy link
Contributor Author

The next step on this PR is to implement the responsibility of the update of the tokens name on the TokensController instead of the TokenDetectionController

More context on this thread

@mcmire
Copy link
Contributor

mcmire commented Apr 18, 2023

@tommasini Cool, just post another comment when you're ready for review again!

@tommasini
Copy link
Contributor Author

@mcmire @Gudahtt sorry for taking so long on this PR, when you are able please can you take a look on it?

@tommasini tommasini requested review from mcmire and Gudahtt June 29, 2023 17:49
Copy link
Contributor

@bergarces bergarces left a comment

Choose a reason for hiding this comment

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

No red flags from me. It should not negatively impact wallet_watchAsset.

legobeat
legobeat previously approved these changes Jul 4, 2023
Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
@tommasini tommasini merged commit 11f2657 into MetaMask:main Jul 7, 2023
@tommasini tommasini deleted the feature/update-name-token-list branch July 7, 2023 14:33
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants