-
Notifications
You must be signed in to change notification settings - Fork 15
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?
Conversation
): TokenConversionRate; | ||
``` | ||
|
||
### Fiat currency representation |
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.
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.
* add handlers and permission definition * update to use `snaps-sdk` * snap -> Snap * Add CAIP-19 ID reference in description and rename token to asset * Add chains caveat * fix typo * allow batching * add fungible reference to asset description * add appendix for fungible assets * fix typos * update rates handler to support batching * unify * rename `AssetUnit` to `FungibleAssetUnit`
fungible: true; | ||
|
||
// Base64 representation of the asset icon. | ||
iconBase64: string; |
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.
Can this be an URL link to the icon?
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?
ticker: string; | ||
|
||
// Whether the asset is native to the chain. | ||
native: boolean; |
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.
How can we identify other types of tokens that need to be handled differently?
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.
The type for an `onAssetConversion` handler function’s return value is: | ||
|
||
```typescript | ||
type AssetConversionRate = { |
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.
How can we get historical market data?
It should be closer to the terminology used by the Token API.
conversionTime: number; | ||
|
||
// The UNIX timestamp of when the conversion rate will expire. | ||
expirationTime: number; |
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:
interface MarketData {
id: string;
price: number;
marketCap: number;
allTimeHigh: number;
allTimeLow: number;
totalVolume: number;
high1d: number;
circulatingSupply: number;
dilutedmarketCap: number;
marketCapPercentChange1d: number;
priceChange1d: number;
pricePercentChange1h: number;
pricePercentChange1d: number;
pricePercentChange7d: number;
pricePercentChange14d: number;
pricePercentChange30d: number;
pricePercentChange200d: number;
pricePercentChange1y: number;
}
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 API
#### Get Asset Conversion Rate | ||
|
||
```typescript | ||
import { OnAssetConversionHandler } from "@metamask/snaps-sdk"; |
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).
fungible: true; | ||
|
||
// Base64 representation of the asset icon. | ||
iconBase64: string; |
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?
Co-authored-by: Charly Chevalier <[email protected]>
Co-authored-by: Charly Chevalier <[email protected]>
Co-authored-by: Charly Chevalier <[email protected]>
Co-authored-by: Charly Chevalier <[email protected]>
Co-authored-by: Charly Chevalier <[email protected]>
} | ||
``` | ||
|
||
`scopes` - A non-empty array of CAIP-2 chain IDs that the Snap supports. This field is useful for a client in order to avoid unnecessary overhead. |
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.
to avoid unnecessary overhead.
Since this information would be static, will the Snap only be "restricted" to those scopes? Or can Snap also returns assets informations for other non-declared scopes?
Co-authored-by: Frederik Bolding <[email protected]>
Co-authored-by: Frederik Bolding <[email protected]>
No description provided.