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.

Forgot to mention I changed this to supported-types

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.

Good point. I don't think it would be too complicated to track, so maybe it is worth implementing.

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.

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

igamigo · 2025-02-11T18:07:13Z

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.

Yes, this is very similar to what I described when discussing this issue as well. The idea was to not implement this for now, but in the end I think we need some type registry which works as you described so initially to work toward this I introduced the trait. I am also not entirely sure we need a general trait right now. In general, I think we want to parse a string either as a Felt or an arbitrary number of Words. It seems to me there is some (maybe very marginal) value in keeping it all under a single trait, because there are at least some types that we might want to have in both (for example, I think u32 might make sense to use in a Felt or Word slot).
Maybe I could get rid of the FeltTye/WordType enums now and try the registry approach.

igamigo · 2025-02-17T04:07:58Z

@PhilippGackstatter I've updated the code to contain a solution that builds toward the more dynamic "type registry". One thing to note is that we can't precisely keep the type map as we discussed before because static methods are not object safe. To get around this, I'm storing a closure but you can register types that implement the traits anyway.

bobbinth

Looks good! Thank you. Not a complete review (though I looked through probably 90% of the code), but I left some comments inline.

One other thought: should we allow specifying felt values for value slots? For example, should something like this be legal:

[[storage]]
name = "foo"
slot = 0
type = "u32"

The main concern is that this may introduce some ambiguity of how the value is stored, and maybe making things more explicit would be a better way to go:

[[storage]]
name = "foo"
slot = 0
value = [
    { type = "u32", name = "bar" },
    { value = "0" }, 
    { value = "0" }, 
    { value = "0" }, 
]

Though, one obvious downside of this is that now the placeholder name would be foo.bar rather than just foo - so, not sure it is worth it.

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

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

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

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

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

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

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

PhilippGackstatter

Looks awesome! Thanks for the updates. I think the template registry is a really nice and extensible approach. For user extension we may have make it non-static, but for now this should work.

I think there is a lot of complexity here, and I wonder if we should have more tests.

PhilippGackstatter · 2025-02-17T07:33:46Z

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

+        let mut templates = BTreeMap::new();
+        for entry in self.storage_entries() {
+            for (name, requirement) in entry.template_requirements() {
+                templates.insert(name, requirement);


Can you add a comment why we don't need to check for duplicates here?
Is it because we have already checked earlier and a duplicate should resolve to the same type?

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

PhilippGackstatter · 2025-02-17T08:44:03Z

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

+/// Type alias for a function that converts a string into a [`Word`].
+type TemplateWordConverter = fn(&str) -> Result<Word, TemplateTypeError>;
+
+// TODO: Implement converting to list of words for multi-slot values


Can we do this without extending the existing traits, for example by calling a TemplateWord::parse_word multiple times? I think that'd be ideal if that works.

PhilippGackstatter · 2025-02-17T08:56:45Z

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

+                // TODO: Maybe types should be explicit every time and only assumed for bare
+                // strings?
+                let felt_type = r#type.unwrap_or(DEFAULT_FELT_TYPE.to_string());


I think we should require types in the map case, so that this is fine:

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 "0", ]

but this is not:

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" }, # err: type = "felt" missing ]

Yeah, I think I tend to prefer this as well. Although maybe it's convenient to be able to fall back to a default type. @bobbinth if you agree I can make this change.

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

crates/miden-objects/src/errors.rs

crates/miden-lib/src/account/mod.rs

igamigo · 2025-02-17T15:08:04Z

Looks good! Thank you. Not a complete review (though I looked through probably 90% of the code), but I left some comments inline.

One other thought: should we allow specifying felt values for value slots? For example, should something like this be legal:
[[storage]]
name = "foo"
slot = 0
type = "u32"
The main concern is that this may introduce some ambiguity of how the value is stored, and maybe making things more explicit would be a better way to go:
[[storage]]
name = "foo"
slot = 0
value = [
    { type = "u32", name = "bar" },
    { value = "0" }, 
    { value = "0" }, 
    { value = "0" }, 
]

I thought the idea of moving toward more general types was to impose certain restrictions on initial values for slots, regardless of the underlying type. If given the choice I'd definitely prefer not allowing your first approach and enforcing the second. This is just a matter of removing the blanket implementation and removing the registration of the type for word slots.

Though, one obvious downside of this is that now the placeholder name would be foo.bar rather than just foo - so, not sure it is worth it.

Actually, this is not currently the case. For any field that has a value, the name is optional. This was done explicitly for cases like the one you mention. So for example this:

        [[storage]]
        slot = 0
        type = "word"
        value = ["0","0","0", { type = "u32", name = "bar" }]

has these requirements:

{StorageValueName { fully_qualified_name: "bar" }: PlaceholderTypeRequirement { type: "u32", description: None }}

PhilippGackstatter

Looks good to me!

bobbinth

Looks good! Thank you! I think the outstanding comments can be addressed in follow-up PRs.

I thought the idea of moving toward more general types was to impose certain restrictions on initial values for slots, regardless of the underlying type. If given the choice I'd definitely prefer not allowing your first approach and enforcing the second. This is just a matter of removing the blanket implementation and removing the registration of the type for word slots.

Yes, let's remove the blanket implementation (in a follow-up PR). I think if we can do something like:

[[storage]]
slot = 0
type = "word"
value = ["0", "0", "0",  { type = "u32", name = "bar" }]

That makes it simple enough to handle cases where we care only about a single element.

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

igamigo added 5 commits February 11, 2025 16:19

reviews: Propagate error correctly, change test, refactor code

8bfc6b0

refactor: Fix docs, imports

ed3a3c6

refactor: publish component's libraries

9a4f5e4

refactor: change key in template requirements

96703f6

chore: Merge next

772a0d6

igamigo force-pushed the igamigo-template-types branch from 98ae187 to 7befa67 Compare February 17, 2025 04:15

bobbinth reviewed Feb 17, 2025

View reviewed changes

PhilippGackstatter reviewed Feb 17, 2025

View reviewed changes

igamigo mentioned this pull request Feb 17, 2025

Follow-ups for component templates #1052

Open

igamigo force-pushed the igamigo-template-types branch 2 times, most recently from 8d8d068 to 40e45dc Compare February 17, 2025 23:15

feat: Use a type registry

6b5d4be

igamigo force-pushed the igamigo-template-types branch from 40e45dc to e172c92 Compare February 17, 2025 23:26

reviews: Address doc comments, refactor checks, add validation of types

0d2e7b0

igamigo force-pushed the igamigo-template-types branch from e172c92 to 0d2e7b0 Compare February 17, 2025 23:27

chore: Merge next

871c0a6

PhilippGackstatter approved these changes Feb 18, 2025

View reviewed changes

bobbinth approved these changes Feb 18, 2025

View reviewed changes

igamigo merged commit dd9d79f into next Feb 18, 2025
12 checks passed

igamigo deleted the igamigo-template-types branch February 18, 2025 14:30

igamigo mentioned this pull request Feb 20, 2025

Refactor StoragePlaceholder #1109

Closed

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

igamigo Feb 12, 2025

PhilippGackstatter Feb 11, 2025

igamigo 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

igamigo commented Feb 11, 2025

igamigo commented Feb 17, 2025

bobbinth left a comment

PhilippGackstatter left a comment

PhilippGackstatter Feb 17, 2025

PhilippGackstatter Feb 17, 2025

PhilippGackstatter Feb 17, 2025

igamigo Feb 17, 2025

igamigo commented Feb 17, 2025

PhilippGackstatter left a comment

bobbinth left a comment

		/// );
		/// 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

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

Choose a reason for hiding this comment

igamigo commented Feb 11, 2025

igamigo commented Feb 17, 2025

bobbinth left a comment

Choose a reason for hiding this comment

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

igamigo commented Feb 17, 2025

PhilippGackstatter left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

igamigo commented Feb 5, 2025 •

edited

Loading