feat: `ComponentMetadata` templating #1027

igamigo · 2024-12-18T23:34:15Z

No description provided.

bobbinth

Looks good! Thank you! Not a full review yet, but I left some comments inline.

bobbinth · 2024-12-26T09:44:50Z

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

+pub enum TemplateValue {
+    Felt(Felt),
+    Word(Word),
+    Map(Vec<(Digest, Word)>),


nit: would Entries be a better name here?

Instead of Map? Sounds a bit more generic IMO but I can rename it if preferred

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

objects/src/accounts/package/mod.rs

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

objects/src/accounts/package/mod.rs

tomyrd · 2024-12-26T19:17:24Z

objects/src/accounts/package/mod.rs

+        // Check for duplicates
+        let mut seen = BTreeSet::new();
+        let duplicate =
+            all_slots
+                .iter()
+                .find_map(|&slot| if !seen.insert(slot) { Some(slot) } else { None });
+
+        if let Some(dup) = duplicate {
+            return Err(ComponentPackageError::DuplicateSlots(dup));
+        }


This check can be done inside the all_slots.windows(2). We should check that slots[1] == slots[0] doesn't happen.

Good point! Changed.

tomyrd · 2024-12-26T19:21:59Z

objects/src/accounts/package/mod.rs

+    #[error("error creating AccountComponent: {0}")]
+    AccountComponentError(AccountError),


Could we use #[from] here?

Did you mean #[source]? I added #[from] in the serde error and #[source] here. I'd rather avoid automatically adding the conversion since there might be places where we benefit from double-checking the context

objects/src/accounts/package/mod.rs

tomyrd · 2024-12-26T19:25:22Z

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

@@ -160,6 +164,59 @@ impl StorageEntry {
            StorageEntry::MultiSlot { .. } => &[],
        }
    }
+
+    /// Returns an iterator over all of the storage entries's template keys.
+    // TODO: Should template keys be typed?


What do we mean by "typed" here? Could keys be of different types other than strings?

EDIT: Oh, maybe we mean they key having the storage entry variant where it came from. I think this can be checked by the caller if necessary.

I was referring to whether we want keys to have what type of value they expect, basically.

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

tomyrd · 2024-12-26T19:40:06Z

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

+    }
+
+    /// Returns `Ok(&Vec<(Digest, Word)>>` if the variant is `Map`, otherwise errors.
+    pub fn as_map(&self) -> Result<&Vec<(Digest, Word)>, ComponentPackageError> {


I think this is not used, will this be used in a later PR?

tomyrd · 2024-12-26T19:49:50Z

objects/src/accounts/package/mod.rs

+    ///   they are not of a valid type)
+    pub fn instantiate_component(
+        &self,
+        template_keys: &BTreeMap<String, TemplateValue>,


Suggested change

template_keys: &BTreeMap<String, TemplateValue>,

template_keys: &BTreeMap<TemplateKey, TemplateValue>,

Same in the try_into_... methods that have this map as a parameter

I'll probably leave refactoring initialization data for future PRs as explained here: #1027 (comment)

tomyrd · 2024-12-26T19:56:20Z

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

+                        FeltRepresentation::Dynamic(template_key) => *template_values
+                            .get(template_key.key())
+                            .ok_or_else(|| {
+                                ComponentPackageError::TemplateValueNotProvided(
+                                    template_key.key().to_string(),
+                                )
+                            })?
+                            .as_felt()?,


Maybe this should be moved into a FeltRepresentation::try_into_felt(template_values) method, so it is more in line with the other objects.

Factored out the code

tomyrd · 2024-12-26T20:01:06Z

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

+                // Attempt to deserialize as TemplateKey first
+                if let Ok(tk) = TemplateKey::try_deserialize(value) {
+                    return Ok(WordRepresentation::Dynamic(tk));
+                }


Instead of trying one then the other, we could check the first two chars, if they are 0x the we use the parse_hex_string_as_word. This way the error message could be different in both cases because we know which variant we were expecting.

While implementing this I realized it is still valuable to output valid formats for string-based values. For example: maybe someone writes "123" expecting decimals to work but then it won't match with the "0x" and so we would assume a dynamic value when that's not really the case.

bobbinth · 2024-12-29T22:22:22Z

I think maybe the way to proceed with this and #1015 PRs is:

Address some small comments in this PR and merge it into #1015 .
Review #1015 comprehensively and merge it.

Otherwise, I find myself flipping back-and-forth between the two PRs to see where a given comment may be more appropriate.

Co-authored-by: Tomas Rodriguez Dala <[email protected]>

igamigo · 2024-12-30T21:05:44Z

I think maybe the way to proceed with this and #1015 PRs is:

Address some small comments in this PR and merge it into #1015 .

Review #1015 comprehensively and merge it.

Otherwise, I find myself flipping back-and-forth between the two PRs to see where a given comment may be more appropriate.

Sounds fair. @bobbinth if it's OK by you, I'll merge this and continue on the base branch

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

feat: Allow component templating

c73a3d2

igamigo force-pushed the igamigo-pkg-template branch from ba8cc33 to c73a3d2 Compare December 23, 2024 15:16

igamigo added 2 commits December 23, 2024 12:31

test: Improve test

5abe6c1

test: Improve test

81f09dd

igamigo marked this pull request as ready for review December 23, 2024 15:37

igamigo and others added 2 commits December 23, 2024 12:50

chore: Lints

01fdc7d

Merge branch 'igamigo-acc-pkg' into igamigo-pkg-template

5e36791

bobbinth reviewed Dec 26, 2024

View reviewed changes

tomyrd reviewed Dec 26, 2024

View reviewed changes

bobbinth and others added 2 commits December 29, 2024 13:57

Merge branch 'igamigo-acc-pkg' into igamigo-pkg-template

7f6548d

chore: fix lints

468dbe7

igamigo and others added 6 commits December 30, 2024 17:08

Reviews: Address most small comments

3ea09c1

Reviews: Address most small comments

6d9b32f

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

2a015c4

Co-authored-by: Tomas Rodriguez Dala <[email protected]>

Reviews: Factor out felt code

c498bfc

Reviews: feature guard on component constructor

45ea3de

Reviews: errors

565192b

igamigo added 2 commits December 30, 2024 22:57

chore: Doc build

df611b0

chore: Format

2f71529

bobbinth merged commit 3095071 into igamigo-acc-pkg Jan 2, 2025
9 checks passed

bobbinth deleted the igamigo-pkg-template branch January 2, 2025 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `ComponentMetadata` templating #1027

feat: `ComponentMetadata` templating #1027

igamigo commented Dec 18, 2024 •

edited

Loading

bobbinth left a comment

bobbinth Dec 26, 2024

igamigo Dec 30, 2024

tomyrd Dec 26, 2024

igamigo Dec 30, 2024

tomyrd Dec 26, 2024

igamigo Dec 30, 2024

tomyrd Dec 26, 2024

igamigo Dec 30, 2024

tomyrd Dec 26, 2024

tomyrd Dec 26, 2024

tomyrd Dec 26, 2024

igamigo Dec 30, 2024

tomyrd Dec 26, 2024

igamigo Dec 30, 2024

tomyrd Dec 26, 2024

igamigo Dec 30, 2024

bobbinth commented Dec 29, 2024

igamigo commented Dec 30, 2024

		#[error("error creating AccountComponent: {0}")]
		AccountComponentError(AccountError),

	template_keys: &BTreeMap<String, TemplateValue>,
	template_keys: &BTreeMap<TemplateKey, TemplateValue>,

feat: ComponentMetadata templating #1027

feat: ComponentMetadata templating #1027

Conversation

igamigo commented Dec 18, 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

bobbinth commented Dec 29, 2024

igamigo commented Dec 30, 2024

feat: `ComponentMetadata` templating #1027

feat: `ComponentMetadata` templating #1027

igamigo commented Dec 18, 2024 •

edited

Loading