refactor: destructure `AccountComponentCode` by igamigo · Pull Request #2597 · 0xMiden/protocol

igamigo · 2026-03-12T19:38:27Z

bobbinth

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

bobbinth · 2026-03-17T00:25:27Z

crates/miden-protocol/src/account/component/code.rs

+pub struct AccountComponentCode {
+    mast: Arc<MastForest>,
+    exports: Vec<ProcedureExport>,
+}


Do we actually need to store full ProcedureExport here? I guess we use these to build Library out of account component node - but I wonder if that's actually needed.

The alternative could be to follow the pattern we have in NoteScript - e.g., this could look like:

pub struct AccountComponentCode { mast: Arc<MastForest>, exports: Vec<MastNodeId>, }

Currently, we have the following pattern for component code:

static BASIC_WALLET_LIBRARY: LazyLock<Library> = ... pub fn basic_wallet_library() -> Library { BASIC_WALLET_LIBRARY.clone() } procedure_digest!( BASIC_WALLET_RECEIVE_ASSET, BasicWallet::NAME, BasicWallet::RECEIVE_ASSET_PROC_NAME, basic_wallet_library ); impl BasicWallet { pub const NAME: &'static str = "miden::standards::components::wallets::basic_wallet"; const RECEIVE_ASSET_PROC_NAME: &str = "receive_asset"; pub fn receive_asset_digest() -> Word { *BASIC_WALLET_RECEIVE_ASSET } }

While thinking about a trait AccountComponentInterface, I thought it would make more sense if we would actually use AccountComponentCode instead of Library and make the code accessible on the component itself, so:

static BASIC_WALLET_CODE: LazyLock<AccountComponentCode> = LazyLock::new(|| { let bytes = include_bytes!(concat!( env!("OUT_DIR"), "/assets/account_components/wallets/basic_wallet.masl" )); let library = Library::read_from_bytes(bytes).expect("Shipped Basic Wallet library is well-formed"); AccountComponentCode::from(library) }); impl BasicWallet { pub fn code() -> &'static AccountComponentCode { &BASIC_WALLET_CODE } }

This would also mean changing procedure_digest to take AccountComponentCode as an input instead of a Library, so it would need to have the ability to call get_procedure_root_by_path.

That would be cleaner and increase type safety - we'd get more out of the AccountComponentCode type. Maybe this is still possible with what you proposed. Just mentioning that this would be nice to be able to do eventually and the changes here would have to be compatible with that.

bobbinth · 2026-03-17T00:26:51Z

crates/miden-protocol/src/account/component/code.rs

+    pub fn get_procedure_root_by_path(&self, path: impl AsRef<Path>) -> Option<Word> {
+        let path = path.as_ref().to_absolute();
+        self.exports
+            .iter()
+            .find(|export| export.path.as_ref() == path.as_ref())
+            .map(|export| self.mast[export.node].digest())
+    }


Similar to the previous comment: is this used anywhere outside of tests?

crates/miden-protocol/src/account/component/code.rs

bobbinth · 2026-03-17T00:32:06Z

crates/miden-protocol/src/account/component/code.rs

+    /// Creates a new [`AccountComponentCode`] from the provided MAST forest and procedure exports.
+    pub fn new(mast: Arc<MastForest>, exports: Vec<ProcedureExport>) -> Self {
+        Self { mast, exports }
    }


We should probably check consistency here between mast and exports to make sure the exported procedures are actually in the MAST forest.

Now that I think about it, we should probably do the same for NoteScript::new() constructor.

Also, it may be good to add AccountComponentCode::from_parts(), AccountComponentCode::from_library(), and maybe AccountComponentCode::from_library_reference() to follow the pattern we have with NoteScript.

crates/miden-protocol/src/account/component/code.rs

PhilippGackstatter

Looks good to me!

crates/miden-protocol/src/account/component/mod.rs

PhilippGackstatter · 2026-03-17T15:51:10Z

crates/miden-protocol/src/account/component/code.rs

+impl From<&AccountComponentCode> for Library {
+    fn from(value: &AccountComponentCode) -> Self {
+        let exports: BTreeMap<_, _> =
+            value.exports.iter().cloned().map(|e| (e.path.clone(), e.into())).collect();
+
+        Library::new(value.mast.clone(), exports)
+            .expect("AccountComponentCode should have at least one export")


I generally think we should not implement conversion using references when the implementation clones. In such cases, I think it's better to make the clone explicit to the caller instead of hiding it, e.g. Library::from(component_code.clone()).

Implementing both APIs could easily lead to a clone even when one actually has an owned value that could be moved.

Yeah, sorry about this one, you've mentioned this before. I think I was mostly focused in keeping the API close to what it was before (mainly for linking in CodeBuilder)

crates/miden-protocol/src/account/component/code.rs

…/protocol into igamigo-destructure-code

PhilippGackstatter

Left two comments regarding no longer having AsRef<Library> and the from_library_reference implementation.

PhilippGackstatter · 2026-03-18T11:36:21Z

crates/miden-protocol/src/account/component/code.rs

+    /// Returns a new [`AccountComponentCode`] containing only external node references to the
+    /// procedures in the provided [`Library`].
+    ///
+    /// Note: This method creates a minimal [`MastForest`] where each exported procedure is
+    /// represented by an external node referencing its digest, rather than copying the entire
+    /// library's MAST forest. The actual procedure code will be resolved at runtime via the
+    /// `MastForestStore`.
+    pub fn from_library_reference(library: &Library) -> Self {


Do we not need the full account code at the component level, so that when we create an Account from an AccountBuilder, that account has the full code and not just references?
Or in other words, how would the Account supply its code at tx execution time then? I think the following functions rely on the full code being present:

LocalTransactionProver::prove

AccountDeltaTracker::new

TransactionContextBuilder::build being an example for how users of the TransactionExecutor probably inserts code, at least that's my understanding.

So, I think having a function similar to NoteScript::from_library_reference does not make sense for account component code, since we're constructing a library and not an executable.

Also, I think a better name for NoteScript::from_library_reference would be from_library_procedure to better capture that we're creating a script from a procedure in a library.

crates/miden-protocol/src/account/component/mod.rs

PhilippGackstatter · 2026-03-18T11:42:38Z

crates/miden-protocol/src/lib.rs

    pub use miden_assembly::ast::{Module, ModuleKind, ProcedureName, QualifiedProcedureName};
    pub use miden_assembly::debuginfo::SourceManagerSync;
-    pub use miden_assembly::library::LibraryExport;
+    pub use miden_assembly::library::{LibraryExport, ProcedureExport as LibraryProcedureExport};


What is the reason that we need this rename? I think, ideally, we'd keep the original name as having two names for the same thing often leads to confusion and I recall having IDE import problems with the Hasher type (which is also renamed and I wish we would remove that).

PhilippGackstatter · 2026-03-18T11:55:19Z

crates/miden-standards/src/account/interface/test.rs

    let note_script = CodeBuilder::default()
-        .with_dynamically_linked_library(custom_component.component_code())
+        .with_dynamically_linked_library(custom_component.component_code().clone())


I wonder if the destructuring of AccountComponentCode into its parts is worth these clones. These are necessary because we no longer have impl AsRef<Library> for AccountComponentCode, afaict.

On a conceptual level, an account component's code is a Library (contrary to NoteScript or TransactionScript which are built from libraries but are executables) and dynamically linking a library only requires an AsRef<Library> so the clone is "conceptually" unnecessary.

The clone is cheap for the mast forest, and not too bad for the exports I suppose. Still, the previous structure provided better usability on this front.

How much are we actually saving in terms of space with the new structure, which I think is the motivation for the refactor, right?

Yes, the clones are indeed necessary because AsRef is no longer possible/appropriate. And to be honest, I'm not entirely sure if there's a specific motivation for the refactor other than reducing some space (though it's minimal and the remainder still dominates).

One of the motivations is that Library struct will go away soon (it will be replaced by Package struct). But putting a full Package into the AccountComponentCode seems like an overkill.

I think my main question is whether we need AccountComponentCode to be convertible into libraries. AFAICT, this is only used in test code (but maybe I missed something?). If so, maybe there is some other solution that that we could apply to tests specifically?

Alternatively, we can delay this change until the Library struct actually goes away, and for now apply some of the more straight-forward changes that @igamigo mentioned in #2597 (comment).

clear_debug_info() (would need upstream support on Library AFAICT)

If we no longer deal with Library structs, we should be able to do this on MastForest directly.

igamigo · 2026-03-18T17:20:27Z

With the conceptual difference between note/transaction scripts versus account component code (the scripts are executables, and the latter is a library) API being less ergonomic overall and with minimal space savings being won from this refactor, it might be worth revising whether this is a good refactor. We could still decide to keep some of the changes in this PR even if we wanted to discard the main change, such as:

procedures() returning impl Iterator<Item = (AccountProcedureRoot, bool)>
procedure_roots() on AccountComponentCode
clear_debug_info() (would need upstream support on Library AFAICT)
The rename of get_procedures to procedures
add_procedures parameter change

igamigo added 3 commits March 12, 2026 16:36

refactor: destructure code

b83cefe

refactor: avoid clone

3094e94

chore: changelog

956d014

igamigo requested review from PhilippGackstatter and bobbinth March 16, 2026 21:17

igamigo marked this pull request as ready for review March 16, 2026 21:17

Merge branch 'next' into igamigo-destructure-code

477ad21

bobbinth reviewed Mar 17, 2026

View reviewed changes

PhilippGackstatter mentioned this pull request Mar 17, 2026

Implement AccountComponentInterface trait #2621

Open

PhilippGackstatter reviewed Mar 17, 2026

View reviewed changes

igamigo added 6 commits March 17, 2026 21:35

reviews: remove from<ref>, improve and add constructor

cfbe27e

chore: merge

93cf2ee

Merge branch 'next' into igamigo-destructure-code

b97861c

test: fixes

ea2fde3

Merge branch 'igamigo-destructure-code' of https://github.com/0xMiden…

b0a9f65

…/protocol into igamigo-destructure-code

docs: correctly link

e4168d4

PhilippGackstatter reviewed Mar 18, 2026

View reviewed changes

reviews: rename nits

b8f1786

Conversation

igamigo commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igamigo Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igamigo commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

igamigo commented Mar 12, 2026 •

edited

Loading

igamigo Mar 18, 2026 •

edited

Loading