RMRK multi asset implementation #14
Conversation
| token: Id, | ||
| #[ink(topic)] | ||
| asset: AssetId, | ||
| } |
There was a problem hiding this comment.
I know I'm late but hopefully I can provide some help
We thought it was useful to include here the replaced asset Id. A caveat is we do it only when it was really replaced, that is for instance: You have asset A on token, then add B to replace A and also C to replace A. If B is accepted first, when you accept C, it is not really replacing A, so we would set it to zero.
I hope it makes sense 😅
There was a problem hiding this comment.
Sorry, I don't get it. Please create an issue
There was a problem hiding this comment.
I think replacing assets is not yet implement, is it?
I'll create the issue when it is
| } | ||
|
|
||
| /// Used to retrieve asset's uri | ||
| fn get_asset_uri(&self, asset_id: AssetId) -> Option<String> { |
There was a problem hiding this comment.
There's a big difference here we have on EVM and you might find useful.
Some context first:
On regular NFTs, there's tokenURI function which receives the tokenID. Some collections set the token URI for each token, and others do an enumerated version, so they have just a baseURI and append the tokenId to the baseURI. This saves them from doing 1 call for every NFT to set it's URI, and instead doing just 1!
We considered it might be important to have the same behavior (at least make it possible) for assetURI. So in this function we also receive token tokenId. That way, if the implementer wants some assets to be enumerated (so they only create one asset instead of possible thousands) it can be easily done by appending the tokenId at the end of the assetURI.
What do you think?
There was a problem hiding this comment.
apparently I did not yet implement this function, this is just a check if token exists.
I wanted to discuss it with your team. I will create an issue for someone to implement it.
There was a problem hiding this comment.
I don't think it's just a check, it does return a string. The check is just part of it.
Our suggestions is that this method should also receive the tokenId, but let's discuss please.
|
|
||
| /// Used to retrieve asset's uri | ||
| fn get_asset_uri(&self, asset_id: AssetId) -> Option<String> { | ||
| self.asset_id_exists(asset_id) |
There was a problem hiding this comment.
This call here makes my claim to rename the function even stronger 😅 It's very confusing currently.
There was a problem hiding this comment.
Not sure how your comment on renaming applies for this line. Please explain
There was a problem hiding this comment.
My suggestion is to rename asset_id_exists to get_asset_uri. It is very confusing currently.
Adding multi Asset functionality for RMRK. (assets were called resources in previous versions of RMRK)
This ink implementation follows evm implementation of multiasset defined here
Implemented functions are:
Getters
Events
Test