Skip to content
Open
Show file tree
Hide file tree
Changes from 10 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 @@ -41,6 +41,7 @@

### Changes

- [BREAKING] Deconstructed `AccountComponentCode` into `Arc<MastForest>` + `Vec<ProcedureExport>` fields, replacing the `Library` newtype wrapper ([#2597](https://github.com/0xMiden/protocol/pull/2597)).
- [BREAKING] Removed `NoteAssets::add_asset`; `OutputNoteBuilder` now accumulates assets in a `Vec` and computes the commitment only when `build()` is called, avoiding rehashing on every asset addition. ([#2577](https://github.com/0xMiden/protocol/pull/2577))
- [BREAKING] Made `supported_types` a required parameter of `AccountComponentMetadata::new()`; removed `with_supported_type`, `with_supported_types`, `with_supports_all_types`, and `with_supports_regular_types` builder methods; added `AccountType::all()` and `AccountType::regular()` helpers ([#2554](https://github.com/0xMiden/protocol/pull/2554)).
- [BREAKING] Migrated to miden-vm 0.21 and miden-crypto 0.22 ([#2508](https://github.com/0xMiden/miden-base/pull/2508)).
Expand Down
12 changes: 6 additions & 6 deletions crates/miden-protocol/src/account/code/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,9 @@ impl AccountCode {
pub(super) fn from_components_unchecked(
components: &[AccountComponent],
) -> Result<Self, AccountError> {
let mast_forests: Vec<_> = components.iter().map(|component| component.mast()).collect();
let (merged_mast_forest, _) =
MastForest::merge(components.iter().map(|component| component.mast_forest()))
MastForest::merge(mast_forests.iter().map(|mast| mast.as_ref()))
.map_err(AccountError::AccountComponentMastForestMergeError)?;

let mut builder = AccountProcedureBuilder::new();
Expand Down Expand Up @@ -338,7 +339,7 @@ impl AccountProcedureBuilder {
fn add_auth_component(&mut self, component: &AccountComponent) -> Result<(), AccountError> {
let mut auth_proc_count = 0;

for (proc_root, is_auth) in component.get_procedures() {
for (proc_root, is_auth) in component.procedures() {
self.add_procedure(proc_root);

if is_auth {
Expand All @@ -358,20 +359,19 @@ impl AccountProcedureBuilder {
}

fn add_component(&mut self, component: &AccountComponent) -> Result<(), AccountError> {
for (proc_mast_root, is_auth) in component.get_procedures() {
for (proc_root, is_auth) in component.procedures() {
if is_auth {
return Err(AccountError::AccountCodeMultipleAuthComponents);
}
self.add_procedure(proc_mast_root);
self.add_procedure(proc_root);
}

Ok(())
}

fn add_procedure(&mut self, proc_mast_root: Word) {
fn add_procedure(&mut self, proc_root: AccountProcedureRoot) {
// Allow procedures with the same MAST root from different components, but only add them
// once.
let proc_root = AccountProcedureRoot::from_raw(proc_mast_root);
if !self.procedures.contains(&proc_root) {
self.procedures.push(proc_root);
}
Expand Down
158 changes: 130 additions & 28 deletions crates/miden-protocol/src/account/component/code.rs
Original file line number Diff line number Diff line change
@@ -1,33 +1,125 @@
use miden_assembly::Library;
use miden_processor::mast::MastForest;
use alloc::collections::BTreeMap;
use alloc::sync::Arc;
use alloc::vec::Vec;

use miden_assembly::library::ProcedureExport;
use miden_assembly::{Library, Path};
use miden_core::Word;
use miden_core::mast::{ExternalNodeBuilder, MastForest, MastForestContributor, MastNodeExt};

use crate::account::AccountProcedureRoot;
use crate::vm::AdviceMap;

// ACCOUNT COMPONENT CODE
// ================================================================================================

/// A [`Library`] that has been assembled for use as component code.
/// The code associated with an account component, consisting of a [`MastForest`] and the set of
/// procedures exported by the component.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct AccountComponentCode(Library);
pub struct AccountComponentCode {
mast: Arc<MastForest>,
exports: Vec<ProcedureExport>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


impl AccountComponentCode {
/// Returns a reference to the underlying [`Library`]
pub fn as_library(&self) -> &Library {
&self.0
// CONSTRUCTORS
// --------------------------------------------------------------------------------------------

/// Creates a new [`AccountComponentCode`] from the provided [`Library`].
pub fn from_library(library: Library) -> Self {
let mast = library.mast_forest().clone();
let exports: Vec<ProcedureExport> =
library.exports().filter_map(|export| export.as_procedure().cloned()).collect();

Self { mast, exports }
}

/// Returns a reference to the code's [`MastForest`]
pub fn mast_forest(&self) -> &MastForest {
self.0.mast_forest().as_ref()
/// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

let mut mast = MastForest::new();
let mut exports = Vec::new();

for export in library.exports() {
if let Some(proc_export) = export.as_procedure() {
// Get the digest of the procedure from the library
let digest = library.mast_forest()[proc_export.node].digest();

// Create an external node referencing the digest
let node_id = ExternalNodeBuilder::new(digest)
.add_to_forest(&mut mast)
.expect("adding external node to forest should not fail");
mast.make_root(node_id);

exports.push(ProcedureExport {
node: node_id,
path: proc_export.path.clone(),
signature: proc_export.signature.clone(),
attributes: proc_export.attributes.clone(),
});
}
}

Self { mast: Arc::new(mast), exports }
}

/// Consumes `self` and returns the underlying [`Library`]
pub fn into_library(self) -> Library {
self.0
/// Creates a new [`AccountComponentCode`] from the provided MAST forest and procedure exports.
///
/// # Panics
///
/// Panics if any of the exported procedure node IDs are not found in the MAST forest.
pub fn from_parts(mast: Arc<MastForest>, exports: Vec<ProcedureExport>) -> Self {
for export in &exports {
assert!(
mast.get_node_by_id(export.node).is_some(),
"exported procedure node not found in the MAST forest"
);
}

Self { mast, exports }
}

/// Returns a new [AccountComponentCode] with the provided advice map entries merged into the
/// underlying [Library]'s [MastForest].
// PUBLIC ACCESSORS
// --------------------------------------------------------------------------------------------

/// Returns the [`MastForest`] wrapped in an [`Arc`].
pub fn mast(&self) -> Arc<MastForest> {
self.mast.clone()
}

/// Returns the procedure exports of this component.
pub fn exports(&self) -> &[ProcedureExport] {
&self.exports
}

/// Returns an iterator over the [`AccountProcedureRoot`]s of this component's exported
/// procedures.
pub fn procedure_roots(&self) -> impl Iterator<Item = AccountProcedureRoot> + '_ {
self.exports
.iter()
.map(|export| AccountProcedureRoot::from_raw(self.mast[export.node].digest()))
}

/// Returns the digest of the procedure with the specified path, or `None` if it was not found
/// in this component.
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())
}

// MUTATORS
// --------------------------------------------------------------------------------------------

/// Returns a new [`AccountComponentCode`] with the provided advice map entries merged into the
/// underlying [`MastForest`].
///
/// This allows adding advice map entries to an already-compiled account component,
/// which is useful when the entries are determined after compilation.
Expand All @@ -36,28 +128,39 @@ impl AccountComponentCode {
return self;
}

Self(self.0.with_advice_map(advice_map))
let mut mast = Arc::unwrap_or_clone(self.mast);
mast.advice_map_mut().extend(advice_map);

Self {
mast: Arc::new(mast),
exports: self.exports,
}
}
}

impl AsRef<Library> for AccountComponentCode {
fn as_ref(&self) -> &Library {
self.as_library()
/// Clears the debug info from the underlying [`MastForest`].
pub fn clear_debug_info(&mut self) {
let mut mast = self.mast.clone();
Arc::make_mut(&mut mast).clear_debug_info();
self.mast = mast;
}
}

// CONVERSIONS
// ================================================================================================

impl From<Library> for AccountComponentCode {
fn from(value: Library) -> Self {
Self(value)
fn from(library: Library) -> Self {
Self::from_library(library)
}
}

impl From<AccountComponentCode> for Library {
fn from(value: AccountComponentCode) -> Self {
value.into_library()
let exports: BTreeMap<_, _> =
value.exports.into_iter().map(|e| (e.path.clone(), e.into())).collect();

Library::new(value.mast, exports)
.expect("AccountComponentCode should have at least one export")
}
}

Expand All @@ -79,13 +182,12 @@ mod tests {
.expect("failed to assemble library");
let component_code = AccountComponentCode::from(library);

assert!(component_code.mast_forest().advice_map().is_empty());
assert!(component_code.mast().advice_map().is_empty());

// Empty advice map should be a no-op (digest stays the same)
let cloned = component_code.clone();
let original_digest = cloned.as_library().digest();
let original_digest = *Library::from(component_code.clone()).digest();
let component_code = component_code.with_advice_map(AdviceMap::default());
assert_eq!(original_digest, component_code.as_library().digest());
assert_eq!(&original_digest, Library::from(component_code.clone()).digest());

// Non-empty advice map should add entries
let key = Word::from([10u32, 20, 30, 40]);
Expand All @@ -95,7 +197,7 @@ mod tests {

let component_code = component_code.with_advice_map(advice_map);

let mast = component_code.mast_forest();
let mast = component_code.mast();
let stored = mast.advice_map().get(&key).expect("entry should be present");
assert_eq!(stored.as_ref(), value.as_slice());
}
Expand Down
40 changes: 18 additions & 22 deletions crates/miden-protocol/src/account/component/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub use storage::*;
mod code;
pub use code::AccountComponentCode;

use crate::account::{AccountType, StorageSlot};
use crate::account::{AccountProcedureRoot, AccountType, StorageSlot};
use crate::assembly::Path;
use crate::errors::AccountError;
use crate::{MastForest, Word};
Expand Down Expand Up @@ -166,9 +166,9 @@ impl AccountComponent {
&self.code
}

/// Returns a reference to the underlying [`MastForest`] of this component.
pub fn mast_forest(&self) -> &MastForest {
self.code.mast_forest()
/// Returns the [`MastForest`] of this component wrapped in an [`Arc`](alloc::sync::Arc).
pub fn mast(&self) -> alloc::sync::Arc<MastForest> {
self.code.mast()
}

/// Returns a slice of the underlying [`StorageSlot`]s of this component.
Expand Down Expand Up @@ -196,31 +196,27 @@ impl AccountComponent {
self.metadata.supported_types().contains(&account_type)
}

/// Returns a vector of tuples (digest, is_auth) for all procedures in this component.
/// Returns an iterator over ([`AccountProcedureRoot`], is_auth) for all procedures in this
/// component.
///
/// A procedure is considered an authentication procedure if it has the `@auth_script`
/// attribute.
pub fn get_procedures(&self) -> Vec<(Word, bool)> {
let library = self.code.as_library();
let mut procedures = Vec::new();
for export in library.exports() {
if let Some(proc_export) = export.as_procedure() {
let digest = library
.mast_forest()
.get_node_by_id(proc_export.node)
.expect("export node not in the forest")
.digest();
let is_auth = proc_export.attributes.has(AUTH_SCRIPT_ATTRIBUTE);
procedures.push((digest, is_auth));
}
}
procedures
pub fn procedures(&self) -> impl Iterator<Item = (AccountProcedureRoot, bool)> + '_ {
let mast = self.code.mast();
self.code.exports().iter().map(move |proc_export| {
let digest = mast
.get_node_by_id(proc_export.node)
.expect("export node not in the forest")
.digest();
let is_auth = proc_export.attributes.has(AUTH_SCRIPT_ATTRIBUTE);
(AccountProcedureRoot::from_raw(digest), is_auth)
})
}

/// Returns the digest of the procedure with the specified path, or `None` if it was not found
/// in this component's library or its library path is malformed.
/// in this component.
pub fn get_procedure_root_by_path(&self, proc_name: impl AsRef<Path>) -> Option<Word> {
self.code.as_library().get_procedure_root_by_path(proc_name)
self.code.get_procedure_root_by_path(proc_name)
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/miden-protocol/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ pub use protocol::ProtocolLib;
pub mod assembly {
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};
Copy link
Contributor

Choose a reason for hiding this comment

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

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

pub use miden_assembly::{
Assembler,
DefaultSourceManager,
Expand Down
8 changes: 4 additions & 4 deletions crates/miden-standards/src/account/interface/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,7 @@ fn test_custom_account_custom_notes() {
end
";
let note_script = CodeBuilder::default()
.with_dynamically_linked_library(account_component.component_code())
.with_dynamically_linked_library(account_component.component_code().clone())
.unwrap()
.compile_note_script(compatible_source_code)
.unwrap();
Expand All @@ -488,7 +488,7 @@ fn test_custom_account_custom_notes() {
end
";
let note_script = CodeBuilder::default()
.with_dynamically_linked_library(account_component.component_code())
.with_dynamically_linked_library(account_component.component_code().clone())
.unwrap()
.compile_note_script(incompatible_source_code)
.unwrap();
Expand Down Expand Up @@ -571,7 +571,7 @@ fn test_custom_account_multiple_components_custom_notes() {
end
";
let note_script = CodeBuilder::default()
.with_dynamically_linked_library(custom_component.component_code())
.with_dynamically_linked_library(custom_component.component_code().clone())
.unwrap()
.compile_note_script(compatible_source_code)
.unwrap();
Expand Down Expand Up @@ -609,7 +609,7 @@ fn test_custom_account_multiple_components_custom_notes() {
end
";
let note_script = CodeBuilder::default()
.with_dynamically_linked_library(custom_component.component_code())
.with_dynamically_linked_library(custom_component.component_code().clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

One of the motivations is that Library struct will go away soon

Ah, I didn't know. What would APIs like Assembler::link_dynamic_library take instead of Library?

I think my main question is whether we need AccountComponentCode to be convertible into libraries.

I think we need to be able to pass an AccountComponentCode to CodeBuilder::link_dynamic_library or its future equivalent. As long as we can do that, reasonably efficiently, I think that's fine.

AFAICT, this is only used in test code

True. I brought this up because my understanding was that CodeBuilder is also used in non-test code, outside of this repo, and the motivation was to avoid the clones in those non-test use cases.

.unwrap()
.compile_note_script(incompatible_source_code)
.unwrap();
Expand Down
Loading
Loading