feat: Introduce `AccountComponentTemplate` #1015

igamigo · 2024-12-11T21:47:36Z

Part of #550.
Introduces a first draft of AccountComponentPackage and AccountComponentTemplate.

For now it can serialize and deserialize configs such as this one:

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

[[storage]]
name = "slot0"
description = "First slot"
slot = 0
value = "0x2551340000000000af02000000000000a3db000000000000ffeeff0f00000000"

[[storage]]
name = "multi"
description = "Multi slot entry"
slots = [1, 2, 3]
values = [["3428645", "687", "0xdba3", "0xfffeeff"], "0x2551340000000000af02000000000000a3db000000000000ffeeff0f00000000", "0x2551340000000000af02000000000000a3db000000000000ffeeff0f00000000"]

[[storage]]
name = "map"
slot = [4]
values = [{key="0x1", value="0x2"}]

Most structs down to the element need to have custom (de)serialization due to the different alternatives discussed in #550.

bobbinth

Looks good! Thank you! I left just a few comments inline.

objects/src/accounts/package/storage_entry/mod.rs

objects/src/accounts/package/mod.rs

objects/src/lib.rs

tomyrd · 2024-12-26T17:28:38Z

objects/src/accounts/package/storage_entry/mod.rs

+        if slots.len() != values.len() {
+            return Err(ComponentMetadataError::InvalidMultiSlotEntry);
+        }
+
+        Ok(StorageEntry::MultiSlot {
+            name: name.into(),
+            description: description.map(Into::<String>::into),
+            slots,
+            values: values.into_iter().map(Into::into).collect(),
+        })


The description above says:

A multi-slot entry (spanning multiple contiguous slots, with multipe values).

Should we check that the slots are contiguous? (I know we also check it at the ComponentPackage level.

Added this change

tomyrd · 2024-12-26T17:49:16Z

objects/src/accounts/package/storage_entry/mod.rs

+                if !slot_present {
+                    return Err(D::Error::missing_field("slot"));
+                }
+                if raw.value.is_some() {


nit: could we use value_present here?

tomyrd · 2024-12-26T18:00:09Z

objects/src/accounts/package/storage_entry/mod.rs

+                if slot_present {
+                    return Err(D::Error::custom(
+                        "Fields 'slot' and 'slots' are mutually exclusive.",
+                    ));
+                }
+                if value_present {
+                    return Err(D::Error::custom(
+                        "Fields 'value' and 'values' are mutually exclusive.",
+                    ));
+                }


These checks for mutual exclusivity can be done before the match to reduce code repetition

tomyrd · 2024-12-26T18:00:40Z

objects/src/accounts/package/storage_entry/mod.rs

+                    slot: raw.slot.expect("was checked to be present"),
+                    value: raw.value.expect("was checked to be present"),


instead of the if + expect we could do .ok_or(...)? here

Changed! I ended up refactoring this section of code many times and left a mix of solutions, but these changes are helping shape it up better

tomyrd · 2024-12-26T18:10:42Z

objects/src/accounts/package/storage_entry/mod.rs

+                let slots = raw.slots.ok_or_else(|| D::Error::missing_field("slots"))?;
+                let values = raw.values.ok_or_else(|| D::Error::missing_field("values"))?;


If we change the match to be match (raw.slots, raw.values) we could deconstruct in the case to (Some(slots), Some(values)) and remove these ok_or_else.

tomyrd · 2024-12-26T18:30:56Z

objects/src/accounts/package/storage_entry/word.rs

+impl Default for WordRepresentation {
+    fn default() -> Self {
+        WordRepresentation::SingleHex(Default::default())
+    }
+}


Should this be in the previous section?

tomyrd · 2024-12-26T18:39:09Z

objects/src/lib.rs

+        }
+        // Enforce and skip the '0x' prefix.
+        let hex_bytes = match hex.as_bytes() {
+            [b'0', b'x', rest @ ..] => rest,


Should we allow for "0X" prefix like we do for Felts?

I think I will disallow 0X in felts instead, unless there's a good reason to do otherwise. cc @bobbinth @PhilippGackstatter (and probably @Fumuran) do you think 0X should be a valid prefixes for things like the digest!() macro?

If it doesn't complicate things too much, i think making 0x prefix optional could be a good idea. For example, the following would result in an identical digest:

digest!("123456"); digest!("0x123456");

But this is a pretty minor thing - so, I'm fine with keeping as is as well.

digest!("123456"); digest!("0x123456");

Not sure I'm following the context completely, but isn't it confusing to have these two be the same? Unless you know that digest! takes a hex string, it would be easy to interpet digest!("123456"); as a decimal number, especially since macros are even less predictable than regular Rust functions. So wouldn't it be better to enforce hex strings to start with 0x to avoid ambiguity?

Ah - good point! Scratch my comment then - let's enforce 0x prefix for hex values.

tomyrd · 2024-12-26T18:41:31Z

objects/src/lib.rs

-                // SAFETY: u8 cast to u64 is safe. We cannot use u64::from in const context so we
-                // are forced to cast.


We should add this safety comment back to the match case

Good catch, re-added

objects/src/accounts/package/mod.rs

objects/src/accounts/component/mod.rs

objects/src/accounts/package/mod.rs

objects/src/accounts/package/storage_entry/mod.rs

PhilippGackstatter

Looks good to me conceptually!

There are quite a few pub types that are not documented which would be great to add.

I'm also wondering if making everything pub by default is good. There are a lot of new types added in this PR and the API surface of miden-base increases quite a bit. If we only export some of the higher level types, it would be great to double-check if the lower level types are properly excluded from the docs and that they are not exposed through methods on the higher level types (although rustdoc should complain in that case anyway).

Otherwise, I left many small nits, mostly on errors and docs. Feel free to pick what you think makes sense!

PhilippGackstatter · 2025-01-03T09:37:15Z

objects/src/accounts/package/mod.rs

+mod storage_entry;
+pub use storage_entry::{StorageEntry, TemplateKey, TemplateValue};
+
+// COMPONENT PACKAGE


Suggested change

// COMPONENT PACKAGE

// ACCOUNT COMPONENT TEMPLATE

I guess the file name or module name should also be template then?

Changed this

PhilippGackstatter · 2025-01-03T09:37:40Z

objects/src/accounts/component/mod.rs

+    ///   they are not of a valid type)
+    #[cfg(feature = "std")]
+    pub fn from_template(
+        package: &AccountComponentTemplate,


Suggested change

package: &AccountComponentTemplate,

template: &AccountComponentTemplate,

Nit: Looks like this should be template or something else instead of package now.

The documentation might also need an update to reflect the new naming.

Thanks, changed this

PhilippGackstatter · 2025-01-03T09:39:20Z

objects/src/accounts/package/mod.rs

+/// a component within the system. It includes the configuration details and the compiled
+/// library code required for the component's operation.
+///
+/// A package can be instantiated into [AccountComponent](super::AccountComponent) objects.


Suggested change

/// A package can be instantiated into [AccountComponent](super::AccountComponent) objects.

/// A template can be instantiated into [AccountComponent](super::AccountComponent) objects.

PhilippGackstatter · 2025-01-03T09:40:18Z

objects/src/accounts/package/mod.rs

+    /// The component's metadata. This describes the component and how the storage is laid out,
+    /// alongside how storage values are initialized.
+    metadata: ComponentMetadata,
+    /// The account's previously-assembled code. This defines all functionality related to the


Suggested change

/// The account's previously-assembled code. This defines all functionality related to the

/// The account component's assembled code. This defines all functionality related to the

Nit: Maybe just this?

Sounds good! Changed

PhilippGackstatter · 2025-01-03T09:44:10Z

objects/src/accounts/package/mod.rs

+/// The component metadata can be defined with generic keys that can be replaced at instantiation
+/// time.


Nit: Here it's called "generic keys" while in other places it seems to be called "template keys". Ideally, we use just one term for this.

Reading this for the first time, my immediate question is how these template keys work, so it would be great to link to some place where more can be learned, or even add a more extensive example here (or as module docs).

PhilippGackstatter · 2025-01-03T12:19:28Z

objects/src/accounts/package/storage_entry/template.rs

+
+impl core::fmt::Display for TemplateKey {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "{}", self.key)


I'm wondering if this should include braces. The inverse operation is basically TryFrom<&str> and it would expect braces, so maybe this would be most consistent.

Yes, agreed, good catch. I originally did From<&str> instead of TryFrom<&str> which worked as the inverse of this, but then changed it based on Bobbin's suggestion and didn't realize this should've been changed as well.

PhilippGackstatter · 2025-01-03T12:22:23Z

objects/src/accounts/package/storage_entry/word.rs

+            WordRepresentation::Array(array) => {
+                let mut result = [Felt::ZERO; 4];
+                for (index, felt_repr) in array.iter().enumerate() {
+                    result[index] = felt_repr.clone().try_into_felt(template_values)?;
+                }
+                Ok(result)
+            },


Would be good to add a comment here that every index of result is guaranteed to be overwritten.

PhilippGackstatter · 2025-01-03T12:29:08Z

objects/src/accounts/package/storage_entry/word.rs

+        } else if let Ok(decimal_value) = value.parse::<u64>() {
+            Ok(FeltRepresentation::SingleDecimal(Felt::new(decimal_value)))


I don't think we would want to silently wrap around if the given u64 exceeds the field modulus. If users specify such a value it would always be an error I would think, so it would be better to use Felt::try_from and propagate the error.

Good catch, changed

PhilippGackstatter · 2025-01-03T12:30:33Z

objects/src/accounts/package/storage_entry/word.rs

+        } else if let Ok(key) = TemplateKey::try_from(&value) {
+            Ok(FeltRepresentation::Dynamic(key))
+        } else {
+            Err(serde::de::Error::custom("Value is not a valid element"))


Suggested change

Err(serde::de::Error::custom("Value is not a valid element"))

Err(serde::de::Error::custom("deserialized string value is not a valid variant of FeltRepresentation"))

Changed this

objects/src/accounts/package/storage_entry/word.rs

igamigo · 2025-01-03T14:36:58Z

I'm also wondering if making everything pub by default is good. There are a lot of new types added in this PR and the API surface of miden-base increases quite a bit. If we only export some of the higher level types, it would be great to double-check if the lower level types are properly excluded from the docs and that they are not exposed through methods on the higher level types (although rustdoc should complain in that case anyway).

Yeah, I left a TODO to address this because I wanted to double check what we would need and restrict everything from there, but definitely agree that it should not all be pub by default and was planning to change this.

bobbinth

Not a review - but quickly scanning through the code, I think there are still some smaller comments that need to be addressed. And we also still need to introduce something like InitStorageData struct.

Also, in doc comments we should update references to "package" vs. "template" (e.g., doc comments for AccountComponentTemplate struct).

igamigo · 2025-01-06T21:11:36Z

I think all comments have been addressed (but will double check just in case). The only remaining question from my side is whether we want TemplateKey to carry the expected key's type, which could be identified when deserialized based on the placement of the key.

bobbinth

Thank you! Looks good! I left some comments inline.

One other thing I was thinking about is naming of TemplateKey and TemplateValue. Maybe these are fine, but I also thought that maybe these should indicate that we are dealing specifically with storage. An alternative could be something like StoragePlaceholder and StorageValue. But this is not a strong opinion by any means.

objects/src/accounts/component/mod.rs

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

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

bobbinth · 2025-01-07T08:29:21Z

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

+    /// Returns an iterator over all of the storage entries's template keys.
+    // TODO: Should template keys be typed?
+    pub fn template_keys(&self) -> Box<dyn Iterator<Item = &TemplateKey> + '_> {


re TODO: do you mean to get rid of dyn? If so, I think having dyn here is probably fine.

What I meant is this, basically

Let's create an issue for this. it sounds like a good idea, but I'm not sure this will be possible in all circumstances.

Created #1051

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

…rrors

igamigo · 2025-01-07T18:42:00Z

I also changed TemplateKey into StoragePlaceholder and TemplateValue into StorageValue. I think the latter specifically sounds a bit generic but overall this might be better. Hopefully did not miss too many doc changes, will double check again soon.

bobbinth

Looks good! Thank you! I left a few comments inline - these are mostly nits or things we can do in subsequent PRs.

objects/src/accounts/account_id.rs

objects/src/accounts/storage/map.rs

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

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

PhilippGackstatter

Looks good to me! Just a couple of nits.

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

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

objects/src/errors.rs

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

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

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

objects/src/errors.rs

bobbinth

All looks good! Thank you!

igamigo force-pushed the igamigo-acc-pkg branch 2 times, most recently from 0cee24f to 123e92c Compare December 11, 2024 22:10

igamigo changed the title ~~feat: Introduce ComponentPackage~~ feat: Introduce ComponentPackage and ComponentConfig Dec 11, 2024

igamigo force-pushed the igamigo-acc-pkg branch from f12c492 to 1f66753 Compare December 12, 2024 21:39

igamigo changed the title ~~feat: Introduce ComponentPackage and ComponentConfig~~ feat: Introduce ComponentPackage and ComponentTemplate Dec 12, 2024

igamigo marked this pull request as ready for review December 12, 2024 21:55

bobbinth reviewed Dec 13, 2024

View reviewed changes

objects/src/accounts/package/storage_entry/mod.rs Outdated Show resolved Hide resolved

objects/src/accounts/package/storage_entry/mod.rs Outdated Show resolved Hide resolved

objects/src/accounts/package/mod.rs Outdated Show resolved Hide resolved

greenhat mentioned this pull request Dec 17, 2024

Implement the account storage API 0xPolygonMiden/compiler#340

Open

igamigo force-pushed the igamigo-acc-pkg branch 2 times, most recently from 266af8f to 34abed2 Compare December 17, 2024 20:34

feat: Introduce ComponentPackage and ComponentMetadata

9df6689

igamigo force-pushed the igamigo-acc-pkg branch from e5291d0 to 9df6689 Compare December 19, 2024 14:44

igamigo and others added 4 commits December 19, 2024 16:19

chore: Lints

975469f

refactor: New methods

a41bb4b

Merge branch 'next' into igamigo-acc-pkg

f23d572

fix build

cf05071

bobbinth reviewed Dec 26, 2024

View reviewed changes

objects/src/lib.rs Show resolved Hide resolved

tomyrd reviewed Dec 26, 2024

View reviewed changes

Merge branch 'next' into igamigo-acc-pkg

52adeb3

bobbinth reviewed Dec 29, 2024

View reviewed changes

objects/src/accounts/package/mod.rs Outdated Show resolved Hide resolved

bobbinth mentioned this pull request Dec 29, 2024

feat: ComponentMetadata templating #1027

Merged

feat: ComponentMetadata templating (#1027)

3095071

bobbinth reviewed Jan 2, 2025

View reviewed changes

objects/src/accounts/component/mod.rs Outdated Show resolved Hide resolved

objects/src/accounts/package/mod.rs Outdated Show resolved Hide resolved

objects/src/accounts/package/storage_entry/mod.rs Outdated Show resolved Hide resolved

igamigo added 5 commits January 2, 2025 12:40

reviews: Storage entry serde

02ab433

chore: CHANGELOG

5e3bbfc

chore: CHANGELOG

99b0352

reviews: Take reference on conversions

ed6e9aa

chore: update versions

ca3eaaf

igamigo changed the title ~~feat: Introduce ComponentPackage and ComponentTemplate~~ feat: Introduce AccountComponentTemplate Jan 2, 2025

PhilippGackstatter reviewed Jan 3, 2025

View reviewed changes

igamigo added 4 commits January 3, 2025 13:13

reviews: Error messages and docs

44bd140

reviews: Undo file

89af097

feat: Use binary serde for component template

1341b89

fix: Test

034be9f

bobbinth reviewed Jan 5, 2025

View reviewed changes

igamigo added 2 commits January 6, 2025 13:05

reviews: package->template, docs

761dff5

refactor: Move error enum

e9e40b2

igamigo added 3 commits January 6, 2025 18:19

chore: CHANGELOG

2ca12f2

chore: rebase

37dffd9

fix: docs

85815a8

bobbinth reviewed Jan 7, 2025

View reviewed changes

igamigo added 2 commits January 7, 2025 15:25

reviews: Rename TemplateKey and TemplateValue, create toml mod, fix e…

487efea

…rrors

fix: Tests

a21f683

igamigo requested review from bobbinth, tomyrd and PhilippGackstatter January 7, 2025 18:42

bobbinth approved these changes Jan 8, 2025

View reviewed changes

PhilippGackstatter approved these changes Jan 8, 2025

View reviewed changes

igamigo added 2 commits January 8, 2025 14:08

review: repr(u64) to repr(u8), docs and tests improvements

96e5517

Merge branch 'next' into igamigo-acc-pkg

61b2322

This was referenced Jan 8, 2025

Improvements to AccountComponentMetadata #1050

Open

Investigate adding type to StoragePlaceholder #1051

Closed

Follow-ups for component templates #1052

Open

bobbinth approved these changes Jan 9, 2025

View reviewed changes

bobbinth merged commit 206e247 into next Jan 9, 2025
9 checks passed

bobbinth deleted the igamigo-acc-pkg branch January 9, 2025 00:20

bobbinth mentioned this pull request Jan 9, 2025

Develop a way to create and a package format for smart contracts #550

Closed

		slot: raw.slot.expect("was checked to be present"),
		value: raw.value.expect("was checked to be present"),

		let slots = raw.slots.ok_or_else(\|\| D::Error::missing_field("slots"))?;
		let values = raw.values.ok_or_else(\|\| D::Error::missing_field("values"))?;

		// SAFETY: u8 cast to u64 is safe. We cannot use u64::from in const context so we
		// are forced to cast.

	package: &AccountComponentTemplate,
	template: &AccountComponentTemplate,

	/// A package can be instantiated into [AccountComponent](super::AccountComponent) objects.
	/// A template can be instantiated into [AccountComponent](super::AccountComponent) objects.

	/// The account's previously-assembled code. This defines all functionality related to the
	/// The account component's assembled code. This defines all functionality related to the

		/// The component metadata can be defined with generic keys that can be replaced at instantiation
		/// time.

		} else if let Ok(decimal_value) = value.parse::<u64>() {
		Ok(FeltRepresentation::SingleDecimal(Felt::new(decimal_value)))

	Err(serde::de::Error::custom("Value is not a valid element"))
	Err(serde::de::Error::custom("deserialized string value is not a valid variant of FeltRepresentation"))

feat: Introduce AccountComponentTemplate #1015

feat: Introduce AccountComponentTemplate #1015

Conversation

igamigo commented Dec 11, 2024 • edited Loading

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

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

igamigo Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo Jan 3, 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 3, 2025 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

igamigo commented Jan 6, 2025

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

igamigo commented Jan 7, 2025

bobbinth left a comment

Choose a reason for hiding this comment

PhilippGackstatter left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

feat: Introduce `AccountComponentTemplate` #1015

feat: Introduce `AccountComponentTemplate` #1015

igamigo commented Dec 11, 2024 •

edited

Loading

igamigo Jan 3, 2025 •

edited

Loading

igamigo Jan 3, 2025 •

edited

Loading

igamigo commented Jan 3, 2025 •

edited

Loading