MIP-1 proposed watch asset changes #138
Conversation
adonesky1
force-pushed
the
mip-1-proposed-watchAsset-changes
branch
from
3cce57d to
567a429
Compare
|
Looks great! We will merge this once we have all the ducks in a row (mip + extension + mobile pr). For now, could you add |
|
Are there any errors we should document in the spec? (possibly including ones that were missed in the original wallet_watchAsset as well) |
|
@BelfordZ how can we indicate that the stability index for this method is: Experimental (1) |
| "code": -32602, | ||
| "message": "Must specify address, symbol, and decimals." | ||
| }, | ||
| { | ||
| "code": -32602, | ||
| "message": "Invalid symbol: not a string." | ||
| }, | ||
| { | ||
| "code": -32602, | ||
| "message": "Invalid symbol '${symbol}': longer than 11 characters." | ||
| }, | ||
| { | ||
| "code": -32602, | ||
| "message": "Invalid decimals '${decimals}': must be 0 <= 36." | ||
| }, | ||
| { | ||
| "code": -32602, | ||
| "message": "Invalid address '${address}'." | ||
| }, |
There was a problem hiding this comment.
There's no way to link errors with the param types, other than just mentioning it in the description
There was a problem hiding this comment.
@shanejonas Do you think it makes sense to describe this in the root description field?
There was a problem hiding this comment.
ping here @shanejonas ?
There was a problem hiding this comment.
I think its fine to have diff errors from diff params here, like decimals only applies to erc20 but we don't explicitly say that anywhere in the error.
There was a problem hiding this comment.
but we don't explicitly say that anywhere in the error.
I can't tell, are you suggesting that we should say it explicitly in the error, or that its okay that we don't say it explicitly?
|
No comments from me. Looks good! |
adonesky1
force-pushed
the
mip-1-proposed-watchAsset-changes
branch
from
dc3dc3f to
6f76560
Compare
openrpc.json
Outdated
| "ERC20" | ||
| ] | ||
| } | ||
| "description": "Supports ERC-20, ERC-721, and ERC-1155 tokens.", |
There was a problem hiding this comment.
I feel like we don't have a great handle across all our codebases on the use of the term token vs asset. Part of me feels like asset is less confusing when we're referring to both ERC20 tokens and ERC1155/ERC721 tokens since the latter are much more commonly referred to as NFTs and, as such, thought of as a separate category from (fungible) tokens. Anyone have thoughts on this and whether it makes sense to change the term token to asset here? cc @BelfordZ @vandan @shanejonas @jiexi
There was a problem hiding this comment.
Token is in the name for these types so we could stick with the term even though our method name uses the more generic watchAsset ¯_(ツ)_/¯
There was a problem hiding this comment.
Can we update the method-level and type-level description to specify that ERC-721 & ERC-1155 support is only available in the Extension and considered stability level (1) "Experimental", which is subject to change?
|
LGTM, but lets also try to coordinate merging this + making a new release of metamask-docs w/ the release of extension |
This PR implements the changes to
wallet_watchAssetas proposed in MIP-1.