-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add metamask_watchAsset #4606
Add metamask_watchAsset #4606
Conversation
Right now this PR is working as follow @cjeria @danfinlay any thoughts on how it should be the cancellation of this suggestion? |
A new component was created to confirm the suggested tokens as you can see on the video. I think that could be a better approach for it. Thoughts? Now we have the possibility to developers that want to use this feature to send a personal message through the method to the user, giving more information about this suggested token (?). |
Okay, three thoughts:
|
Following your comments @danfinlay
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! just a few strings that should be moved to the localization files and a potential safe check.
@@ -247,7 +306,8 @@ class PreferencesController { | |||
} else { | |||
tokens.push(newEntry) | |||
} | |||
this._updateAccountTokens(tokens) | |||
assetImages[address] = image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's really needed but maybe use toChecksumAddress or normalizeAddress to make sure it will always find the right key (assuming that assetImages keys are already checksummed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in line 295 the normalization happens, so for each token added it will have as key the correct normalized address.
|
||
const ownAddress = identitiesList.includes(standardAddress) | ||
if (ownAddress) { | ||
msg = 'Personal address detected. Input the token contract address.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with all these comments about messages.json file, but the original old ui (old-ui/app/add-tokens.js
) all names are in this way and are not used to do translations, so I assumed that this wasn't necessary. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disregard all the previous comments about localization. Didn't realize it was old-ui
sandbox.restore() | ||
}) | ||
|
||
it('should do anything if method not corresponds', async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. There are a few strings that can be internationalized, but nothing show-stopping. Excited to see this feature in the wild.
One question: are we standardizing on the metamask_
custom RPC method prefix? Should we version this as well in case new versions of the proposal arise?
@@ -1,5 +1,6 @@ | |||
const ObservableStore = require('obs-store') | |||
const normalizeAddress = require('eth-sig-util').normalize | |||
const isValidAddress = require('ethereumjs-util').isValidAddress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You can save one level of indirection by doing const { isValidAddress } = require('ethereumjs-util');
Thanks for reviewing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Ping me after resolving conflicts and I'll merge
1c801b5
to
3106374
Compare
Why is there a Character Limit for the symbol? e.g. Tokens with a character length of 8 can be imported into Metamask via the address but not with this method. |
CLA Signature Action: Thank you for your submission, we really appreciate it. We ask that you all read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:
By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository. 1 out of 2 committers have signed the CLA. |
Trying to add the same token twice gives the following error: This warning should not be given if the token was already added to the wallet with the same |
ethereum.request({ |
Adds the method
metamask_watchAsset
, a vendor-prefixed version ofwallet_watchAsset
per EIP 747.To Do:
Sample Usage