Stateless account compatibility checker

[Fumuran]

This PR implements a checker which can tell whether some note could be consumed by the account with specified code.

Corresponding issue: #831

[bobbinth]

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

Also, not for this PR, but we should add something like build_send_note_procedure() to AccountInterface. It could look something like this:

impl AccountInterface {
    pub fn build_send_notes_procedure(
        &self,
        notes: &[PartialNote], // or maybe an iterator of Into<PartialNote>
    ) -> Result<String, SomeError> {
        ...
    }
}

Let's create an issue for this.

[bobbinth]

Why is this constructor needed?

[Fumuran]

I use it in the tests while creating the custom account interface. I need to know the library path of this component to be able to call it in the note script (here is an import). As far as I know when we are creating a component library without specifying a path, it has a default #anon path, so we can't import anything from this library.

[PhilippGackstatter]

I agree that I'm not sure this constructor is needed. This could also be done by compiling the library in the code that needs it and then using AccountComponent::new, which allows passing a Library. Would that work?

[Fumuran]

The issue is that we don't have a public procedure in assembler which will allow assembling a library with some specified path, the only way to do so is to compile modules with paths separately and then provide them to the library assembling procedure.

In other words, to do as you said I essentially need to move the internals of my compile_with_path procedure to the place in code where I create a library, which looks very user unfriendly and requires to have a lot of additional imports (that's why I created this procedure).

IMO the best option is to create a new public procedure in the Assembler similar to assemble_library, but with the ability to provide the library path (currently assemble_library provides None as a path (here))

[PhilippGackstatter]

I agree. I stumbled on the lack of this API a couple of times before as well, so I think this would warrant creating an issue in miden-vm.

This is one example:

miden-base/crates/miden-tx/src/tests/mod.rs

Lines 917 to 927 in 6d82ff2

	let source_manager = Arc::new(DefaultSourceManager::default());
	let external_library_module = Module::parser(ModuleKind::Library)
	.parse_str(
	LibraryPath::new("external_library::external_module").unwrap(),
	EXTERNAL_LIBRARY_CODE,
	&source_manager,
	)
	.unwrap();
	let external_library = TransactionKernel::assembler()
	.assemble_library([external_library_module])
	.unwrap();

[Fumuran]

Created an issue: 0xPolygonMiden/miden-vm#1673

[PhilippGackstatter]

Should we remove the constructor for now so we don't add it to the public API when merging this PR? If you need it in the tests, you can conveniently implement it in test code as an extension trait, e.g.

trait AccountComponentExt {
    pub fn compile_with_path(
      source_code: impl ToString,
      assembler: Assembler,
      storage_slots: Vec<StorageSlot>,
      library_path: LibraryPath,
  ) -> Result<Self, AccountError>;
}

impl AccountComponentExt for AccountComponent { ... }

[bobbinth]

Would Library::from_dir() not work here?

[Fumuran]

I'm not sure that it will be convenient to use from_dir() here: it requires the code to be stored in some directory, but we use compile_with_path() procedure with the code which is provided as a plain string.

[PhilippGackstatter]

Looks good! I left some initial comments, but I have to take a second look later.

AccountComponentInterface being an enum rather than a trait feels a bit rigid and I'm not sure if it is user-extensible enough.

At least for our well-known account components (wallet, faucet, rpo falcon auth) we could reconstruct the types we have (BasicWallet, BasicFungibleFaucet, RpoFalcon512) from the code and storage of an account, since they do not contain the code (Library) but only storage. The internal representation could just be a BTreeSet<Digest> for each component, but the public interface (e.g. for the client) could maybe be something like:

let faucet_component: BasicFungibleFaucet = account_interface.try_get_component::<BasicFungibleFaucet>()?;

where BasicFungibleFaucet implements some TryFrom<&[Word]> or similar, which would attempt to reconstruct the token symbol, decimals and max supply from the given storage slots. If it succeeds, the component is part of the interface, if not, the component is not part of the interface.

That would be a more unified interface for users of the crate since they would only work with account components in construction and inspection of accounts, but this approach would have to be thought through more.

For now I think the direction is fine, maybe this is something to look into for the future if desired.

[PhilippGackstatter]

Nit: Can we move this below where we use it, so that all components (wallet, faucet, rpo falcon) are logically grouped together?

[PhilippGackstatter]

Does this happen to work because we only call account procedures but kernel procedures are syscalled? So the branches we collect will only contain procedures that are called on the account?
Would be great to add a comment explaining this, i.e. what guarantees it relies on.

I suppose that works for now, but also feels fragile.

[PhilippGackstatter]

Should we add this to the doc comment?

/// This check relies on the fact that account procedures are the only procedures that are `call`ed
/// from note scripts, while kernel procedures are `sycall`ed.

[Fumuran]

Ah, right, for sure we should. Sorry, I forgot to update the corresponding comment. Thank you!

[PhilippGackstatter]

I think I'm struggling a bit to understand this, specifically the calls to component_proc_digests. Why do we need the distinction between custom and non-custom procedures? Can we not simplify this to always include the standard procedures if they are actually present?

If I rewrite it like this, the compatibility tests also pass, and this seems simpler to read:

    if is_basic_wallet_note
        && self.component_interfaces.contains(&AccountComponentInterface::BasicWallet)
    {
        return CheckResult::Yes;
    }

    verify_note_script_compatibility(
        note.script(),
        component_proc_digests(self.components(), false),
    )
}

[Fumuran]

My initial thought was to slightly optimize this check by handling the state where we have a basic wallet note and the account have only incompatible interfaces (faucet and/or rpo falcon). Essentially that's what I'm checking on the line 140: if the aggregation of the non-standard interface digests resulted in an empty vector (which was cheap in that case), that means that we don't need to run the expensive verify_note_script_compatibility procedure and can already return No. If the vector is not empty, that means that we have to run the verify_note_script_compatibility anyway, but we already have the required interface procedure digests vector.

Not sure that the benefit of this optimization is big enough to make code much more complicated (your code is much more simple and readable, I agree), but that was my motivation.

[PhilippGackstatter]

I think what I don't understand is the logic with the empty vector. If we are in the is_basic_wallet_note branch, and the component interfaces do not contain the basic wallet, then we can immediately say no, right? I don't see why we need the empty vec check.

So is this not the same?

if is_basic_wallet_note {
    if self.component_interfaces.contains(&AccountComponentInterface::BasicWallet) {
        return NoteAccountCompatibility::Yes;
    } else {
        return NoteAccountCompatibility::No;
    }
}

verify_note_script_compatibility(note.script(), component_proc_digests(self.components()))

And then we can get rid of the bool argument in component_proc_digests, afaict.

Edit: Okay, I see, it's because P2ID for example doesn't require the entire basic wallet interface but only receive_asset, so it could still be compatible. It would be great if we could document this in this check here because that is not obvious.

[PhilippGackstatter]

In that case I would rewrite it like that:

    fn contains_non_standard_interface(&self) -> bool {
        self.component_interfaces
            .iter()
            .any(|interface| matches!(interface, AccountComponentInterface::Custom(_)))
    }

    /// checks if a note can be consumed against the current [AccountInterface] instance.
    pub fn can_consume(&self, note: &Note) -> NoteAccountCompatibility {
        let basic_wallet_notes = [p2id_commitment(), p2idr_commitment(), swap_commitment()];
        let is_basic_wallet_note = basic_wallet_notes.contains(&note.script().hash());

        if is_basic_wallet_note {
            if self.component_interfaces.contains(&AccountComponentInterface::BasicWallet) {
                return NoteAccountCompatibility::Yes;
            }

            if !self.contains_non_standard_interface() {
                return NoteAccountCompatibility::No;
            }
        }

        verify_note_script_compatibility(note.script(), component_proc_digests(self.components()))
    }

Would be great to document why we do this non standard check and move the matches! in a method on AccountComponentInterface.

It would also be good to test this case specifically where an account with a non-standard interface contains receive_asset and its compatibility is checked against P2ID for example. When I rewrote the code with my previous suggestion, all tests passed, but one test should be there to make sure this works.

[Fumuran]

Agreed! I'll rewrite this check as you suggested and add comments and dedicated test.

[PhilippGackstatter]

We should document what "Yes" and "No" really mean. If I have a swap note and my account is a basic wallet, it will return yes, but it doesn't mean I have enough of the swap note's requested asset in my account to actually consume it. So limitations like these should be documented.

Due to that, I'm also wondering if the distinction between Yes and Maybe is meaningful enough? Basically, Yes is also just Maybe, because of the above, so reducing this check to answer the question "is this note script compatible with this account interface?" with a compatible/incompatible answer, seems basically the same to me and a bit more straightforward to work with.

[Fumuran]

We should document what "Yes" and "No" really mean. If I have a swap note and my account is a basic wallet, it will return yes, but it doesn't mean I have enough of the swap note's requested asset in my account to actually consume it. So limitations like these should be documented.

Agreed, I added a more detailed doc comments for each variant. But also see my comment below.

Due to that, I'm also wondering if the distinction between Yes and Maybe is meaningful enough? Basically, Yes is also just Maybe, because of the above, so reducing this check to answer the question "is this note script compatible with this account interface?" with a compatible/incompatible answer, seems basically the same to me and a bit more straightforward to work with.

I'm totally agree with you, I was struggling with this question at first, not being able to guarantee that a note will be consumed without actually trying to consume it. That's the main point of separating stateless check with stateful check. First (which I am implementing now) will show whether the note could be consumed (or, actually, is it compatible with an account interface, as you correctly pointed) by some account interface, and at that point the only things we know are note and account interfaces (so we don't have a full account state, that's why it is stateless). And in terms of the stateless check if we try to consume, for example, a swap note with a basic wallet, it should return Yes. In other words, we use Yes to guarantee that the note script is correct (since it is a basic note script and we now it is correct), so potentially during the stateful check we will be able to make sure this note will be consumed without executing it. So we need this distinction for the optimization during the stateful check.

Situation which you describe in the first quote will be checked during the stateful check, where we will be able to verify the incoming and outgoing assets balance, note inputs and so on (you can ask why don't we check the note inputs here, since we have a full note and the interfaces against it will be consumed. Well, the line between stateless and stateful sometimes could be faint, let's just say that we decided to leave it for the stateful check. I think they will be used together anyway).

[PhilippGackstatter]

Thanks for the overview!

Personally, I would make the stateless check just about note-account interface compatibility. I think the only note type that we have, for which we can say for sure if it can be consumed after this check is the P2ID note, because there are no branches. P2IDR or SWAP require stateful checks and I assume most notes will have some branching and/or will not be known to this check directly anyway. So simplifying this on a conceptual level into an interface check and a stateful check would be preferable I think.

[PhilippGackstatter]

Same here, can we add burn?

[PhilippGackstatter]

Can you add 1-2 sentence comments explaining the purpose of the test, what result it expects and why?

[PhilippGackstatter]

We should be able to add call.test_account::procedure_1 in the else branch without changing the test outcome, right?

[PhilippGackstatter]

This test is supposed to check that when multiple components are present, the result is "maybe" if the branches contain procedures from the account's components, right? Shouldn't we then have procedures from multiple components in all branches, to not make this test "too easy", so to say?
So basically, can we add more valid calls to procedures in the else branch to make the test more complex? Otherwise it seems similar to the previous test.

[PhilippGackstatter]

Is the usage of MockChain essential in these tests? It feels like these should be unit tests rather than integration tests. Could we not build accounts using AccountBuilder and interpret some random word as an RPO falcon public key?
The tests should also become much faster then, if we can avoid generating an RPO falcon secret key from entropy (which is what MockChain::add_existing_wallet does if Auth is set), which takes 8 seconds in debug mode on my machine for a single test.

[Fumuran]

Hm, that makes sense, I'll try to rework the tests to avoid using MockChain.

[Fumuran]

Also, not for this PR, but we should add something like build_send_note_procedure() to AccountInterface.
Let's create an issue for this.

Created an issue: #1180

[PhilippGackstatter]

Thanks for extending the tests and describing the supported/unsupported procedures. Definitely helpful while reviewing 🙏.

[PhilippGackstatter]

I think we could implement Copy for AccountProcedureInfo since it's just 40 bytes.

[PhilippGackstatter]

the correctness of the note script is guaranteed

What does that mean? We can't actually guarantee "correctness" of the note script, depending on the definition of correctness. But I would probably remove that part of the sentence.

[Fumuran]

Since we can have Yes result only in case we are using basic note scripts (P2ID, P2IDR and SWAP), we know their code for sure and can guarantee that this code is correct, since it is our own code. But I'll remove this sentence anyway, since it confuses more than it helps.

[PhilippGackstatter]

I think instead of correctness I would write it something like

"The account has all necessary procedures of one execution branch of the note script. This means the note may be able to be consumed by the account if that branch is executed."

[PhilippGackstatter]

Nit: The &mut is unnecessary.

[Fumuran]

It would also be good to test this case specifically where an account with a non-standard interface contains receive_asset and its compatibility is checked against P2ID for example.

This test revealed a bug, when we were skipping procedures from the basic interfaces even if these interfaces was not actually used and this (reexported) proc was just a part of some custom interface. Thank you!

[PhilippGackstatter]

Looks good to me.

I left some comments re docs and code simplification.

Compatibility of Yes and Maybe

I'm still of the opinion that the distinction between "Yes" and "Maybe" is not meaningful enough. For example, if we inspect the compatiblity of an account with BasicWallet and a P2ID note, then the answer is "Yes".
If the account has only the receive_asset procedure, which is actually sufficient for P2ID, then compatibility with a P2ID note will be "Maybe". Which is just a quirk of the code, but making it return "Yes" would just add more complexity. Ultimately, I believe these answers will be treated the same anyway: Both will need a subsequent "stateful check", so the distinction doesn't seem worth the additional code that is already there.
I think I'm just a fan of solutions that are general, like the verify_note_script_compatibility approach, which gives a useful answer for all account <-> note interfaces - just to explain where I'm coming from.
So my preference would be to have this just be an interface compatibility check, which says yes, there is at least one possible execution branch, or no there isn't.

Account Component Abstraction

This would also mean we wouldn't need enum representation of an account component interface like BasicWallet, RpoFalcon512, etc. Instead we could treat it as just a set of procedures.
I think this would play nicely with the approach I mentioned before (#1171 (review)). For any user-supplied component, as well as the standard ones, we can implement something like (and this is a very rough idea):

// TODO: Better name.
pub trait AccountComponentTrait: TryFrom<PartialAccountStorage, Error = Box<dyn Error>> {
  fn library() -> Library;
}

pub struct PartialAccountStorage { ... }

impl TryFrom<PartialAccountStorage> for RpoFalcon512 {
    type Error = &'static str;

    fn try_from(storage: PartialAccountStorage) -> Result<Self, Self::Error> {
        if let Some(public_key) = storage.get_item(RpoFalcon512::PUBLIC_KEY_OFFSET) {
          Ok(RpoFalcon512::new(public_key))
        } else {
          return Err("missing public key at storage slot 0")
        }
    }
}

and on the AccountInterface:

pub fn try_get_component<C: AccountComponentTrait>(&self) -> Result<C, &'static str> {
      // - check if self contains all of the component's procedures (i.e. C::library())
      // - construct a partial account storage from all storage slots referenced by the component's procedure infos
      // - Call C::try_from(partial_storage)
    }

How exactly we deal with storage would be the main question with this (store it in account interface, or pass a partial or full version of it in, etc.).

The main point of this is that users have a unified way to reconstruct the account components given an Account or a partial view of it. Plus all components are treated equally and the standard ones don't have special logic, e.g.:

let faucet_component: BasicFungibleFaucet = account_interface.try_get_component()?;
let custom_component: MyComponent = account_interface.try_get_component()?;

We can provide all of the convenience we want still through the existing account component types we have as well as things like AccountInterface::is_basic_wallet or AccountInterface::try_get_basic_wallet() -> Result<BasicWallet, _>.

That would be a nice interface I think, but it's also possible I have a different goal in mind than what was the original intention.

This PR

I think we should deal with the first point in this PR, i.e. decide whether Yes/Maybe should stay, or just Yes. The other point can be a follow-up one way or another if you find the idea has merits.

[PhilippGackstatter]

Suggested change

	/// Returns a boolean flag whether the custom interfaces are used by the reference account.
	/// Returns a boolean flag indicating whether at least one custom interface is used in the reference account.

[PhilippGackstatter]

Suggested change

	fn from(value: &Account) -> Self {
	fn from(account: &Account) -> Self {

Nit

[PhilippGackstatter]

Technically, we could just link to miden_lib::account::wallets::BasicWallet, miden_lib::account::faucets::BasicFungibleFaucet and miden_lib::account::auth::RpoFalcon512 here, so we don't have to define the exposed methods again.

[Fumuran]

Indeed, that would be better!

[PhilippGackstatter]

Suggested change

	/// Exposes the procedures vector specified by its internal value.
	Custom(Vec<AccountProcedureInfo>),
	/// A non-standard, custom interface which exposes the contained procedures.
	Custom(Vec<AccountProcedureInfo>),

Nit: Maybe add a brief description.

[PhilippGackstatter]

I think this is fairly hard to read, so maybe we could use a different approach? What do you think about something like this?

pub fn from_procedures(procedures: &[AccountProcedureInfo]) -> BTreeSet<Self> {
    let mut component_interface_set = BTreeSet::new();

    let mut procedures: BTreeMap<_, _> = procedures
        .into_iter()
        .map(|procedure_info| (*procedure_info.mast_root(), procedure_info))
        .collect();

    // Basic Wallet
    // ------------------------------------------------------------------------------------------------

    if basic_wallet_library()
        .mast_forest()
        .procedure_digests()
        .all(|proc_digest| procedures.contains_key(&proc_digest))
    {
        basic_wallet_library().mast_forest().procedure_digests().for_each(
            |component_procedure| {
                procedures.remove(&component_procedure);
            },
        );

        component_interface_set.insert(AccountComponentInterface::BasicWallet);
    }

    // Basic Fungible Faucet
    // ------------------------------------------------------------------------------------------------

    if basic_fungible_faucet_library()
        .mast_forest()
        .procedure_digests()
        .all(|proc_digest| procedures.contains_key(&proc_digest))
    {
        basic_fungible_faucet_library().mast_forest().procedure_digests().for_each(
            |component_procedure| {
                procedures.remove(&component_procedure);
            },
        );

        component_interface_set.insert(AccountComponentInterface::BasicFungibleFaucet);
    }

    // RPO Falcon 512
    // ------------------------------------------------------------------------------------------------

    let rpo_falcon_proc = rpo_falcon_512_library()
        .mast_forest()
        .procedure_digests()
        .next()
        .expect("rpo falcon 512 component should export exactly one procedure");

    if let Some(proc_info) = procedures.remove(&rpo_falcon_proc) {
        component_interface_set
            .insert(AccountComponentInterface::RpoFalcon512(proc_info.storage_offset()));
    }

    // Custom interfaces
    // ------------------------------------------------------------------------------------------------

    let mut custom_interface_procs_map = BTreeMap::<u8, Vec<AccountProcedureInfo>>::new();
    procedures.into_iter().for_each(|(_, proc_info)| {
        match custom_interface_procs_map.get_mut(&proc_info.storage_offset()) {
            Some(proc_vec) => proc_vec.push(*proc_info),
            None => {
                custom_interface_procs_map.insert(proc_info.storage_offset(), vec![*proc_info]);
            },
        }
    });

    if !custom_interface_procs_map.is_empty() {
        for proc_vec in custom_interface_procs_map.into_values() {
            component_interface_set.insert(AccountComponentInterface::Custom(proc_vec));
        }
    }

    component_interface_set
}

Ideally, we'd use a HashMap here which doesn't require PartialOrd and doesn't rebalance upon removal, but that can be optimized later, if at all necessary.

[Fumuran]

I was thinking about this approach too, but wanted to avoid allocating potentially large map. But now I think that this approach will be better: account interface shouldn't be that big, and benefits from more simple and readable component interface set creation are obvious. I'll try to implement this version.

[PhilippGackstatter]

Yeah, I wanted to suggest using a map before as well but didn't because the allocation wasn't strictly necessary. However, I think for readability it does make sense.

[bobbinth]

I like this approach, but similar to one of my other comments, I wonder if it would make sense to create something like a WellKnownAccountComponent enum and move most of the logic from here into that enum.

[Fumuran]

I'm not sure that creation of the WellKnownAccountComponent will be as convenient and beneficial as WellKnownNote enum, because we can't create a WellKnownAccountComponent instance from the AccountProcedureInfo array, it's more like "is this well known component could be constructed from this array". We could create a procedure which will return an array of well known components which could be constructed from this array, but it won't really help us to construct custom components: we still have to create a procedures map and remove procs which are used in basic components. Potentially we could move the entire logic to this new enum, but this will require to add a Custom interface to it, so it will lose its original point.

IMO creation of another enum with basic components is not worth it, it will bring more confusion than benefit.

[PhilippGackstatter]

Suggested change

	///
	/// If `only_custom` flag set to `true`, returns digests of custom interfaces only, ignoring all
	/// other types.

[PhilippGackstatter]

Should we add this to the doc comment?

/// This check relies on the fact that account procedures are the only procedures that are `call`ed
/// from note scripts, while kernel procedures are `sycall`ed.

[PhilippGackstatter]

It doesn't really make sense to me to implement ordering for AccountProcedureInfo. It doesn't seem like account procedures have a natural order. This is just a side-effect of needing it for being able to put AccountComponentInterface in a BTreeSet, which itself doesn't seem to have a natural order.

Overall, I would probably prefer implementing Hash and using a Hash{Set,Map} instead, but it's probably not worth changing at this point.

[Fumuran]

I think we can use just Vec instead of BTreeSet for component_interfaces: I don't think that the can_consume check will work that slower if we will use vector instead of set, but it will allow to get rid of this default implementations.

[PhilippGackstatter]

That's a good idea, I haven't looked into every usage of it, but it sounds like a good idea. I guess the uniqueness that BTreeSet gives us is essentially given by construction, i.e. we cannot have BasicWallet twice in the interfaces.

[bobbinth]

I agree with removing PartialOrd and Ord from here.

[PhilippGackstatter]

Should we remove the constructor for now so we don't add it to the public API when merging this PR? If you need it in the tests, you can conveniently implement it in test code as an extension trait, e.g.

trait AccountComponentExt {
    pub fn compile_with_path(
      source_code: impl ToString,
      assembler: Assembler,
      storage_slots: Vec<StorageSlot>,
      library_path: LibraryPath,
  ) -> Result<Self, AccountError>;
}

impl AccountComponentExt for AccountComponent { ... }

[bobbinth]

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

[bobbinth]

We should probably add an explanation of this to the doc comments to the function. If I understood correctly, this creates one file per MASM script file which contains the serialized commitment.

[bobbinth]

I agree with removing PartialOrd and Ord from here.

[bobbinth]

nit: maybe I'd call this filed just interfaces.

[bobbinth]

Looking at these now, it does feel like generating _commitment files is a bit of on overkill. We should be able to have these functions and just use the static variables defined above, right?

One thing I do wonder if it would make sense to create something like a WellKnownNote enum that could look something like this:

pub enum WellKnownNote {
    P2ID,
    P2IDR,
    SWAP,
}

impl WellKnownNote {
    pub fn from_note(note: &Note) -> Option<Self> {
        ....
    }

    pub fn script(&self) -> NoteScript {
        ...
    }

    pub fn script_root(&self) -> Digest {
        ...
    }

    pub fn is_consumable_by(&self, &AccountInterface) -> bool {
        ...
    }
}

This could make handling of well-known notes pretty generic (i.e., adding a new well-known note in the future would be pretty self-contained).

[bobbinth]

I like this approach, but similar to one of my other comments, I wonder if it would make sense to create something like a WellKnownAccountComponent enum and move most of the logic from here into that enum.

[bobbinth]

I agree with one of @PhilippGackstatter comments - let's get rid of this enum in favor of using simple booleans.

[PhilippGackstatter]

I'm not sure if I said that. Generally, I'm actually more in favor of using enums than booleans because the enum can be documented and is usually more readable at call-sites. If we end up using this as if interface.can_consume(...) { ... } though, then using booleans could be fine.

[bobbinth]

Ah, yes - I don't think you've actually suggested using a boolean. I think I'm fine either way - with boolean and with No/Maybe enum (and maybe enum is a bit better).

[bobbinth]

Would Library::from_dir() not work here?

[bobbinth]

I would maybe change the logic here a bit:

1. First figure out if note is a well-known note (e.g., by using a WellKnownNote enum I mentioned in another comment).
2. If it is a well-known note, check if it can be consumed via something like WellKnownNote::is_consumable_by().
3. If it isn't, run verify_note_script_compatibility() as we do now.

[PhilippGackstatter]

Should we rename this to be more aligned with what it actually expresses? Since we can't definitively say whether "a note can be consumed", I would make this answer the question "is this note compatible with this account interface?". So perhaps is_compatible, which would read like account_interface.is_compatible(note).

[bobbinth]

Yes, is_compatible() makes more sense.

[PhilippGackstatter]

Looks good to me.

[PhilippGackstatter]

Nit: I think I'd put this in a new file.

[PhilippGackstatter]

Suggested change

	P2ID_SCRIPT.clone().hash()
	P2ID_SCRIPT.hash()

I think this should work as well.

[PhilippGackstatter]

If we add ModuleKind here, then I think we can get rid of the assembly crate imports in crates/miden-lib/src/account/interface/test.rs and maybe remove it from the dev-dependencies of miden-lib.

[igamigo]

Took a quick look to see if it roughly matched how we currently use it in the client and all looks great! Leaving some minor comments as well, feel free to disregard.

[igamigo]

nit: "checks" -> "Checks". But maybe it could be written as

Suggested change

	/// checks if the provided note is compatible with the current [AccountInterface].
	/// Returns [NoteAccountCompatibility::Maybe] is the note script is compatible with the account's interface (...).

or something similar.

[igamigo]

Seems like we use various names for this: commitment, hash, and root. Within NoteScript, root and hash are used, should it be changed here so that they are aligned?

[Fumuran]

Hmm, that's a good question. As far as I remember we wanted to update the code and docs to use the commitment term, that's why I used it here. I believe that this is the corresponding issue: #1069

[igamigo]

nit: The inner value contains the absolute storage index, right? In that case, instead of "offset" I think I'd just name it as "storage index" to avoid ambiguity. "Offset" makes it sound relative to some other number/index and instead most of the account storage interface deals with absolute "storage indices" so I think it fits slightly better (ie, it is equivalent to the component's storage offset because it only holds one storage value).

[Fumuran]

(ie, it is equivalent to the component's storage offset because it only holds one storage value)

Yes, that's exactly why I started calling it as a "storage offset", but I agree, in that case "storage index" will be better.