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 add custom token #3942

Merged
merged 13 commits into from
Apr 25, 2022
Merged

Fix add custom token #3942

merged 13 commits into from
Apr 25, 2022

Conversation

jpcloureiro
Copy link
Contributor

Description

Fix payload when adding a custom token (wallet_watchAsset rpc call). Also use the token logo if provided.

wallet_watchAsset rpc method spec

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #3722

@jpcloureiro jpcloureiro added needs-qa Any New Features that needs a full manual QA prior to being added to a release. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 23, 2022
@jpcloureiro jpcloureiro requested a review from a team as a code owner March 23, 2022 15:04
@jpcloureiro jpcloureiro changed the title Fix/watch asset rpc call Fix add custom token Mar 23, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 24, 2022

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.

@jpcloureiro
Copy link
Contributor Author

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

use TokenImage component
when rendering token logo
@jpcloureiro jpcloureiro added the type-bug Something isn't working label Mar 24, 2022
@gantunesr gantunesr removed the type-bug Something isn't working label Mar 25, 2022
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

Good work @jpcloureiro! Left some comments

app/components/UI/TokenImage/index.js Outdated Show resolved Hide resolved
app/components/UI/AssetOverview/index.js Show resolved Hide resolved
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM

@gantunesr gantunesr removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 28, 2022
@chrisleewilcox chrisleewilcox self-assigned this Apr 8, 2022
@chrisleewilcox chrisleewilcox added QA in Progress QA has started on the feature. QA'd but questions A QA run through has been done but you need clarification on minor issues you found and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA'd but questions A QA run through has been done but you need clarification on minor issues you found labels Apr 8, 2022
@chrisleewilcox
Copy link
Contributor

chrisleewilcox commented Apr 11, 2022

Getting the following error when running yarn setup...

**ERROR** Failed to apply patch for package @metamask/design-tokens at path
  
    node_modules/@metamask/design-tokens

  This error was caused because @metamask/design-tokens has changed since you
  made the patch file for it. This introduced conflicts with your patch,
  just like a merge conflict in Git when separate incompatible changes are
  made to the same piece of code.

  Maybe this means your patch file is no longer necessary, in which case
  hooray! Just delete it!

  Otherwise, you need to generate a new patch file.

  To generate a new one, just repeat the steps you made to generate the first
  one.

  i.e. manually make the appropriate file changes, then run 

    patch-package @metamask/design-tokens

  Info:
    Patch file: patches/@metamask+design-tokens+1.4.2.patch
    Patch was made for version: 1.4.2
    Installed version: 1.5.1

---
patch-package finished with 1 warning(s), 1 error(s).
4. Create xcconfig files...
5. Init git submodules

image

@wachunei
Copy link
Member

@chrisleewilcox this was fixed here b9d3f4b, updating this branch with main will fix it.

@chrisleewilcox chrisleewilcox added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) QA in Progress QA has started on the feature. and removed QA in Progress QA has started on the feature. labels Apr 11, 2022
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

Here is a recording of the test. Data is shown as expected and token icon is shown.

https://recordit.co/pjMnCIaTr4

LGTM

@chrisleewilcox chrisleewilcox added QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed QA in Progress QA has started on the feature. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Apr 14, 2022
@gantunesr gantunesr merged commit e1e10e8 into main Apr 25, 2022
@gantunesr gantunesr deleted the fix/watch-asset-rpc-call branch April 25, 2022 15:50
@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wallet_watchAsset not working on mobile but works at browser extension
6 participants