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

SIP-29 Improvements #155

Merged
merged 13 commits into from
Dec 6, 2024
Merged

SIP-29 Improvements #155

merged 13 commits into from
Dec 6, 2024

Conversation

GuillaumeRx
Copy link
Contributor

@GuillaumeRx GuillaumeRx commented Dec 5, 2024

This PR tries to add a permission and handlers definition to the SIP-29 (#154). It also propose to use asset rather than the token denomination.

@GuillaumeRx GuillaumeRx requested review from Montoya and a team as code owners December 5, 2024 11:24
SIPS/sip-29.md Outdated Show resolved Hide resolved
SIPS/sip-29.md Outdated Show resolved Hide resolved
SIPS/sip-29.md Outdated Show resolved Hide resolved
SIPS/sip-29.md Outdated Show resolved Hide resolved
@danroc
Copy link
Contributor

danroc commented Dec 5, 2024

I wasn't sure whether to use asset or token in the method names, as asset is more generic and could include NFTs, for instance, where the details payload would differ significantly from that of tokens.

@GuillaumeRx
Copy link
Contributor Author

GuillaumeRx commented Dec 5, 2024

I wasn't sure whether to use asset or token in the method names, as asset is more generic and could include NFTs, for instance, where the details payload would differ significantly from that of tokens.

I wonder if this API couldn't be versatile enough that we could add the support for NFT down the road rather than having multiple endpoints. It should just be a return type change

@FrederikBolding
Copy link
Member

I wasn't sure whether to use asset or token in the method names, as asset is more generic and could include NFTs, for instance, where the details payload would differ significantly from that of tokens.

I wonder if this API couldn't be versatile enough that we could add the support for NFT down the road rather than having multiple endpoints. It should just be a type change

I would strongly prefer this as well.

SIPS/sip-29.md Outdated
```typescript
import { OnAssetDescriptionHandler } from "@metamask/snaps-sdk";

export const onAssetDescription: OnAssetDescriptionHandler = async ({
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this will be covered in another doc, but I'm curious how data is added to this list provided by the caller. How are assets discovered originally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is covered by the kering API with the listAccountAssets method IIRC @danroc can provide more details about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then the snap uses any API that provides asset metadata to create its token description

SIPS/sip-29.md Outdated
{
"initialPermissions": {
"endowment:assets": {
"chains": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we allow namespaces as well? And we should probably name it scopes to be aligned with SIP-26, CAIP-25/27.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's your thinking for allowing namespaces ? I would prefer this to be granular and to have to define reference supported rather than defining namespaces that would probably introduce calls for reference that are not supported

@danroc
Copy link
Contributor

danroc commented Dec 6, 2024

I wasn't sure whether to use asset or token in the method names, as asset is more generic and could include NFTs, for instance, where the details payload would differ significantly from that of tokens.

I wonder if this API couldn't be versatile enough that we could add the support for NFT down the road rather than having multiple endpoints. It should just be a type change

I would strongly prefer this as well.

Me too :)

What we did in the listAccountTransactions was to create a tagged union ({fungible: true; ... } | {fungible: false; ... }), maybe we should apply the same pattern here, since fungible and non-fungible assets have very distinct properties.

@danroc danroc merged commit 5712f64 into dr/assets Dec 6, 2024
3 checks passed
@danroc danroc deleted the gr/assets-updates branch December 6, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants