-
Notifications
You must be signed in to change notification settings - Fork 34
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
pset: elip100: add and get token metadata #206
pset: elip100: add and get token metadata #206
Conversation
This PR attempts to be as much consistent as possible with the existing code. We could consider adding the token metadata with the same call that adds asset metadata and check consistency there. |
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.
Concept ACK, checked test vectors matches the one in the elip doc
|
||
self.global | ||
.proprietary | ||
.get(&key) | ||
.map(|data| AssetMetadata::deserialize(data)) | ||
} | ||
|
||
/// Add token information to the PSET, returns None if it wasn't present or Some with the old | ||
/// data if already in the PSET |
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.
Should we mention in the doc that you need also to add the relative contract metadata?
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.
Or should we change add_asset_metadata
to optionally accept TokenMetadata
?
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.
Perhaps both.
In this PR I wanted to be the least disruptive possible, and just add a way to add/get token metadata in the PSET.
We could either:
- add an alternative method that sets asset and (optionally) token metadata at once (verifying their consistency and contract commitment)
- change
add_asset_metadata
to acceptOption<TokenMetadata>
But those require a bit of discussion, and it might take time.
A viable approach could be to go with this PR and later add a "safer to use" method with a separate PR.
Then we can deprecate the add/get_asset/token_metadata
, or keep them if we feel to.
In this way we can think a bit more about the interface we want to expose, while not blocking who needs to use this new fields.
ACK 1166089 |
Following changes in ElementsProject/ELIPs#17