Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SIP-29: Snap Assets API #154
base: main
Are you sure you want to change the base?
SIP-29: Snap Assets API #154
Changes from 6 commits
7a1176b
8f7b7d0
f0a9807
03b33e5
5712f64
ec71c69
5657a9c
fe5f635
6fa4590
96f0aa1
03772b3
2cd9315
2219c5d
16bea28
fb45e98
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
is this function implemented within the @metamask/snap-sdk ? is it part of the assets team scope ?
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.
No, the Assets team will be a caller of this method, but its implementation will be handled by other teams (Solana and Snaps).
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.
hey @danroc
The data we currently have does not include MarketData, which means we cannot display the 1-day price changes or chart-based price changes. I believe this should be updated to include an interface like the following:
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.
Hey @salimtb, thanks! We didn’t plan to include market data in the first release, so we left it out of this API to give ourselves more time to figure out what’s really needed.
Are there any fields you’d like to add or remove from the current EVM implementation? Now might be a good time to make any changes you’ve been thinking about.
Also, it’s not a big deal if it gets added later to this SIP or even a new one.
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.
thanks for your response, @danroc. Not all the fields are currently in use, but we decided to keep them for potential future features. If you'd like, I can provide you with a list of the properties that are currently being used.
all of the properties are returned by the endpoint
/spot-price
of the price APIThere 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.
We should create a CAIP for 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.
This is a good point. This is technically CAIP-19, but with unregistered namespaces.
I started a discussion on the CAIPs repo, let's see what other think of it.
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.
Some examples might be differentiating between brc-20 and rune tokens on bitcoin. Or eth, erc20s, and staking contracts on ethereum. I anticipate more types of tokens than just native vs not. Not sure if anything built into caip19 already helps distinguish 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.
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 think it has to be an icon, the static token icon server is going to return pngs
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: Do we want to limit the size of this icon somehow?