Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
- Made `BasicFungibleFaucet::MAX_DECIMALS` public (#1063).
- [BREAKING] Removed `miden-tx-prover` crate and created `miden-proving-service` and `miden-remote-provers` (#1047).
- Deduplicate `masm` procedures across kernel and miden lib to a shared `util` module (#1070).
- Added storage placeholder types and support for templated map (#1074).

## 0.6.2 (2024-11-20)

Expand Down
3 changes: 2 additions & 1 deletion objects/src/accounts/component/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ use vm_processor::MastForest;
mod template;
pub use template::{
AccountComponentMetadata, AccountComponentTemplate, FeltRepresentation, InitStorageData,
StorageEntry, StoragePlaceholder, StorageValue, WordRepresentation,
MapRepresentation, PlaceholderType, StorageEntry, StoragePlaceholder, StorageValue,
WordRepresentation,
};

use crate::{
Expand Down
36 changes: 29 additions & 7 deletions objects/src/accounts/component/template/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use alloc::{
collections::BTreeSet,
collections::{btree_map::Entry, BTreeMap, BTreeSet},
string::{String, ToString},
vec::Vec,
};
Expand Down Expand Up @@ -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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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?

Copy link
Collaborator Author

@igamigo igamigo Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let mut placeholder_map = BTreeMap::new();
for storage_entry in &self.storage {
for key in storage_entry.placeholders() {
placeholder_set.insert(key.clone());
for (placeholder, placeholder_type) in storage_entry.placeholders() {
placeholder_map.insert(placeholder.clone(), placeholder_type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
placeholder_set
placeholder_map
}

/// Returns the name of the account component.
Expand Down Expand Up @@ -254,6 +254,29 @@ impl AccountComponentMetadata {
return Err(AccountComponentTemplateError::NonContiguousSlots(slots[0], slots[1]));
}
}

// Check that placeholders do not appear more than once with a different type
let mut placeholders = BTreeMap::new();
Comment on lines +278 to +279
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

for storage_entry in &self.storage {
for (placeholder, placeholder_type) in storage_entry.placeholders() {
match placeholders.entry(placeholder.clone()) {
Entry::Occupied(entry) => {
// if already exists, make sure it's the same type
if *entry.get() != placeholder_type {
return Err(
AccountComponentTemplateError::StoragePlaceholderDuplicate(
placeholder.clone(),
),
);
}
},
Entry::Vacant(slot) => {
slot.insert(placeholder_type);
},
}
}
}

Ok(())
}
}
Expand Down Expand Up @@ -290,7 +313,6 @@ impl Deserializable for AccountComponentMetadata {

#[cfg(test)]
mod tests {

use assembly::Assembler;
use assert_matches::assert_matches;
use storage::WordRepresentation;
Expand Down
123 changes: 75 additions & 48 deletions objects/src/accounts/component/template/storage/mod.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,16 @@
use alloc::{boxed::Box, string::String, vec::Vec};

use vm_core::{
utils::{ByteReader, ByteWriter, Deserializable, Serializable},
Word,
};
use vm_processor::{DeserializationError, Digest};
use vm_core::utils::{ByteReader, ByteWriter, Deserializable, Serializable};
use vm_processor::DeserializationError;

mod word;
pub use word::*;
mod value_type;
pub use value_type::*;

use super::AccountComponentTemplateError;
use crate::accounts::{StorageMap, StorageSlot};
use crate::accounts::StorageSlot;

mod placeholder;
pub use placeholder::{StoragePlaceholder, StorageValue};
pub use placeholder::{PlaceholderType, StoragePlaceholder, StorageValue};

mod init_storage_data;
pub use init_storage_data::InitStorageData;
Expand Down Expand Up @@ -54,7 +51,7 @@ pub enum StorageEntry {
/// The numeric index of this map slot in the component's storage.
slot: u8,
/// A list of key-value pairs to initialize in this map slot.
map_entries: Vec<MapEntry>,
map_entries: MapRepresentation,
},

/// A multi-slot entry, representing a single logical value across multiple slots.
Expand Down Expand Up @@ -91,13 +88,13 @@ impl StorageEntry {
name: impl Into<String>,
description: Option<impl Into<String>>,
slot: u8,
map_entries: Vec<MapEntry>,
map_representation: MapRepresentation,
) -> Self {
StorageEntry::Map {
name: name.into(),
description: description.map(Into::<String>::into),
slot,
map_entries,
map_entries: map_representation,
}
}

Expand Down Expand Up @@ -167,24 +164,14 @@ impl StorageEntry {
}
}

/// Returns the map entries for a `Map` variant as a slice.
/// Returns an empty slice for non-map variants.
pub fn map_entries(&self) -> &[MapEntry] {
match self {
StorageEntry::Map { map_entries: values, .. } => values.as_slice(),
StorageEntry::Value { .. } => &[],
StorageEntry::MultiSlot { .. } => &[],
}
}

/// Returns an iterator over all of the storage entries's placeholder keys.
// TODO: Should placeholders be typed?
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(
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&self,
) -> Box<dyn Iterator<Item = (&StoragePlaceholder, PlaceholderType)> + '_> {
match self {
StorageEntry::Value { value, .. } => value.placeholders(),
StorageEntry::Map { map_entries: values, .. } => {
Box::new(values.iter().flat_map(|word| word.placeholders()))
},
StorageEntry::Map { map_entries, .. } => map_entries.placeholders(),
StorageEntry::MultiSlot { values, .. } => {
Box::new(values.iter().flat_map(|word| word.placeholders()))
},
Expand All @@ -209,16 +196,7 @@ impl StorageEntry {
Ok(vec![StorageSlot::Value(slot)])
},
StorageEntry::Map { map_entries: values, .. } => {
let entries = values
.iter()
.map(|map_entry| {
let key = map_entry.key().try_build_word(init_storage_data)?;
let value = map_entry.value().try_build_word(init_storage_data)?;
Ok((key.into(), value))
})
.collect::<Result<Vec<(Digest, Word)>, AccountComponentTemplateError>>()?; // Collect into a Vec and propagate errors

let storage_map = StorageMap::with_entries(entries);
let storage_map = values.try_build_map(init_storage_data)?;
Ok(vec![StorageSlot::Map(storage_map)])
},
StorageEntry::MultiSlot { values, .. } => Ok(values
Expand Down Expand Up @@ -285,13 +263,13 @@ impl Deserializable for StorageEntry {
// Map
1 => {
let slot = source.read_u8()?;
let values: Vec<MapEntry> = source.read()?;
let map_representation: MapRepresentation = source.read()?;

Ok(StorageEntry::Map {
name,
description,
slot,
map_entries: values,
map_entries: map_representation,
})
},

Expand Down Expand Up @@ -335,7 +313,9 @@ impl MapEntry {
&self.value
}

pub fn placeholders(&self) -> impl Iterator<Item = &StoragePlaceholder> {
/// Returns an iterator over all of the storage entries's placeholder keys, alongside their
/// expected type.
pub fn placeholders(&self) -> impl Iterator<Item = (&StoragePlaceholder, PlaceholderType)> {
self.key.placeholders().chain(self.value.placeholders())
}

Expand Down Expand Up @@ -365,12 +345,14 @@ impl Deserializable for MapEntry {

#[cfg(test)]
mod tests {
use core::panic;
use std::collections::BTreeSet;

use assembly::Assembler;
use assert_matches::assert_matches;
use semver::Version;
use vm_core::{Felt, FieldElement};
use vm_core::{Felt, FieldElement, Word};
use vm_processor::Digest;

use super::*;
use crate::{
Expand Down Expand Up @@ -402,7 +384,7 @@ mod tests {
name: "map".into(),
description: Some("A storage map entry".into()),
slot: 1,
map_entries: vec![
map_entries: MapRepresentation::List(vec![
MapEntry {
key: WordRepresentation::Template(
StoragePlaceholder::new("foo.bar").unwrap(),
Expand All @@ -419,7 +401,7 @@ mod tests {
key: WordRepresentation::Hexadecimal(digest!("0x3").into()),
value: WordRepresentation::Hexadecimal(digest!("0x4").into()),
},
],
]),
},
StorageEntry::MultiSlot {
name: "multi".into(),
Expand Down Expand Up @@ -470,7 +452,7 @@ mod tests {
slot = 0
values = [
{ key = "0x1", value = ["{{value.test}}", "0x1", "0x2", "0x3"] },
{ key = "{{key.test}}", value = "0x3" },
{ key = "{{map.key.test}}", value = "0x3" },
{ key = "0x3", value = "0x4" }
]

Expand All @@ -488,12 +470,25 @@ mod tests {
"{{word.test}}",
["1", "0", "0", "0"],
]

[[storage]]
name = "map-template"
description = "a templated map"
slot = 4
values = "{{map.template}}"
"#;

let component_metadata = AccountComponentMetadata::from_toml(toml_text).unwrap();
let library = Assembler::default().assemble_library([CODE]).unwrap();
for (key, placeholder_type) in component_metadata.get_storage_placeholders() {
match key.inner() {
"map.key.test" | "word.test" => assert_eq!(placeholder_type, PlaceholderType::Word),
"value.test" => assert_eq!(placeholder_type, PlaceholderType::Felt),
"map.template" => assert_eq!(placeholder_type, PlaceholderType::Map),
_ => panic!("all cases are covered"),
}
}

assert_eq!(component_metadata.storage_entries().first().unwrap().map_entries().len(), 3);
let library = Assembler::default().assemble_library([CODE]).unwrap();

let template = AccountComponentTemplate::new(component_metadata, library);

Expand All @@ -504,7 +499,7 @@ mod tests {

let storage_placeholders = InitStorageData::new([
(
StoragePlaceholder::new("key.test").unwrap(),
StoragePlaceholder::new("map.key.test").unwrap(),
StorageValue::Word(Default::default()),
),
(
Expand All @@ -515,6 +510,10 @@ mod tests {
StoragePlaceholder::new("word.test").unwrap(),
StorageValue::Word([Felt::ZERO, Felt::ZERO, Felt::ZERO, Felt::new(128)]),
),
(
StoragePlaceholder::new("map.template").unwrap(),
StorageValue::Map(vec![(Digest::default(), Word::default())]),
),
]);

let component = AccountComponent::from_template(&template, &storage_placeholders).unwrap();
Expand Down Expand Up @@ -542,4 +541,32 @@ mod tests {
))
);
}

#[test]
pub fn fail_duplicate_key() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn fail_duplicate_key() {
pub fn fail_placeholder_type_mismatch() {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

[[storage]]
name = "word"
slot = 1
value = "{{value.test}}"
"#;
let component_metadata = AccountComponentMetadata::from_toml(toml_text);
assert_matches!(
component_metadata,
Err(AccountComponentTemplateError::StoragePlaceholderDuplicate(_))
);
}
}
20 changes: 20 additions & 0 deletions objects/src/accounts/component/template/storage/placeholder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,26 @@ pub struct StoragePlaceholder {
key: String,
}

/// An identifier for the expected type for a storage placeholder.
/// These indicate which variant of [StorageValue] should be provided when instantiating a
/// component.
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum PlaceholderType {
Felt,
Map,
Word,
}
bobbinth marked this conversation as resolved.
Show resolved Hide resolved

impl core::fmt::Display for PlaceholderType {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
match self {
PlaceholderType::Felt => f.write_str("Felt"),
PlaceholderType::Map => f.write_str("Map"),
PlaceholderType::Word => f.write_str("Word"),
}
}
}

impl StoragePlaceholder {
/// Creates a new [StoragePlaceholder] from the provided string.
///
Expand Down
Loading
Loading