feat: Introduce high-level types for templates #1124

igamigo · 2025-02-05T23:17:19Z

Provides a way of expressing types in placeholder values within a component template (currently hardcoded to a list of variants).
Comes with some refactors, hopefully improving code a bit. There are some other refactors that I would like to make and will make explicit in this PR later. Multi-slot value templating has not been implemented yet due to lack of native types that would use them.
Currently allows for toml definitions such as this:

name = "Test Component"
description = "This is a test component"
version = "1.0.1"
targets = ["FungibleFaucet", "RegularAccountImmutableCode"]

[[storage]]
name = "token_metadata"
description = "Contains metadata about the token associated to the faucet account"
slot = 0
value = [
    { type = "felt", name = "max_supply", description = "Maximum supply of the token in base units" }, # placeholder
    { type = "tokensymbol", value = "TST" }, # hardcoded non-felt type
    { type = "u8", name = "decimals", description = "Number of decimal places" }, # placeholder
    { value = "0" }, 
]

[[storage]]
name = "recallable_height"
slot = 1
type = "u32"

PhilippGackstatter

Looks good! Not a full review yet.

I wonder if FeltType and WordType are the right approach, since users won't be able to extend them with their own types. It doesn't feel right to have FeltType::TokenSymbol or WordType::RpoFalcon512PublicKey, as these should not be hard-coded. If that's just the approach for now, then I suppose that's fine, but to support user-extensibility, we'd need a different approach, right?

I think something like the TemplateType trait is a good direction for extensibility. Ideally we could implement this for Felt, Word within miden-base, and users can implement it for UserType and then register all of them in some Map<TypeName, Box<dyn TemplateType>> (or something like that). If some entry is specified to be of type TokenSymbol, then we look this up in the map and find an opaque parse function which only needs to convert a &str to a Felt, afaict, and in the process verify that it is a valid instance of TokenSymbol. In that way, we should be able to be opaque over the concrete types. I'm not completely sure yet whether every type should be convertible to Felt and Word. TokenSymbol only converts to Felt and RpoFalcon512PublicKey only converts to Word, so maybe this needs to be reflected in the approach, perhaps by having to different FeltTemplateType and WordTemplateType traits?

Does this make any sense and do you think this would be possible and support user extensibility? I'm not saying that we have to do this in this PR or at all, just trying to get at the limitations of the current approach.

PhilippGackstatter · 2025-02-11T09:58:23Z

crates/miden-objects/src/account/component/template/mod.rs

+            name: "test".into(),
+            description: "desc".into(),
+            version: Version::parse("0.1.0").unwrap(),
+            targets: BTreeSet::new(),


Nit: Not necessarily in this PR, but should we align the naming of AccountComponentMetadata::targets with AccountComponent::supported_types? Seems like it would be good to make the naming consistent (in fields, methods and docs). I don't have a strong preference just wanted to bring it up.

PhilippGackstatter · 2025-02-11T10:05:34Z

crates/miden-objects/src/account/component/template/mod.rs

 /// );
+/// let storage_entry = StorageEntry::new_value(0, word_representation);


This binding is not actually passed to AccountComponentTemplate::new, which seems to have been the intention?

The test passes anyway, because the data in InitStorageData is just not used. I wonder if it should error if not all values from the InitStorageData are used? But this may be somewhat complicated to track. As long as we don't have default values, then that should be fine, since a mistyped key would still produce an error and it would not simply fallback to the default.

PhilippGackstatter · 2025-02-11T10:15:53Z

crates/miden-objects/src/account/component/template/storage/entry_content.rs

+
+    /// Returns the name associated with the word representation.
+    /// - For the `Template` variant, it always returns a reference to the name.
+    /// - For the `Value` variant, it returns `Some(&str)` if a name is present, or `None`


Suggested change

/// - For the `Value` variant, it returns `Some(&str)` if a name is present, or `None`

/// - For the `Value` variant, it returns `Some(&StorageValueName)` if a name is present, or `None`

The type seems incorrect.

Nit: I think we could also omit writing the type in the docs and only mention when it returns Some or None.

PhilippGackstatter · 2025-02-11T10:18:16Z

crates/miden-objects/src/account/component/template/storage/entry_content.rs

+        }
+    }
+
+    /// Returns the default value (an array of 4 `FeltRepresentation`s) if this is a `Value`


Nit: Is it correct to say "default value" when we don't always have a value (i.e. when it's a Template variant)? "Default" suggests it is always available.

PhilippGackstatter · 2025-02-11T10:29:37Z

crates/miden-objects/src/account/component/template/storage/entry_content.rs

+                let placeholder_prefix = placeholder_prefix.with_suffix(placeholder_key);
+                let value = init_storage_data.get(&placeholder_prefix);
+                if let Some(v) = value {
+                    let parsed_value = r#type.try_parse_word(v).unwrap();


Suggested change

let parsed_value = r#type.try_parse_word(v).unwrap();

let parsed_value = r#type.try_parse_word(v)?;

This should propagate, right?

PhilippGackstatter · 2025-02-11T10:54:20Z

crates/miden-objects/src/account/component/template/storage/mod.rs

+            match key.as_str() {
+                "token_metadata.max_supply"
+                | "token_metadata.decimals"
+                | "default_recallable_height" => continue,
+                "map_entry.map_key_template" => assert!(requirement.r#type.to_string() == "word"),
+                _ => panic!("all cases should have been covered"),


Nit: I think this would be more readable if we collect the iterator into a BTreeSet and then write individual asserts/assert_eqs to check that each key exists/has the expected value.

PhilippGackstatter · 2025-02-11T10:57:39Z

crates/miden-objects/src/account/component/template/storage/placeholder.rs

+            if !key.inner().is_empty() {
+                key.fully_qualified_name.push('.');
+            }
+            key.fully_qualified_name.push_str(&suffix.to_string());


Suggested change

key.fully_qualified_name.push_str(&suffix.to_string());

key.fully_qualified_name.push_str(suffix.inner());

Nit: Avoid allocation.

PhilippGackstatter · 2025-02-11T10:58:31Z

crates/miden-objects/src/account/component/template/storage/placeholder.rs

    pub fn inner(&self) -> &str {
-        &self.key
+        &self.fully_qualified_name


Personally I would prefer naming these kinds of functions as_str to follow the rust convention and to be more readable. When I read inner somewhere else in the code I don't really know what this returns, whereas as_str gives me that information.

PhilippGackstatter · 2025-02-11T11:03:34Z

crates/miden-objects/src/account/component/template/storage/placeholder.rs

+        if !segment.chars().all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') {
+            return Err(StorageValueNameError::InvalidCharacter {
+                part: segment.to_string(),
+                character: segment
+                    .chars()
+                    .find(|c| !(c.is_ascii_alphanumeric() || *c == '_' || *c == '-'))
+                    .unwrap(),
+            });


Could we not also use find(...) directly? If it returns None the string is valid and if it returns Some(c) we already have the offending character that we can put in the error message.

PhilippGackstatter · 2025-02-11T11:06:40Z

crates/miden-objects/src/account/component/template/storage/placeholder.rs

-                }
-            }
-        }
+impl Serializable for StorageValueName {


Do we actually need to implement Serializable for all the template-related types? Where would we use the binary representations? Looks like they are currently unused.

igamigo force-pushed the igamigo-template-types branch 3 times, most recently from c3d7263 to 2d5a9b1 Compare February 10, 2025 05:20

igamigo marked this pull request as ready for review February 10, 2025 05:23

igamigo force-pushed the igamigo-template-types branch 3 times, most recently from 8449edd to 6a6505a Compare February 10, 2025 16:07

feat: add native types to storage placeholders

386f794

igamigo force-pushed the igamigo-template-types branch from 6a6505a to 386f794 Compare February 10, 2025 16:07

bobbinth requested review from bobbinth and PhilippGackstatter February 10, 2025 17:23

PhilippGackstatter reviewed Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce high-level types for templates #1124

feat: Introduce high-level types for templates #1124

igamigo commented Feb 5, 2025 •

edited

Loading

PhilippGackstatter left a comment

PhilippGackstatter Feb 11, 2025

PhilippGackstatter Feb 11, 2025

PhilippGackstatter Feb 11, 2025

PhilippGackstatter Feb 11, 2025

PhilippGackstatter Feb 11, 2025

PhilippGackstatter Feb 11, 2025

PhilippGackstatter Feb 11, 2025

PhilippGackstatter Feb 11, 2025

PhilippGackstatter Feb 11, 2025

PhilippGackstatter Feb 11, 2025

		/// );
		/// let storage_entry = StorageEntry::new_value(0, word_representation);

	/// - For the `Value` variant, it returns `Some(&str)` if a name is present, or `None`
	/// - For the `Value` variant, it returns `Some(&StorageValueName)` if a name is present, or `None`

	let parsed_value = r#type.try_parse_word(v).unwrap();
	let parsed_value = r#type.try_parse_word(v)?;

	key.fully_qualified_name.push_str(&suffix.to_string());
	key.fully_qualified_name.push_str(suffix.inner());

feat: Introduce high-level types for templates #1124

Are you sure you want to change the base?

feat: Introduce high-level types for templates #1124

Conversation

igamigo commented Feb 5, 2025 • edited Loading

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo commented Feb 5, 2025 •

edited

Loading