refactor: add SlotName to StorageSlotHeader with Cow lifetime parameter

[Farukest]

Summary

Refactors StorageSlotHeader to contain SlotName using Cow<'name, SlotName> for efficient borrowing without unnecessary cloning.

Changes

• StorageSlotHeader now has a lifetime parameter and contains slot name
• AccountStorageHeader uses Vec<StorageSlotHeader<'static>>
• Added Serializable/Deserializable implementations for SlotName
• Updated AccountStorage::to_header() to use placeholder slot names
• Updated miden-tx crate to use the new API

Closes #2126

[PhilippGackstatter]

@Farukest Thank you! Can you rebase this onto next (or merge next)? There have been some changes and renames, e.g. SlotName -> StorageSlotName.

[Farukest]

Hey @PhilippGackstatter working on it. thanks :)

[Farukest]

Done @PhilippGackstatter

[PhilippGackstatter]

Looks good to me! Great work! You did a great job of matching the miden-base style of Rust 😄. Thanks for adding docs, separators and the little things. Much appreciated!

Can you merge in next again?

I left two more comments for simplification.

[PhilippGackstatter]

I think I would simplify this a bit more and add some convenience constructors on the header type, i.e.:

/// Creates empty slots of the same slot types as the to-be-created account.
fn empty_storage_header_from_account(account: &PartialAccount) -> AccountStorageHeader {
    let slots: Vec<StorageSlotHeader> = account
        .storage()
        .header()
        .slots()
        .map(|slot_header| match slot_header.slot_type() {
            StorageSlotType::Value => {
                StorageSlotHeader::with_empty_value(slot_header.name().clone())
            },
            StorageSlotType::Map => StorageSlotHeader::with_empty_map(slot_header.name().clone()),
        })
        .collect();

    AccountStorageHeader::new(slots).expect("storage header should be valid")
}

(The sort step was removed in #2154 which is on next).
This requires:

pub fn with_empty_value(name: StorageSlotName) -> Self {
    Self::with_owned_name(name, StorageSlotType::Value, Word::empty())
}

pub fn with_empty_map(name: StorageSlotName) -> Self {
    Self::with_owned_name(name, StorageSlotType::Map, EMPTY_STORAGE_MAP_ROOT)
}

[Farukest]

Done !

[PhilippGackstatter]

I think we can make this a bit nicer as well by defining a storage slot -> header conversion, i.e.:

impl<'name> From<&'name StorageSlot> for StorageSlotHeader<'name> {
    fn from(slot: &'name StorageSlot) -> Self {
        StorageSlotHeader::with_borrowed_name(slot.name(), slot.slot_type(), slot.value())
    }
}

And then we can do:

AccountStorageHeader::new(
          self.slots
              .iter()
              .map(StorageSlotHeader::from)
              .map(StorageSlotHeader::into_owned)
              .collect(),
      )
      .expect("slots should be valid as ensured by AccountStorage")

And then I think we can remove AccountStorageHeader::from_tuples, as it's only really needed in tests and encourages use of the header type directly. If you prefer keeping it for the tests, that's fine too. In that case, let's move it behind #[cfg(any(feature = "testing", test))] and to crates/miden-objects/src/testing/storage.rs.

[Farukest]

Done too :)

[Farukest]

Looks good to me! Great work! You did a great job of matching the miden-base style of Rust 😄. Thanks for adding docs, separators and the little things. Much appreciated!

I try my best for miden. thank you for your kind words 😄
@PhilippGackstatter

[bobbinth]

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

[bobbinth]

I thought the intent was to keep StorageSlotHeader as an internal-only type. If we do want to make it public, I'd rather not complicate the interface with borrowed vs. owned semantics.

If the main reason for this is to avoid cloning strings, we could define StorageSlotName as:

pub struct StorageSlotName {
    name: Arc<str>,
    id: StorageSlotId,
}

[Farukest]

Done too @bobbinth

[bobbinth]

Thank you! But I think the question is still whether we want to add lifetime parameters to StorageSlotHeader. My preference would be not to do this and update StorageSlotName as I mention in the above comment. But would be great to get @PhilippGackstatter thoughts on this too.

[Farukest]

I'm waiting for your discussion result :)

[PhilippGackstatter]

After making the decision to keep the slot header internal, I encountered a few places where we exposed the underlying 3-tuple of the header and where it would be beneficial to have a wrapper type for that. Besides, the tuple representation commits to the structure as much as a wrapper type, so the benefit of keeping it internal no longer seemed great enough. So I now think it's fine to expose the StorageSlotHeader.

I think redefining StorageSlotName to use Arc<str> is also fine to avoid the lifetime parameter. In the context of the account component metadata refactor I thought we may have to replace StorageValueName with StorageSlotName. In that case, having a String internally may be better (for mutability), but after realizing there are conflicting requirements between those two types (slot name should enforce a minimum length; storage value name can also represent a single part of a name) I think we cannot deduplicate those and so making the internal representation immutable seems fine.

@Farukest Let's change StorageSlotName to use Arc<str> and then change StorageSlotHeader to use name: StorageSlotName and remove the lifetime.

[Farukest]

Done !

[bobbinth]

Also, @Farukest - could you merge the latest next into this branch?

[bobbinth]

It seems like there are some extraneous changes in this file.

[Farukest]

Sorry my work environment is too messy nowadays.
Fixed :)

[bobbinth]

Looks good! Thank you! Let's add a single entry to the changelog, and I think this is good to go.

[Farukest]

Looks good! Thank you! Let's add a single entry to the changelog, and I think this is good to go.

Done @bobbinth