feat: Type storage placeholders and support for templated maps #1074

igamigo · 2025-01-16T15:53:53Z

This PR introduces PlaceholderType so that users can get a typed template keys when querying for storage values requirements in component templates. It also provides support for templated maps.
Also renames WordRepresentation::Hexadecimal to WordRepresentation::Value, and the word submodule into value_type

…lygonMiden/miden-base into igamigo-typed-template-keys

PhilippGackstatter

Looks good to me! It's nice that placeholder types can be inferred rather than having to be specified by users directly.

I think my main comment is that StorageMap::with_entries does not care about duplicates right now and so a template with duplicate keys does not return an error. I think we should address that, in that constructor generally, but for this PR we could just address it by inserting the keys ourselves and erroring on duplicates. If you agree this should be addressed generally, we can make a follow-up issue.

The other (totally optional) comment I have is that InitStorageData as a name for the "placeholder replacements" doesn't quite click for me. To me the important aspect of this type is that it contains the replacements for the placeholders rather than that it is used for storage initialization. So I wonder if there's a better name? The typical terms used for something like this are interpolation, substitution, replacement or resolution. I think replacement is the most intuitive to understand of these and the least technical. So what do you think about just PlaceholderReplacements (not amazing either, but can't come up with something better)? But again, totally optional.

PhilippGackstatter · 2025-01-17T08:24:44Z

objects/src/accounts/component/template/mod.rs

+        // Check that placeholders do not appear more than once with a different type
+        let mut placeholders = BTreeMap::new();


As you mentioned in the last call, this should be added to the docs on validate.

Should we also describe the guarantees of this type on the AccountComponentMetadata docs (i.e. the ones mentioned on validate), maybe in a # Guarantees or # Constraints section?

It would also be nice to mention that types are checked and they are inferred, so users don't have to specify them (which is what I was initially assuming). As a user, I think it's helpful to know what the guarantees are and what I can be rely upon, and what I have to check myself (like if I wanted a boolean I would have to ensure for myself that the Felt I'm using is 0 or 1).

PhilippGackstatter · 2025-01-17T08:32:28Z

objects/src/accounts/component/template/mod.rs

@@ -188,14 +188,14 @@ impl AccountComponentMetadata {
    /// be used for initializing storage slot values, or storage map entries.
    /// For a full example on how a placeholder may be utilized, refer to the docs
    /// for [AccountComponentMetadata].
-    pub fn get_storage_placeholders(&self) -> BTreeSet<StoragePlaceholder> {
-        let mut placeholder_set = BTreeSet::new();
+    pub fn get_storage_placeholders(&self) -> BTreeMap<StoragePlaceholder, PlaceholderType> {


This can't return an iterator like self.storage_entries().iter().flat_map(StorageEntry::placeholders) because that wouldn't deduplicate the entries and that is the goal of this function, correct?

I think we should add the word unique in the docs to say that this returns a map of unique placeholders, even if they appear more than once in the template. It's kind of obvious if you think about it, since the return type is a map, but spelling this guarantee out doesn't hurt I think. Or put another way, I think it'd be good to understand that the job of this function is deduplication.

The docs should be updated to say "map" rather than "set" and "StoragePlaceholder" rather than "string", I think?

This can't return an iterator like self.storage_entries().iter().flat_map(StorageEntry::placeholders) because that wouldn't deduplicate the entries and that is the goal of this function, correct?

Correct, and agreed on your other points. I fixed up the docs a little bit.

PhilippGackstatter · 2025-01-17T08:36:41Z

objects/src/accounts/component/template/mod.rs

+            for (placeholder, placeholder_type) in storage_entry.placeholders() {
+                placeholder_map.insert(placeholder.clone(), placeholder_type);


Suggested change

for (placeholder, placeholder_type) in storage_entry.placeholders() {

placeholder_map.insert(placeholder.clone(), placeholder_type);

for (placeholder, placeholder_type) in storage_entry.placeholders() {

// The constructors of this type guarantee each placeholder has

// the same type, so reinserting them multiple times is fine.

placeholder_map.insert(placeholder.clone(), placeholder_type);

Nit: It would be good to mention this.

PhilippGackstatter · 2025-01-17T08:39:42Z

objects/src/accounts/component/template/storage/mod.rs

-    pub fn placeholders(&self) -> Box<dyn Iterator<Item = &StoragePlaceholder> + '_> {
+    /// Returns an iterator over all of the storage entries's placeholder keys, alongside their
+    /// expected type.
+    pub fn placeholders(


Should we align the name of this function with get_storage_placeholders?
Maybe all_placeholders_iter for this and unique_placeholders_map for the get_storage_placeholders, or something along those lines? Just to indicate that this function might return the same placeholder multiple times ("all of them") and the other only the unique set/map?
(Edit: I realize these are on different types, for some reason I thought they were on the same type. But maybe it would still be better if change?)

I think the convention here is to use get_ for non-trivial getters, where some work is done (I forget where I read this, I think it was maybe in conversation with Bobbin). This is why the only one with that prefix is the one from AccountComponentMetadata, which coalesces keys into a new map.
I changed the iter functions as suggested and added the _unique_ section to the metadata one.

PhilippGackstatter · 2025-01-17T09:23:39Z

objects/src/errors.rs

+    #[error(
+        "storage placeholder `{0}` appears more than once, representing different types of values"
+    )]
+    StoragePlaceholderDuplicate(StoragePlaceholder),


I think I'd not call this a "duplicate" but rather a "type mismatch". Duplicate implies that it's not allowed to have a placeholder more than once, but that's fine. It's only are problem if the same placeholder has a different type.

So maybe StoragePlaceholderTypeMismatch? Perhaps the error can take StoragePlaceholder and the two PlaceholderTypes that appeared, to produce a message like "storage placeholder {placeholder} appears multiple times but with different types {previous_type} and {current_type}"?

PhilippGackstatter · 2025-01-17T09:25:23Z

objects/src/accounts/component/template/storage/mod.rs

@@ -542,4 +541,32 @@ mod tests {
            ))
        );
    }
+
+    #[test]
+    pub fn fail_duplicate_key() {


Suggested change

pub fn fail_duplicate_key() {

pub fn fail_placeholder_type_mismatch() {

PhilippGackstatter · 2025-01-17T09:34:21Z

objects/src/accounts/component/template/storage/mod.rs

@@ -542,4 +541,32 @@ mod tests {
            ))
        );
    }
+
+    #[test]
+    pub fn fail_duplicate_key() {


I think we should have a test called duplicate key though, because this test does not pass, but should probably produce an error about a map key being used twice?

#[test] pub fn fail_duplicate_key() { let toml_text = r#" name = "Test Component" description = "This is a test component" version = "1.0.1" targets = ["FungibleFaucet"] [[storage]] name = "map" description = "A storage map entry" slot = 0 values = [ { key = "0x1", value = ["{{value.test}}", "0x1", "0x2", "0x3"] }, { key = "0x1", value = ["0x1", "0x2", "0x3", "{{value.test}}"] }, ] "#; // TODO: Replace with assertion for expected error. AccountComponentMetadata::from_toml(toml_text).unwrap_err(); }

We may also want a variant of this test with the second key as key = "{{map.key}}" which resolves to "0x1", so it also produces a duplicate, just after the placeholder replacement.

To fix this we could replace StorageMap::with_entries in try_build_map with our own version of StorageMap::with_entries which does not ignore duplicates.

Even better would be to make StorageMap::with_entries return an error on duplicates, like Smt::with_entries does, but we could defer to another PR since this requires updating some other call sites unrelated to this PR.

So my suggestions would be to make an inline copy of StorageMap::with_entries which checks for duplicates and returns an error.

Good point! I added validations to this and a variation of your test with a few more assertions

objects/src/accounts/component/template/storage/placeholder.rs

igamigo · 2025-01-17T17:52:47Z

The other (totally optional) comment I have is that InitStorageData as a name for the "placeholder replacements" doesn't quite click for me. To me the important aspect of this type is that it contains the replacements for the placeholders rather than that it is used for storage initialization. So I wonder if there's a better name? The typical terms used for something like this are interpolation, substitution, replacement or resolution. I think replacement is the most intuitive to understand of these and the least technical. So what do you think about just PlaceholderReplacements (not amazing either, but can't come up with something better)? But again, totally optional.

Yeah, I think keeping the "placeholder" in the name might be better. I think PlaceholderReplacements or PlaceholderValues could be decent replacements, although I don't mind InitStorageData either. cc @bobbinth

…lygonMiden/miden-base into igamigo-typed-template-keys

PhilippGackstatter

Looks good to me! Thanks for addressing the feedback.

Left a few more small comments but these are mostly optional.

PhilippGackstatter · 2025-01-20T07:28:41Z

objects/src/accounts/component/template/mod.rs

@@ -228,6 +240,7 @@ impl AccountComponentMetadata {
    /// # Errors
    ///
    /// - If the specified storage slots contain duplicates.
+    /// - If the template contains multiple storage placeholders with  of different type.


Suggested change

/// - If the template contains multiple storage placeholders with of different type.

/// - If the template contains multiple storage placeholders of different type.

PhilippGackstatter · 2025-01-20T07:57:28Z

objects/src/accounts/component/template/storage/value_type.rs

+                f.write_str(&format!("{}, ", array[0]))?;
+                f.write_str(&format!("{}, ", array[1]))?;
+                f.write_str(&format!("{}, ", array[2]))?;
+                f.write_str(&format!("{}, ", array[3]))?;


Nit: We can avoid the four allocations here by using write_fmt with format_args!, e.g.

f.write_fmt(format_args!("{}, ", array[0]))?;

PhilippGackstatter · 2025-01-20T08:13:50Z

objects/src/accounts/component/template/mod.rs

+    /// Retrieves a map of unique storage placeholders mapped to their expected type that require
+    /// a value at the moment of component instantiation. These values will be used for
+    /// initializing storage slot values, or storage map entries.For a full example on how a
+    /// placeholder may be utilized, please refer to the docs for [AccountComponentMetadata].


Suggested change

/// Retrieves a map of unique storage placeholders mapped to their expected type that require

/// a value at the moment of component instantiation. These values will be used for

/// initializing storage slot values, or storage map entries.For a full example on how a

/// placeholder may be utilized, please refer to the docs for [AccountComponentMetadata].

/// Retrieves a map of unique storage placeholders mapped to their expected type that require

/// a value at the moment of component instantiation. These values will be used for

/// initializing storage slot values, or storage map entries. For a full example on how a

/// placeholder may be utilized, please refer to the docs for [AccountComponentMetadata].

Nit: Insert space.

PhilippGackstatter · 2025-01-20T08:16:29Z

objects/src/accounts/component/template/mod.rs

@@ -92,6 +92,14 @@ impl Deserializable for AccountComponentTemplate {
 /// When the `std` feature is enabled, this struct allows for serialization and deserialization to
 /// and from a TOML file.
 ///
+/// # Guarantees


Maybe before the Guarantees section we should add one or two sentences explaining that the template is typed and that types are inferred (with a link to PlaceholderType).

PhilippGackstatter · 2025-01-20T08:24:03Z

objects/src/accounts/component/template/mod.rs

+        Ok(())
+    }
+
+    /// Validates map keys.


Suggested change

/// Validates map keys.

/// Validates map keys by checking for duplicates.

///

/// This can only check keys given as immediate values (i.e. not placeholders) since placeholders are replaced at a later step at which point another duplicate check is done.

PhilippGackstatter · 2025-01-20T08:27:48Z

objects/src/accounts/component/template/storage/value_type.rs

+            FeltRepresentation::Hexadecimal(base_element) => f.write_str(&base_element.to_string()),
+            FeltRepresentation::Decimal(base_element) => f.write_str(&base_element.to_string()),
+            FeltRepresentation::Template(storage_placeholder) => {
+                f.write_str(&storage_placeholder.to_string())
+            },


Nit: We can avoid allocations here as well, e.g. with f.write_fmt(format_args!("{}", base_element)).

PhilippGackstatter · 2025-01-20T08:30:09Z

objects/src/accounts/component/template/storage/value_type.rs

+            WordRepresentation::Template(storage_placeholder) => {
+                f.write_str(&storage_placeholder.to_string())


Suggested change

WordRepresentation::Template(storage_placeholder) => {

f.write_str(&storage_placeholder.to_string())

WordRepresentation::Template(storage_placeholder) => {

f.write_fmt(format_args!("{}", storage_placeholder))

feat: Type storage placeholders and support for templated maps

10b5281

igamigo force-pushed the igamigo-typed-template-keys branch from 73cdf8a to 10b5281 Compare January 16, 2025 17:56

igamigo marked this pull request as ready for review January 16, 2025 17:56

Merge branch 'next' into igamigo-typed-template-keys

5e7ca4e

igamigo requested a review from PhilippGackstatter January 16, 2025 19:57

igamigo added 3 commits January 16, 2025 19:17

fix: Expose placeholder type

a608ea6

Merge branch 'igamigo-typed-template-keys' of https://github.com/0xPo…

cecbfff

…lygonMiden/miden-base into igamigo-typed-template-keys

refactor: Expose MapRepresentation

7bd48d5

PhilippGackstatter reviewed Jan 17, 2025

View reviewed changes

bobbinth reviewed Jan 17, 2025

View reviewed changes

objects/src/accounts/component/template/storage/placeholder.rs Show resolved Hide resolved

reviews: Doc updates, validation for map

0a30796

chore: Merge

5b24ba5

igamigo force-pushed the igamigo-typed-template-keys branch from 3bee78a to 5b24ba5 Compare January 17, 2025 18:12

igamigo added 6 commits January 17, 2025 15:13

chore: Merge fixes

c554886

chore: Merge fixes

4d5a7d8

Merge branch 'next' into igamigo-typed-template-keys

6685173

refactor: change inner type of StorageValue::Map

b230aeb

Merge branch 'igamigo-typed-template-keys' of https://github.com/0xPo…

e98158f

…lygonMiden/miden-base into igamigo-typed-template-keys

docs: Change description

07c5e27

PhilippGackstatter approved these changes Jan 20, 2025

View reviewed changes

reviews: Docs and avoid allocation when formatting

b7672e4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Type storage placeholders and support for templated maps #1074

feat: Type storage placeholders and support for templated maps #1074

igamigo commented Jan 16, 2025 •

edited

Loading

PhilippGackstatter left a comment

PhilippGackstatter Jan 17, 2025

PhilippGackstatter Jan 17, 2025

igamigo Jan 17, 2025 •

edited

Loading

PhilippGackstatter Jan 17, 2025

PhilippGackstatter Jan 17, 2025 •

edited

Loading

igamigo Jan 17, 2025

PhilippGackstatter Jan 17, 2025

igamigo Jan 17, 2025

PhilippGackstatter Jan 17, 2025

PhilippGackstatter Jan 17, 2025

igamigo Jan 17, 2025

igamigo commented Jan 17, 2025 •

edited

Loading

PhilippGackstatter left a comment

PhilippGackstatter Jan 20, 2025

PhilippGackstatter Jan 20, 2025

PhilippGackstatter Jan 20, 2025

PhilippGackstatter Jan 20, 2025

PhilippGackstatter Jan 20, 2025

PhilippGackstatter Jan 20, 2025

PhilippGackstatter Jan 20, 2025

		// Check that placeholders do not appear more than once with a different type
		let mut placeholders = BTreeMap::new();

		for (placeholder, placeholder_type) in storage_entry.placeholders() {
		placeholder_map.insert(placeholder.clone(), placeholder_type);

	pub fn fail_duplicate_key() {
	pub fn fail_placeholder_type_mismatch() {

	/// - If the template contains multiple storage placeholders with of different type.
	/// - If the template contains multiple storage placeholders of different type.

-    /// Validates map keys.
+    /// Validates map keys by checking for duplicates.
+    ///
+    /// This can only check keys given as immediate values (i.e. not placeholders) since placeholders are replaced at a later step at which point another duplicate check is done.

		WordRepresentation::Template(storage_placeholder) => {
		f.write_str(&storage_placeholder.to_string())

feat: Type storage placeholders and support for templated maps #1074

Are you sure you want to change the base?

feat: Type storage placeholders and support for templated maps #1074

Conversation

igamigo commented Jan 16, 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

igamigo Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter Jan 17, 2025 • edited Loading

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 Jan 17, 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

igamigo commented Jan 16, 2025 •

edited

Loading

igamigo Jan 17, 2025 •

edited

Loading

PhilippGackstatter Jan 17, 2025 •

edited

Loading

igamigo commented Jan 17, 2025 •

edited

Loading