Token Metadata extension to form standard#2439
Token Metadata extension to form standard#2439afa7789 wants to merge 54 commits into0xMiden:nextfrom
Conversation
55282e1 to
b1dba96
Compare
| account_storage_mode: AccountStorageMode, | ||
| auth_scheme: AuthScheme, | ||
| name: Option<TokenName>, | ||
| logo_uri: Option<TokenLogoURI>, |
There was a problem hiding this comment.
Why do we have both logo_uri & content_uri? we can rename it later for NFTs
There was a problem hiding this comment.
yeah, renaming it, u right.
There was a problem hiding this comment.
Thank you. contract_uri would be a better option.
|
We should also consider adding the corresponding metadata and the new constructor to the network fungible faucets: https://github.com/afa7789/miden-base/blob/f7426116833b1f76da3195738ccb838a52880f80/crates/miden-standards/src/account/faucets/network_fungible.rs#L93-L101 |
|
@afa7789 we should also add a flag and procedure to change max_supply. It's basically similar to have we have done in |
In this branch ? pr ? |
Yes, it would be better if you can have this in this PR. |
|
@bobbinth @mmagician this is ready for review :) |
bobbinth
left a comment
There was a problem hiding this comment.
Not a review - but I left a couple of comments inline. The main one is about how we can handle returning large amounts of data from account interface procedures.
| #! | ||
| #! Invocation: call | ||
| @locals(24) | ||
| pub proc get_contract_uri |
There was a problem hiding this comment.
I know we'll replace content URI with other fields, but I think how to return such large chunks of data is a point worth discussing:
Since we can't return more than 16 elements via the stack, the convention we usually use is to return hash of the underlying data, and then the caller can then "unhash" it locally. To make this less awkward to work with, we wrap such a procedure that does hashing/unhashing and then the caller can just use the wrapper.
The wrapper could look like so:
#! Write the content URI into the memory address specified by the ptr.
#!
#! Inputs: [ptr, pad(15)]
#! Outputs: [pad(16)]
#!
#! Invocation: exec
pub proc get_content_uri
# TODO: call.get_content_uri_commitment
# TODO: unhash the commitment into memory
end
The unhashing can be done using miden::core::mem::pipe_double_words_preimage_to_memory (or related) procedure.
Notice two points:
- We still need
get_content_uri_commitmentprocedure which would be invoked via acallinstruction. As a part of this procedure we should add an entry to the advice map to insert the actual URI data into it. - The
get_content_uriprocedure would need to be invoked using anexecinstruction because it is just a wrapper around the actual account interface procedure.
AFAIK, we haven't used this pattern for account interface procedures before - so, would love to get some thoughts from @PhilippGackstatter and @mmagician if they think we could handle this somehow differently.
But, we do use this pattern for kernel procedures - e.g., in active_note::get_assets procedure.
There was a problem hiding this comment.
@bobbinth I believe this is a common issue that we have also experienced this to have get_signers procedure in the authentication level (see the conversation here: #2390 (comment)). I know this pattern is also used in active_note::get_assets but it is complex to implement for each procedure when we face this situtation.
It would be great to have a more generic pattern to resolve this issue by providing an interface. (the procedure would have these fields initial_ptr, end_ptr, num_of_elements so that we can write to the memory and show in the stack easily and a generic way. CC. @PhilippGackstatter @mmagician
There was a problem hiding this comment.
It would be great to have a more generic pattern to resolve this issue by providing an interface. (the procedure would have these fields
initial_ptr,end_ptr,num_of_elementsso that we can write to the memory and show in the stack easily and a generic way
I think we have most of this already in the core library:
miden::core::mem::pipe_double_words_preimage_to_memorytakes a hash and writes its pre-image to memory.adv.insert_meminstruction injects data from memory into the advice provider.
The wrapper procedures would need to be customized for a specific use case, but they should be pretty simple. Let's put these together for a singe example (e.g., description) and then applying this to other fields should be pretty straight-forward.
|
@afa7789 Additionally, as this discussion #2423 (comment) has been concluded, you can update the PR with the following:
For
|
5548bd5 to
609b355
Compare
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct LogoURI([Word; 6]); |
There was a problem hiding this comment.
I assume this type, ExternalLink, TokenName and Description use the encoded representation ([Word; 6]) instead of String to match TokenSymbol? In general I like matching existing approaches, but in this case, I think we should actually use the "un-encoded" internal representation (and refactor TokenSymbol at some point), because when you work with these types, you typically want them easily representable as string, e.g. to be able to (cheaply, without decoding) write things like if token_name.as_str() == "miden" { ... }, whereas you don't care about the encoded representation in such cases.
So eventually we should refactor TokenSymbol to use a string internally, but not for this PR. For the new types though I would suggest choosing a better representation, i.e.:
pub struct TokenName(Box<str>);
pub struct Description(Box<str>);
pub struct LogoURI(Box<str>);
pub struct ExternalLink(Box<str>);Box<str>instead ofStringbecause these types are immutable and soStringis unnecessary andBox<str>'s stack size is 33% smaller.- Each type should enforce as an invariant that it can be successfully encoded into the respective number of felts/words, e.g. iiuc check that it does not exceed
NAME_UTF8_MAX_BYTES, contains valid characters, etc. at construction time. - We can add
Description::to_words(&self) -> [Word; 6](and analogously for the others) to get the encoded representation and probablytry_from_wordif necessary for the decoding.
| let metadata = TokenMetadata::with_supply( | ||
| symbol, | ||
| decimals, | ||
| max_supply, | ||
| token_supply, | ||
| name, | ||
| None, | ||
| None, | ||
| None, | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
This looks quite unwieldy because of all the optional fields. I would introduce a builder at this point, e.g.:
TokenMetadataBuilder::new(name, symbol, max_supply)
.token_supply(...)
.description(...)
.logo_uri(...)
.external_link(...)
.build()?;Token supply can default to 0 if not explicitly set as it does now, I think. Since symbol and name are so closely related, I would consider combining them (maybe in a separate PR) to further reduce the number of fields needed here. Though maybe this isn't really possible if these end up being part of different types (TokenMetadata and Info).
The builder as a whole could be done as a follow-up.
| //! | `metadata::initialized_config` | `[desc_init, logo_init, extlink_init, max_supply_mutable]` | | ||
| //! | `metadata::mutability_config` | `[desc_mutable, logo_mutable, extlink_mutable, max_supply_mutable]` | |
There was a problem hiding this comment.
Is it correct to have max_supply_mutable in both of these? It seems like it should not be present in initialized_config.
I'm also wondering if we definitely need those. If we do need them, I would combine this data into a single slot, e.g. by using a word with the following layout:
felt 0: [62 zero bits | is_desc_mutable (1 bit) | is_desc_initialized (1 bit)],
felt 1: [62 zero bits | is_logo_mutable (1 bit) | is_logo_initialized (1 bit)],
...
But maybe we can do without the _initialized ones in any case. For instance, for token symbols we encode the length into the felt, and so we know that a Felt::ZERO never encodes a valid token symbol, because valid symbols must have length > 0. If we could do something similar for description, logo, extlink, could we not say that an encoded [Word::empty(); 6] means they are absent? Then we wouldn't need at least the _initialized slot.
| let felts: Vec<Felt> = padded | ||
| .chunks_exact(8) | ||
| .map(|chunk| { | ||
| Felt::try_from(u64::from_le_bytes(chunk.try_into().unwrap())) | ||
| .expect("u64 values from 8-byte chunks fit in Felt") | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
Felt::try_from(u64::from_le_bytes([u8::MAX; 8])) panics, so the expect message is not correct.
In general, I'm not clear on what our stance on the encoding is:
TokenSymboluses our own custom encoding and is able to pack a lot into a single felt.- I think the compiler packs 4
u32s in a felt for its memory layouts, which I think may be what we're going for withTokenName? - And then we have this method used for description, logo and external link which wants to fill up a felt completely.
I think since storage is more premium than memory, it would make sense to try and be efficient, e.g. like TokenSymbol. Since this code will be used to decode token names, etc. from felts, I think the exact encoding is not as important, but I could be wrong.
Maybe we should try to generalize the TokenSymbol encoding (or something else that's efficient) for strings that works across multiple elements and words, and then use that across the board for symbol, name, description, logo uri and external link? This encoding would include the length of the string, which would make it easy to interpret all zero words as that field being absent and mapping it to None on the Rust side (related to my other comment).
cc @bobbinth
There was a problem hiding this comment.
I'd probably treat name, description, and URLs separately from the TokenSymbol. I think for token symbol the encoding could be much more restrictive and we do want to fit into a single felt. While for these fields, we should try to use UTF-8 encoding. I think there are two questions that we need to answer:
- How many bytes do we store per field element. The two options here are: 4 bytes (this would be more consistent with how the compiler treats memory) or 7 bytes (this would be more compact). I think compatibility is a bit more important here than efficiency - and so, I'd probably go with 4 bytes (even if we need to bump up the number of words to 8). But maybe there are also good arguments for being more compact?
- Where do we store the length of the string? This could be stored in the first byte - or maybe separately (in the config somewhere). I haven't yet thought about what's the best option here.
There was a problem hiding this comment.
Storage is more premium than memory, so I think I'd prefer the 7 bytes-per-felt approach. These fields are also not useful to be accessed on-chain, and so it seems like compatibility with compiler-generated memory layouts would be relatively unimportant. The exception is when those fields are being updated.
There could be more aspects to it though, that I'm missing.
I think I have a slight preference for including the length in the first byte (next to the UTF-8 bytes), mainly to keep the string contained in a single slot rather than parts of it distributed across multiple slots. Both approaches have pros and cons so not a very strong opinion.
There was a problem hiding this comment.
This change would also reduce the bytes capacity. Right ?
- Old: 24 felts × 8 bytes = 192 bytes capacity
- New: 24 felts × 7 bytes − 1 length byte = 167 bytes capacity
And if we do it with 4 bytes it would be half ( obviously )
The three 6-Word fields lose 25 bytes. if we do it with 7bytes should we also bump it to 7 words ?
cc: @onurinanc
There was a problem hiding this comment.
@afa7789 I would go with the 7 bytes approach and increase the number of words from 6 to 7, this would get us 195 bytes capacity, which is more closer to our previously targeted 192 bytes capacity.
There was a problem hiding this comment.
I will start implementing the change. If anything else is decided here, I refactor.
There was a problem hiding this comment.
One thing that's not clear to me yet is how we'd specify the types for these words when defining component schema. I guess for now we'd just have to define them as generic words and in this case, whether we use 4 or 7 bytes per element doesn't really matter.
I would love to be able to have a "string" type in component schema - but that'll probably have to wait until #2176 is implemented.
So, long story short - I think 7 bytes per felt is fine, and encoding the length as the first byte is probably fine too.
| /// | ||
| /// Bytes are packed little-endian, 4 bytes per felt (8 felts total). The string is | ||
| /// zero-padded to 32 bytes. Returns an error if the UTF-8 byte length exceeds 32. | ||
| pub fn name_from_utf8(s: &str) -> Result<[Word; 2], NameUtf8Error> { |
There was a problem hiding this comment.
Nit: I think this module is quite packed and the relation of each function to the respective type is not as easy to understand. I think it could be clearer if these were methods on the respective types, e.g. TokenName::to_words(&self) -> [Word; 2] and TokenName::from_words([Word; 2]) -> Result<Self, _>. Similarly for the other encoding/decoding functions. Wdyt?
| /// - Slot 12–17: logo_uri (6 Words) | ||
| /// - Slot 18–23: external_link (6 Words) | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct Info { |
There was a problem hiding this comment.
Why is this not part of TokenMetadata directly?
It feels very closely related, so I would consider including it. Making it a separate component means users always need to remember to include it in their account next to TokenMetadata and need to decode both TokenMetadata and Info to get all the related data, so this pushes some complexity up the stack.
It would also be nice if we could set mutability flags directly via the mentioned TokenMetadataBuilder, e.g. TokenMetadataBuilder::new(...).description("abc").mutable_description().build().
Related, I think Info does not be an AccountComponent, since it does not have any code. This suggests it is a set of standard storage slots but not a full account component (a combination of functionality / code and storage). So in the same way as TokenMetadata is not an account component (but more like a standardized storage slot), we could make Info a reusable set of storage slots. I would then include it in TokenMetadata, which in turn is included in BasicFungibleFaucet (a proper account component). Notably, this does not prevent reusing Info for other purposes in the future (such as for NFTs).
Naming: I think this is more aptly described as TokenMetadata. This is more generic metadata than what is currently called TokenMetadata which is specific to fungible assets. So maybe it is better to rename the current TokenMetadata to FungibleTokenMetadata to free up that name for this.
| description: Option<[Word; 6]>, | ||
| logo_uri: Option<[Word; 6]>, | ||
| external_link: Option<[Word; 6]>, |
There was a problem hiding this comment.
Could we not use the types we have for these, description: Description, etc.? I think this results in the APIs being too low-level, e.g. with_name([Word; 2]) or with_name_utf8(&str) are easy to use incorrectly (little help from the type system) and they don't enforce the invariants of the stronger types (min/max length, for instance).
This is a lesson learned from a few similarly low-level APIs we had in the past, where we had Word for too many different things and got things wrong too easily. One of the outcomes of that is #2431 which introduces a wrapper over a key: Word just to give more type safety.
| if let Some(logo_uri) = extension.logo_uri { | ||
| for (i, word) in logo_uri.iter().enumerate() { | ||
| storage_slots.push(StorageSlot::with_value(Info::logo_uri_slot(i).clone(), *word)); | ||
| } | ||
| } |
There was a problem hiding this comment.
So far, we basically never had "optional" storage slots. The main reason for this I think is that on-chain code does (not yet?) have the ability to check if a slot is present or not. Instead, accessing a slot via get_ APIs will simply panic if it isn't present, and account components therefore implicitly assume that all slots are present and initialized.
I guess here the situation is a bit different in that on-chain code never accesses the logo, external link, description or name of the token, and so maybe this is fine. At least I don't see a reason why this would be problematic.
Two caveats:
- As long as the
AccountStorage->AccountComponentdecoding process can deal with an absent storage slot, this should be fine. - The way we define if an account component is present is via procedure MAST roots and storage slots. The optional storage slots could not be used to help with this detection. If all slots are optional that would be problematic since then you can't say for sure if a component is absent or present. I think as long as one storage slot is guaranteed to be present (like the
initialized_configfor theInfo), that should be fine.
(This is more of a general comment - I still think Info should not be an account component).
Edit: Though maybe we will have code that updates the metadata on-chain and so it would actually access the slots, in which case that could panic if the slots happen to not have been initialized. That then raises whether on-chain code could add the slot itself, which we could consider a storage upgrade (#2183).
|
@PhilippGackstatter I did most of the mentioned things. |
ebcbd2e to
671a9dc
Compare
ecf267b to
2e50884
Compare
|
@bobbinth @PhilippGackstatter please review |
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Left a few comments. I'm primarily wondering if we need the on-chain metadata getters and I think we should turn TokenMetadata into a proper AccountComponent with code.
Separately, it would be awesome to split this PR into multiple ones given it adds 4500 lines of code in one go (even though 2100+ is test code).
Maybe we can try to pull the following changes into a separate PR X:
- Rename
TokenMetadatatoFungibleTokenMetadataunless the diff is very small). - Add
TokenName,ExternalLink, and related types and deal with the string encoding.
This PR (2439) can then be rebased on top of X.
crates/miden-agglayer/src/lib.rs
Outdated
| ) -> Result<Self, FungibleFaucetError> { | ||
| let metadata = TokenMetadata::with_supply(symbol, decimals, max_supply, token_supply)?; | ||
| // Use empty name for agglayer faucets (name is stored in Info component, not here). | ||
| let name = TokenName::new("").expect("empty string is valid"); |
There was a problem hiding this comment.
| let name = TokenName::new("").expect("empty string is valid"); | |
| let name = TokenName::default() |
Nit: Should we have default or empty to construct this without the expect?
| pub struct BasicFungibleFaucet { | ||
| metadata: TokenMetadata, | ||
| metadata: FungibleTokenMetadata, | ||
| info: Option<TokenMetadataInfo>, | ||
| } |
There was a problem hiding this comment.
I think we should apply the same pattern here as in #2292, where we initially had NetworkFungibleFaucet contain owner: AccountId and then refactored Ownable2Step into its own account component (in #2572). Here, we would move TokenMetadata(Info) out of BasicFungibleFaucet.
So we would have:
pub struct BasicFungibleFaucet {
metadata: FungibleTokenMetadata,
}When building an account, we'd pass BasicFungibleFaucet and TokenMetadata as two components. I think these don't depend on each other, right? So the faucet doesn't use code from the metadata and vice versa?
The only dependency is BasicFungibleFaucet needing token_supply, which is part of FungibleTokenMetadata, so it makes sense for that to stay where it is.
If that's true, then I think we can remove FungibleTokenMetadata::to_token_metadata_info and remove the new fields added in FungibleTokenMetadata (these are duplicates of the fields in TokenMetadata.
| // Metadata Info component uses the standards library (get_name, get_description, etc. from | ||
| // metadata). | ||
| static METADATA_INFO_COMPONENT_LIBRARY: LazyLock<Library> = | ||
| LazyLock::new(|| Library::from(crate::StandardsLib::default())); |
There was a problem hiding this comment.
Related to a previous comment about TokenMetadata being its own account component: We can change this to a library that basically does pub use ::miden::standards::metadata::fungible::* (what crates/miden-standards/asm/account_components/faucets/network_fungible_faucet.masm currently re-exports) and use it as the library for TokenMetadata.
When we do this, we can remove the corresponding re-exports from basic and network fungible faucet, because these procedures would be provided by the TokenMetadata component instead.
The downside is that if we create an account with BasicFungibleFaucet + TokenMetadata then the account would also export the setters. If I understand correctly, these would fail at runtime because mutability would not be configured and so that's probably fine, albeit not ideal.
There was a problem hiding this comment.
This file is testing a standard, so I'd rename it to crates/miden-testing/src/standards/token_metadata.rs
It would be great to check if we can consolidate some of these using shared test setup functions or rstest to make this easier to review and maintain (primarily get down the number of lines).
| pub use ::miden::standards::metadata::fungible::get_name | ||
| pub use ::miden::standards::metadata::fungible::get_mutability_config | ||
| pub use ::miden::standards::metadata::fungible::is_max_supply_mutable | ||
| pub use ::miden::standards::metadata::fungible::is_description_mutable | ||
| pub use ::miden::standards::metadata::fungible::is_logo_uri_mutable | ||
| pub use ::miden::standards::metadata::fungible::is_external_link_mutable | ||
| pub use ::miden::standards::metadata::fungible::get_description_commitment | ||
| pub use ::miden::standards::metadata::fungible::get_description | ||
| pub use ::miden::standards::metadata::fungible::get_logo_uri_commitment | ||
| pub use ::miden::standards::metadata::fungible::get_logo_uri | ||
| pub use ::miden::standards::metadata::fungible::get_external_link_commitment | ||
| pub use ::miden::standards::metadata::fungible::get_external_link | ||
| pub use ::miden::standards::metadata::fungible::get_token_metadata | ||
| pub use ::miden::standards::metadata::fungible::get_max_supply | ||
| pub use ::miden::standards::metadata::fungible::get_decimals | ||
| pub use ::miden::standards::metadata::fungible::get_token_symbol | ||
| pub use ::miden::standards::metadata::fungible::get_token_supply |
There was a problem hiding this comment.
Question: Are there use cases for accessing this data on-chain? I'm wondering what a contract would do with a logo URI or an external link. The TokenSymbol we've had before doesn't have a getter either because it is not acessed on-chain. So, from what I can tell, this metadata is only accessed off-chain, and so we wouldn't need these getters.
The setters on the other hand are required for updating metadata, so these make sense to me.
There was a problem hiding this comment.
@PhilippGackstatter as a part of the token standard, the on-chain data is a source of canonical trust, and we can not trust the off-chain data for some variables, especially this variables should be on-chain:
- name
- decimals
- symbol
- token supply
For the logo_uri and external_link and description might be off-chain as long as you could provide one of them as a reference, such as we could just have contract_uri would contain other metadata, and then we need to provide a scheme as we have discussed here: as we have discussed here: #2423
Then, we come to the point that instead providing a scheme for a contract_uri we would provide logo_uri, external_link and description as optional slots.
CC: @bobbinth for this.
Additionally, I believe the naming that we use here as get_ is so much repetitive and although it defines exactly what the procedures does, we don't generally use get_ as part of the standards.
A part of an example ERC20 token standard in Ethereum:
function name() public view virtual returns (string memory) {
return _name;
}
function symbol() public view virtual returns (string memory) {
return _symbol;
}
function decimals() public view virtual returns (uint8) {
return 18;
}
function totalSupply() public view virtual returns (uint256) {
return _totalSupply;
}A part of an ERC20 Standard in StarkNet for Cairo
impl ERC20Metadata<
TContractState,
+HasComponent<TContractState>,
impl Immutable: ImmutableConfig,
+ERC20HooksTrait<TContractState>,
> of interface::IERC20Metadata<ComponentState<TContractState>> {
/// Returns the name of the token.
fn name(self: @ComponentState<TContractState>) -> ByteArray {
self.ERC20_name.read()
}
/// Returns the ticker symbol of the token, usually a shorter version of the name.
fn symbol(self: @ComponentState<TContractState>) -> ByteArray {
self.ERC20_symbol.read()
}
/// Returns the number of decimals used to get its user representation.
fn decimals(self: @ComponentState<TContractState>) -> u8 {
Immutable::DECIMALS
}
}I believe you should also consider using a general naming convention of OpenZeppelin in the naming by removing get_
There was a problem hiding this comment.
This is related to the limitation of returning 16 elements through a call: For instance, you get the commitment via call.get_description_commitment which also insertes the preimage into the advice map. Then you pipe the the data from the advice map into memory and check that it matches COMMITMENT.
I think the approach here using get_description_commitment and get_description wouldn't quite work like that because get_description must be exec-uted, and so executes in the context of the caller, not the account, but internally it calls procedures of the account (get_description_chunk_0).
My question is whether we need any of this at all if there is no use to access this data on-chain.
There was a problem hiding this comment.
My question is whether we need any of this at all if there is no use to access this data on-chain.
@PhilippGackstatter I think this should answer your question: #2439 (comment)
There was a problem hiding this comment.
the on-chain data is a source of canonical trust, and we can not trust the off-chain data for some variables
That makes sense! To be clear: It makes sense to me that this data must be stored on-chain. What I'm asking is if the data must also be accessed on-chain. For access, we need the getters, but for storage we don't.
Off-chain (say in the Miden explorer to display metadata), you'd access the data by syncing the faucet account (and its storage) via the client and then use something like TokenMetdadata::try_from_storage(faucet_account.storage())?, e.g. like here:
protocol/crates/miden-testing/tests/scripts/faucet.rs
Lines 288 to 292 in 69f0c9e
So the MASM getters would not be involved in this at all.
I believe you should also consider using a general naming convention of OpenZeppelin in the naming by removing
get_
I'm not sure about this. In Rust, I like avoiding the get_ prefix (as is convention), e.g. token_metadata.token_supply(). In MASM, we don't have methods, only procedures. fungible::get_token_supply is a bit clearer than fungible::token_supply, though the latter is fine.
We currently use get_ consistently (afaict), but if we change this it would be great to do this across all APIs. Then I'm not sure if active_account::get_item can be replaced by active_account::item without loosing clarity.
There was a problem hiding this comment.
Off-chain (say in the Miden explorer to display metadata), you'd access the data by syncing the faucet account (and its storage) via the client and then use something like TokenMetdadata::try_from_storage(faucet_account.storage())?, e.g. like here:
I got your point! It's a nice point! For, description, logo_uri, and external_link, I don't think there are much use cases to access the data on-chain. So, I think removing the getters would be better.
I'm not sure about this. In Rust, I like avoiding the get_ prefix (as is convention), e.g. token_metadata.token_supply(). In MASM, we don't have methods, only procedures. fungible::get_token_supply is a bit clearer than fungible::token_supply, though the latter is fine.
We currently use get_ consistently (afaict), but if we change this it would be great to do this across all APIs. Then I'm not sure if active_account::get_item can be replaced by active_account::item without loosing clarity.
That's totally clear to me now.
There was a problem hiding this comment.
Related to this: #2585, I think we need name, symbol, and symbol to be accessed on-chain to verify the correct hash, or is it still satisfied by an off-chain access? @PhilippGackstatter @mmagician
There was a problem hiding this comment.
Good catch, it looks like that is a use case where we'd need to access name, symbol and decimals on-chain.
Then I think we should drop the description, logo_uri and external_link getters.
There was a problem hiding this comment.
@afa7789 could you remove the getters for description, logo_uri and external_link when possible?
There was a problem hiding this comment.
I think overall we can proceed with two approaches for the metadata structure. In general, I believe the goal was to have TokenMetadata contain token metadata that is not specific to fungible or non-fungible assets, so we can reuse it easily in the future.
Approach 1
Keep fungible metadata in BasicFungibleFaucet and extend it with TokenMetadata.
So we'd have:
FungibleTokenMetadata: Helper structTokenMetadata: Account componentBasicFungibleFaucet: Account component containingFungibleTokenMetadata
Metadata functionality would be split in two:
#! Re-exported from module miden::standards::components::token_metadata
pub use ::miden::standards::metadata::fungible::get_name
pub use ::miden::standards::metadata::fungible::get_mutability_config
pub use ::miden::standards::metadata::fungible::is_max_supply_mutable
pub use ::miden::standards::metadata::fungible::is_description_mutable
pub use ::miden::standards::metadata::fungible::is_logo_uri_mutable
pub use ::miden::standards::metadata::fungible::is_external_link_mutable
pub use ::miden::standards::metadata::fungible::optional_set_description
pub use ::miden::standards::metadata::fungible::optional_set_logo_uri
pub use ::miden::standards::metadata::fungible::optional_set_external_link
pub use ::miden::standards::metadata::fungible::optional_set_max_supply
#! Re-exported from module miden::standards::components::faucets::basic_fungible_faucet
pub use ::miden::standards::metadata::fungible::get_token_metadata
pub use ::miden::standards::metadata::fungible::get_max_supply
pub use ::miden::standards::metadata::fungible::get_decimals
pub use ::miden::standards::metadata::fungible::get_token_symbol
pub use ::miden::standards::metadata::fungible::get_token_supply
Right now we export all of these from the faucet components, but since fungible metadata is defined by the faucets rather than TokenMetadata, it makes sense to re-export it from there.
Approach 2
Unify generic and fungible metadata in FungibleTokenMetadata.
So we'd have:
TokenMetadata: Helper structFungibleTokenMetadata: Account component that wrapsTokenMetadataand re-exports all its functionality and storage slotsBasicFungibleFaucet: Account component without storage slots (pub struct BasicFungibleFaucet;). It would depend onFungibleTokenMetadatabeing present in the account.
Metadata functionality would be unified:
#! Re-exported from module miden::standards::components::fungible_token_metadata
# All of the above mentioned metadata-related procedures are re-exported here.
pub use ::miden::standards::metadata::fungible::*
The second approach results in a slightly more cohesive DevX, since all metadata is accessible via one Rust type and via one component's procedures, so I think my preference is slightly on that approach.
| #! - the note sender is not the owner. | ||
| #! | ||
| #! Invocation: call (from note script context) | ||
| pub proc optional_set_description |
There was a problem hiding this comment.
Should we drop the optional prefix? I think we don't need to indicate in the name that this only works if the mutability flag is set.
There was a problem hiding this comment.
This module is pub and contains many pub free functions/statics/consts. I think we should try to reduce the things that are public to the necessary ones:
- The
statics that define slot names should be exported by the account components that have these slots, e.g.TOKEN_METADATA_SLOTasFungibleToken::token_metadata_slot. So we can makemiden_standards::account::metadata::token_metadata_slotandTOKEN_METADATA_SLOTprivate or at mostpub(crate). The same may apply to the other slot names. - A few functions are unused (but do not get flagged as unused because the module is public), e.g.
field_from_bytes,LOGO_URI_DATA_KEY,name_from_utf8.
I'd also suggest splitting the TokenMetadata into its own module since this module is large now and contains the unrelated AccountSchemaCommitment.
Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
…minting constraints
…ndard setters and remove unused commitment functions
…hanced account metadata management, split from mod.
|
@PhilippGackstatter , I have coded the suggestions you made, and reviewed here on my side, you mind taking a look? |
Unified metadata: One place for account/faucet metadata: token (symbol, decimals, max_supply), owner, name, and content URI. Slot names live under miden::standards::metadata::* (and ownable for owner).
Layout: Token metadata and owner in slots 0–1; name in 2 words (name_0, name_1); content URI in 6 words (content_uri_0..5). Same layout in Rust and MASM.
Faucets: Basic and network fungible faucets support optional name and content URI; both re-export metadata getters (get_name, get_content_uri, get_token_metadata, get_max_supply, get_decimals, get_token_symbol; network also get_owner).
Standalone Info: Non-faucet accounts can use the metadata Info component (name + content URI) for future use (e.g. NFTs).
Testing: Unit tests in miden-standards (metadata storage, recovery); integration tests in miden-testing (MASM getters, faucet + metadata).