feat: `TemplateType` newtype, docs, tests, `InitStorageData::from_toml` #1170

igamigo · 2025-02-19T22:14:59Z

igamigo · 2025-02-19T22:16:11Z

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

+impl Serializable for TemplateType {
+    fn write_into<W: ByteWriter>(&self, target: &mut W) {
+        target.write(self.0.clone())
+    }
+}
+
+impl Deserializable for TemplateType {
+    fn read_from<R: ByteReader>(source: &mut R) -> Result<Self, DeserializationError> {
+        let id: String = source.read()?;
+        Ok(TemplateType::new(id))
+    }
 }


Unfortunately could not make TemplateType contain an inner 'static &str due to serialization, unless we wanted deserialization to leak memory

I guess it would be possible with an inner representation of Cow<'static, str>? But not essential I think.

igamigo · 2025-02-19T22:17:37Z

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

        /// An optional description for the slot, explaining its purpose.
        description: Option<String>,


name and description could be made into their own type since they always appear together, would slightly simplify serde code

Would this be something like?:

pub struct StorageEntryIdent { name: StorageValueName, description: Option<String>, }

I think this could be a good idea, but I'd probably do this in a separate PR.

igamigo · 2025-02-19T22:19:56Z

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

@@ -64,7 +65,7 @@ pub enum StorageEntry {
        /// An optional description for the slot, explaining its purpose.
        description: Option<String>,
        /// The indices of the slots that form this multi-slot entry.
-        slots: Vec<u8>,
+        slots: Range<u8>,


One other change that should be made for Multislot entries is to factor out the actual data into a MultislotRepresentation as done with the other two variants. This would make StorageEntry variants more streamlined (but also we would need a MultislotRepresentation::Template for when templating in multislot values is enabled)

Similar to the above comment: seems like a good idea but I'd do it in a follow-up PR (let's create an issue for follow-ups).

…nto igamigo-template-followups

bobbinth

Looks good! Thank you! Not a very deep review, but I left some small comments inline.

bobbinth · 2025-02-22T10:02:39Z

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

+    pub fn new(s: impl Into<String>) -> Self {
+        Self(s.into())
+    }


Do we want to impose any restrictions on what characters are allowed etc.?

Added some validations

bobbinth · 2025-02-22T10:04:09Z

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

-    fn type_name() -> &'static str {
-        "word"
+    fn type_name() -> TemplateType {
+        TemplateType::new("word")


Should we use default_word_type() here?

bobbinth · 2025-02-22T10:14:01Z

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

+            if let Some(name) = entry.name() {
+                let name_existed = !seen_names.insert(name.as_str());
+                if name_existed {
+                    return Err(AccountComponentTemplateError::DuplicateEntryNames(
+                        name.as_str().into(),
+                    ));
+                }
+            }


We should mention this error condition in the doc comments for this function.

Updated validations and explanations

bobbinth · 2025-02-22T10:16:36Z

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

    /// Creates a new instance of [InitStorageData].
+    /// A [`BTreeMap`] is constructed from the passed iterator, so duplicate keys will cause


nit: I'd add a newline between these lines.

bobbinth · 2025-02-22T10:19:07Z

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

+#[cfg(test)]
+mod tests {


Should this test actually go into the toml module? Originally, I was trying to find them there since that's where the implementation of the function they are testing is in.

Also, these tests should be done only in the std context, right?

Moved this to the toml module (which is based on std)

bobbinth · 2025-02-22T10:27:35Z

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

        /// An optional description for the slot, explaining its purpose.
        description: Option<String>,


Would this be something like?:

pub struct StorageEntryIdent { name: StorageValueName, description: Option<String>, }

I think this could be a good idea, but I'd probably do this in a separate PR.

bobbinth · 2025-02-22T10:28:06Z

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

@@ -64,7 +65,7 @@ pub enum StorageEntry {
        /// An optional description for the slot, explaining its purpose.
        description: Option<String>,
        /// The indices of the slots that form this multi-slot entry.
-        slots: Vec<u8>,
+        slots: Range<u8>,


Similar to the above comment: seems like a good idea but I'd do it in a follow-up PR (let's create an issue for follow-ups).

PhilippGackstatter

Looks good to me!

I left some comments inline. The main question is whether we allow and want to allow multi slot entries of size 1.

PhilippGackstatter · 2025-02-25T07:30:00Z

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

-            storage_placeholders: entries.into_iter().collect(),
+            storage_placeholders: entries
+                .into_iter()
+                .filter(|(k, _)| !k.as_str().is_empty())


Suggested change

.filter(|(k, _)| !k.as_str().is_empty())

.filter(|(entry_name, _)| !entry_name.as_str().is_empty())

Nit: Readability.

PhilippGackstatter · 2025-02-25T07:32:10Z

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

@@ -434,13 +430,14 @@ mod tests {
            storage,
        };
        let toml = config.as_toml().unwrap();
+        println!("{toml}");


Probably leftover?

PhilippGackstatter · 2025-02-25T07:40:03Z

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

+    /// Returns the default felt type,  assigned to entries with no type.
+    pub fn default_felt_type() -> TemplateType {
+        TemplateType::new("felt").expect("type is well formed")
+    }
+
+    /// Returns the default word type, assigned to entries with no type.
+    pub fn default_word_type() -> TemplateType {
+        TemplateType::new("word").expect("type is well formed")
+    }


Could we name these TemplateType::native_felt() and TemplateType::native_word() or would that be ambiguous somehow? I feel like that would better capture that these are the base types, rather than just some default.

Yeah. The *_type suffix is redundant because these are now type functions, and the default_* prefix, while descriptive of their use within the templates, is probably not too appropriate as part of type functions either, so I'll resort to your alternative.

PhilippGackstatter · 2025-02-25T07:42:57Z

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

+        let native: u16 = input
+            .parse()
+            .map_err(|_| TemplateTypeError::ParseError(input.to_string(), Self::type_name()))?;


It would be nice if we could include the source errors in the ParseError. Should we add a source: Box<dyn Error + Send + Sync + 'static> field maybe?

It seems like this error variant would also be used by user implementations (eventually) and they could have more complex source errors which would be great to include to help with debugging.

We could add convenience constructors similar to these:

miden-base/crates/miden-tx/src/errors/mod.rs

Lines 62 to 80 in c5d6275

/// Creates a custom error using the [`TransactionProverError::Other`] variant from an error

/// message.

pub fn other(message: impl Into<String>) -> Self {

let message: String = message.into();

Self::Other { error_msg: message.into(), source: None }

}

/// Creates a custom error using the [`TransactionProverError::Other`] variant from an error

/// message and a source error.

pub fn other_with_source(

message: impl Into<String>,

source: impl Error + Send + Sync + 'static,

) -> Self {

let message: String = message.into();

Self::Other {

error_msg: message.into(),

source: Some(Box::new(source)),

}

}

PhilippGackstatter · 2025-02-25T07:55:07Z

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

+            for pair in slots.windows(2) {
+                if pair[1] != pair[0] + 1 {
+                    return Err(serde::de::Error::custom(format!(
+                        "`slots` field in `{name}` entry is not a valid range"


Wouldn't it be more accurate to say that the entries are not contiguous? 0, 2 would be a valid range in the 0..2 sense, but wouldn't be contiguous in the 0, 2 sense.

PhilippGackstatter · 2025-02-25T07:58:08Z

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

+            let start = slots[0];
+            let end = slots.last().expect("checked validity") + 1;
+
+            Ok(StorageEntry::new_multislot(name, raw.description, start..end, values))


I think we currently allow multi slot entries of size 1. Should we require somewhere (here?) that at least two entries exist? If that's the case, I guess a follow-up PR makes the most sense.

I tried parsing this into an AccountComponentMetadata and it worked:

name = "Test Component" description = "Test multislot non contiguous" version = "1.0.1" supported-types = ["FungibleFaucet"] [[storage]] name = "multislot_non_contiguous" slots = [0] values = [ [ "0x1", "0x2", "0x3", "0x4" ], ]

Is that intentional? I'm not saying this is wrong, but if this is allowed, there is some overlap between this and a regular non-multi entry and we may want to support just one way of doing this instead of two, to reduce the "API surface" of these features and what we have to maintain long-term.

Yeah, I guess I was considering these unfinished until we implemented types, etc. but this makes sense to add right now

Ah I see, feel free to do in a follow-up PR then.

PhilippGackstatter · 2025-02-25T07:59:21Z

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

+        let value: toml::Value = toml::from_str(toml_str)?;
+        let mut placeholders = BTreeMap::new();
+        // Start with an empty prefix (i.e. the default, which is an empty string)
+        Self::flatten_parse_toml_value(StorageValueName::default(), &value, &mut placeholders)?;


Nit: For readability, maybe it would be clearer if we would use StorageValueName::empty() instead of default()? (Still fine to have default).

Agreed. I'll also remove default()

PhilippGackstatter · 2025-02-25T08:09:18Z

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

+    #[error("failed to parse TOML: {0}")]
+    InvalidToml(#[from] toml::de::Error),


#[from] implies #[source] so this both includes the error in the display impl as well as return it from source. It should only do one of those two. For simplicity, you could just remove the : {0}.

PhilippGackstatter · 2025-02-25T08:12:30Z

crates/miden-objects/src/errors.rs

    #[error("error deserializing component metadata: {0}")]
    MetadataDeserializationError(String),
+    #[error("multi-slot entry should contain as many values as storage slots indices")]


Suggested change

#[error("multi-slot entry should contain as many values as storage slots indices")]

#[error("multi-slot entry should contain as many values as storage slot indices")]

PhilippGackstatter · 2025-02-25T08:26:23Z

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

+/// let last_element = FeltRepresentation::new_template(
+///     TemplateType::new("felt")?,
+///     StorageValueName::new("foo")?,
+///     None,
+/// );


I wonder if it would be a nicer API if new_{template, value} would not take the optional description and we instead had a with_description(mut self, description: impl Into<String>) -> Self method? Then these could be chained together nicely to set the description .

In terms of API I agree, although it will make parsing a little bit more verbose. Probably a net win though.

…ce; constructors/propery-setters

…nto igamigo-template-followups

igamigo commented Feb 19, 2025

View reviewed changes

feat: TemplateType newtype, docs, tests, InitStorageData::from_toml

1cf5186

igamigo force-pushed the igamigo-template-followups branch from fa017e8 to 1cf5186 Compare February 20, 2025 20:03

igamigo marked this pull request as ready for review February 20, 2025 20:03

igamigo requested review from PhilippGackstatter and bobbinth February 20, 2025 20:03

Merge branch 'next' of https://github.com/0xPolygonMiden/miden-base i…

a77ce7c

…nto igamigo-template-followups

bobbinth approved these changes Feb 22, 2025

View reviewed changes

igamigo added 4 commits February 24, 2025 17:36

reviews: Use default name

aae29bc

reviews: Validate type names

c254f89

fix: Typo

09340d4

fix: Doc test

9dca0b4

PhilippGackstatter approved these changes Feb 25, 2025

View reviewed changes

PhilippGackstatter reviewed Feb 25, 2025

View reviewed changes

igamigo added 5 commits February 25, 2025 13:34

reviews: Multislot spans one slot errors; parsing error includes sour…

f947af0

…ce; constructors/propery-setters

fix: Test

b46d349

chore: Clippy

4620535

Merge branch 'next' of https://github.com/0xPolygonMiden/miden-base i…

fdeacdf

…nto igamigo-template-followups

Merge branch 'next' into igamigo-template-followups

6994003

igamigo requested review from PhilippGackstatter and bobbinth March 5, 2025 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `TemplateType` newtype, docs, tests, `InitStorageData::from_toml` #1170

feat: `TemplateType` newtype, docs, tests, `InitStorageData::from_toml` #1170

igamigo commented Feb 19, 2025

igamigo Feb 19, 2025

PhilippGackstatter Feb 25, 2025

igamigo Feb 19, 2025

bobbinth Feb 22, 2025

igamigo Feb 19, 2025

bobbinth Feb 22, 2025

bobbinth left a comment

bobbinth Feb 22, 2025

igamigo Feb 24, 2025

bobbinth Feb 22, 2025

igamigo Feb 24, 2025

bobbinth Feb 22, 2025

igamigo Feb 24, 2025

bobbinth Feb 22, 2025

bobbinth Feb 22, 2025

igamigo Feb 24, 2025

bobbinth Feb 22, 2025

bobbinth Feb 22, 2025

PhilippGackstatter left a comment

PhilippGackstatter Feb 25, 2025

PhilippGackstatter Feb 25, 2025

PhilippGackstatter Feb 25, 2025

igamigo Feb 25, 2025

PhilippGackstatter Feb 25, 2025

PhilippGackstatter Feb 25, 2025

PhilippGackstatter Feb 25, 2025

igamigo Feb 25, 2025

PhilippGackstatter Feb 25, 2025

PhilippGackstatter Feb 25, 2025

igamigo Feb 25, 2025

PhilippGackstatter Feb 25, 2025

PhilippGackstatter Feb 25, 2025

PhilippGackstatter Feb 25, 2025 •

edited

Loading

igamigo Feb 25, 2025

		/// An optional description for the slot, explaining its purpose.
		description: Option<String>,

		/// Creates a new instance of [InitStorageData].
		/// A [`BTreeMap`] is constructed from the passed iterator, so duplicate keys will cause

	.filter(\|(k, _)\| !k.as_str().is_empty())
	.filter(\|(entry_name, _)\| !entry_name.as_str().is_empty())

	/// Creates a custom error using the [`TransactionProverError::Other`] variant from an error
	/// message.
	pub fn other(message: impl Into<String>) -> Self {
	let message: String = message.into();
	Self::Other { error_msg: message.into(), source: None }
	}

	/// Creates a custom error using the [`TransactionProverError::Other`] variant from an error
	/// message and a source error.
	pub fn other_with_source(
	message: impl Into<String>,
	source: impl Error + Send + Sync + 'static,
	) -> Self {
	let message: String = message.into();
	Self::Other {
	error_msg: message.into(),
	source: Some(Box::new(source)),
	}
	}

		#[error("failed to parse TOML: {0}")]
		InvalidToml(#[from] toml::de::Error),

	#[error("multi-slot entry should contain as many values as storage slots indices")]
	#[error("multi-slot entry should contain as many values as storage slot indices")]

feat: TemplateType newtype, docs, tests, InitStorageData::from_toml #1170

Are you sure you want to change the base?

feat: TemplateType newtype, docs, tests, InitStorageData::from_toml #1170

Conversation

igamigo commented Feb 19, 2025

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

bobbinth 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: `TemplateType` newtype, docs, tests, `InitStorageData::from_toml` #1170

feat: `TemplateType` newtype, docs, tests, `InitStorageData::from_toml` #1170

PhilippGackstatter Feb 25, 2025 •

edited

Loading