Move the test suite helpers to Mockchain

[greenhat]

As we've discussed on the call, we should first explore improving the MockChain/MockChainBuilder before trying to build something on top of it. The low-hanging fruits I see here are:

1. Moving the compile_rust_package to the miden-testing crate but using the (installed via midenup) cargo-miden binary to compile the packages. Because we would rather not introduce the compiler in the crate dependencies there and also we need to support multiple installed toolchains.
2. Integrating an equivalent of build_existing_basic_wallet_account_builder into the MockChainBuilder.
3. Making the note construction easier. See create_note_from_package that might be implemented in the miden-testing crate using a builder pattern.
4. Moving (and refactoring) other generic helpers in tests/integration-network/src/mockchain/helpers.rs. to the miden-testing crate.

Originally posted by @greenhat in #896 (comment)

[greenhat]

@lima-limon-inc Please comment on this issue so I could assign it to you.

[greenhat]

The integration tests in the https://github.com/0xMiden/project-templates should use the new compile_rust_package using cargo-miden installed by the midenup.

[lima-limon-inc]

Noted! I'll get on it. If I'm not mistaken, I'll have to make a PR in miden-base in order to modify miden-testing.

[PhilippGackstatter]

1. Moving the compile_rust_package to the miden-testing crate but using the (installed via midenup) cargo-miden binary to compile the packages. Because we would rather not introduce the compiler in the crate dependencies there and also we need to support multiple installed toolchains.

I'm not sure what this will look like, but if these are compiler specifics, isn't this better suited to be an extension trait of MockChainBuilder instead of integrating the need for midenup into miden-testing (but maybe I'm imagining the wrong thing)?

2. Integrating an equivalent of build_existing_basic_wallet_account_builder into the MockChainBuilder.

It would be nice to come up with an API that lets users add accounts and notes built from packages while keeping the API a bit more generally useful. For example, looking at 0xMiden/protocol#2502 WalletConfig::with_basic_wallet seems like a very specific need from the compiler and I'm not sure it should be part of the MockChainBuilder API.

Could an alternative be to require users to implement types for their account components (which I think they'll need to do anyway?) instead of using packages and init storage data in the mock chain builder directly? We can support this at the MockChainBuilder with a convenient "account from components" function or something of that sort:

impl MockChainBuilder {
    fn add_existing_account_from_components(
        &mut self,
        auth: Auth,
        components: impl IntoIterator<Item = impl Into<AccountComponent>>,
    ) -> anyhow::Result<Account> {
        let mut account_builder =
            Account::builder(rand::rng().random()).storage_mode(AccountStorageMode::Public);

        for component in components {
            account_builder = account_builder.with_component(component);
        }

        self.add_account_from_builder(auth, account_builder, AccountState::Exists)
    }
}

// User type under test
pub struct MyWallet { ... }

impl From<MyWallet> for AccountComponent {
  fn from(wallet: MyWallet) -> Self {
      let package = todo!();
      let init_storage_data = todo!();
      AccountComponent::from_package(package, init_storage_data).expect("todo")
  }
}

fn test() {
    let mut builder = MockChainBuilder::default();
    // Users can easily add or omit BasicWallet.
    builder.add_account_from_components([BasicWallet, MyWallet])
}

In any case, I think it would be good to discuss this before implementing.

[lima-limon-inc]

Thanks for response @PhilippGackstatter!

I'm not sure what this will look like, but if these are compiler specifics, isn't this better suited to be an extension trait of MockChainBuilder instead of integrating the need for midenup into miden-testing (but maybe I'm imagining the wrong thing)?

If I'm not mistaken, this means calling the cargo-miden directly inside the rust code, i.e.: ~std::process::Command::new("cargo miden") (more specifically, in the case of midenup's interface, this would really call the miden build command).
I don't really have any thoughts regarding the extension trait approach; I think it can aid in the refactor while keeping the functionality local to the compiler. Having said that, I believe we will eventually want for that trait to be re-exported in the compiler's sdk in order for users to test their own packages, right? .

2. Integrating an equivalent of build_existing_basic_wallet_account_builder into the MockChainBuilder.

It would be nice to come up with an API that lets users add accounts and notes built from packages while keeping the API a bit more generally useful. For example, looking at 0xMiden/protocol#2502 WalletConfig::with_basic_wallet seems like a very specific need from the compiler and I'm not sure it should be part of the MockChainBuilder API.

Could an alternative be to require users to implement types for their account components (which I think they'll need to do anyway?) instead of using packages and init storage data in the mock chain builder directly? We can support this at the MockChainBuilder with a convenient "account from components" function or something of that sort:
(...)

// User type under test
pub struct MyWallet { ... }

impl From<MyWallet> for AccountComponent {
  fn from(wallet: MyWallet) -> Self {
      let package = todo!();
      let init_storage_data = todo!();
      AccountComponent::from_package(package, init_storage_data).expect("todo")
  }
}

fn test() {
    let mut builder = MockChainBuilder::default();
    // Users can easily add or omit BasicWallet.
    builder.add_account_from_components([BasicWallet, MyWallet])
}

This looks really nice and malleable! Having this separation really frees things, up. I'm particularly fond of:

    builder.add_account_from_components([BasicWallet, MyWallet])

I'm still thinking if there's a similar approach to take regarding notes.

[greenhat]

Thanks for response @PhilippGackstatter!

I'm not sure what this will look like, but if these are compiler specifics, isn't this better suited to be an extension trait of MockChainBuilder instead of integrating the need for midenup into miden-testing (but maybe I'm imagining the wrong thing)?

If I'm not mistaken, this means calling the cargo-miden directly inside the rust code, i.e.: ~std::process::Command::new("cargo miden") (more specifically, in the case of midenup's interface, this would really call the miden build command). I don't really have any thoughts regarding the extension trait approach; I think it can aid in the refactor while keeping the functionality local to the compiler.

Yes, I was thinking of just running miden build and printing the installation instruction for the user if it's not installed.

Having said that, I believe we will eventually want for that trait to be re-exported in the compiler's sdk in order for users to test their own packages, right? .

No. SDK is for the on-chain code consumption only. The testing should be done using the mockchain.

[PhilippGackstatter]

If I'm not mistaken, this means calling the cargo-miden directly inside the rust code, i.e.: ~std::process::Command::new("cargo miden") (more specifically, in the case of midenup's interface, this would really call the miden build command).

Calling a command sounds really brittle since there's no compile-time support to check if that tool exists, if it's called correctly, etc. So I think it would be good to avoid introducing this into the MockchainBuilder if there's another way. This seems related to the other point I mentioned: The mock chain builder would probably best just take an AccountBuilder (via add_account_from_builder) or a list of AccountComponents and merge it into an account (= add_account_from_components), so it doesn't have to deal with all the complexity of compilation, package instantation and InitStorageData. Similar to how it currently doesn't deal with MASM code at all but rather stronger types like BasicWallet. It does have some convenience methods for P2ID notes, basic wallets, etc., but if that is desired, I think an extension trait is basically necessary.

I think we may want to remove all create* methods from the MockChainBuilder because the distinction between create* and add* was too confusing, just hasn't been a priority. So at that point, we'd have to construct accounts differently (including ones created from compiled code), and so integrating this deeply into the MockChainBuilder or MockChain may not make sense and needs to happen outside anyway.

I'm still thinking if there's a similar approach to take regarding notes.

For now, MockChainBuilder::add_output_note(&mut self, note: impl Into<OutputNote>) is the generic function. We're also planning to improve this with an actual trait for notes and we're currently step-by-step introducing proper types for standard notes like P2idNote, SwapNote, etc. This might eventually lead to an API roughly like builder.add_output_note(P2idNote::new().method().build()).

In general, I would recommend guiding users in the direction of creating types for both account components (e.g. BasicWallet) and notes (e.g. P2idNote) because matching the pattern of miden-standards will breed readability and consistency, integrate well with the interfaces we (will) have, and does not require integrating compilation into miden-testing or mock chain.

[greenhat]

2. Integrating an equivalent of build_existing_basic_wallet_account_builder into the MockChainBuilder.

It would be nice to come up with an API that lets users add accounts and notes built from packages while keeping the API a bit more generally useful. For example, looking at 0xMiden/protocol#2502 WalletConfig::with_basic_wallet seems like a very specific need from the compiler and I'm not sure it should be part of the MockChainBuilder API.

Great point! I greatly appreciate your input on the design. Those types and functions started their way in the compiler integration test suite and I did NOT design them for our users. Then they "leaked" in some form into the tutorials and the new project template.

We should redesign them with our users needs in mind and test in https://github.com/0xMiden/project-template/blob/9249de8f4f009acc4e43ceaaf880d796988ad063/integration/tests/counter_test.rs?plain=1 the new project template. The compiler should not have specific needs in this API. Whatever works for the users will do fine for the compiler.

Could an alternative be to require users to implement types for their account components (which I think they'll need to do anyway?) instead of using packages and init storage data in the mock chain builder directly? We can support this at the MockChainBuilder with a convenient "account from components" function or something of that sort:

impl MockChainBuilder {
    fn add_existing_account_from_components(
        &mut self,
        auth: Auth,
        components: impl IntoIterator<Item = impl Into<AccountComponent>>,
    ) -> anyhow::Result<Account> {
        let mut account_builder =
            Account::builder(rand::rng().random()).storage_mode(AccountStorageMode::Public);

        for component in components {
            account_builder = account_builder.with_component(component);
        }

        self.add_account_from_builder(auth, account_builder, AccountState::Exists)
    }
}

// User type under test
pub struct MyWallet { ... }

impl From<MyWallet> for AccountComponent {
  fn from(wallet: MyWallet) -> Self {
      let package = todo!();
      let init_storage_data = todo!();
      AccountComponent::from_package(package, init_storage_data).expect("todo")
  }
}

fn test() {
    let mut builder = MockChainBuilder::default();
    // Users can easily add or omit BasicWallet.
    builder.add_account_from_components([BasicWallet, MyWallet])
}

In any case, I think it would be good to discuss this before implementing.

I love it! Thank you!

[greenhat]

If I'm not mistaken, this means calling the cargo-miden directly inside the rust code, i.e.: ~std::process::Command::new("cargo miden") (more specifically, in the case of midenup's interface, this would really call the miden build command).

Calling a command sounds really brittle since there's no compile-time support to check if that tool exists, if it's called correctly, etc. So I think it would be good to avoid introducing this into the MockchainBuilder if there's another way. This seems related to the other point I mentioned: The mock chain builder would probably best just take an AccountBuilder (via add_account_from_builder) or a list of AccountComponents and merge it into an account (= add_account_from_components), so it doesn't have to deal with all the complexity of compilation, package instantation and InitStorageData. Similar to how it currently doesn't deal with MASM code at all but rather stronger types like BasicWallet. It does have some convenience methods for P2ID notes, basic wallets, etc., but if that is desired, I think an extension trait is basically necessary.

I was thinking that it would remain a free function (compile_rust_package or similar). I never thought of adding it to the MockchainBuilder as a method.

[lima-limon-inc]

I've just pushed a first draft of this approach:

(...)

// User type under test
pub struct MyWallet { ... }

impl From<MyWallet> for AccountComponent {
  fn from(wallet: MyWallet) -> Self {
      let package = todo!();
      let init_storage_data = todo!();
      AccountComponent::from_package(package, init_storage_data).expect("todo")
  }
}

fn test() {
    let mut builder = MockChainBuilder::default();
    // Users can easily add or omit BasicWallet.
    builder.add_account_from_components([BasicWallet, MyWallet])
}

I've only encountered a slight problem regarding the add_account_from_components function: since CustomWallet and BasicWallet are different types, they can't be placed in the same array. They either have to be boxed or be turned into an AccountComponent before being placed in the array, here's the code. If I'm not mistaken the:

    builder.add_account_from_components([BasicWallet, MyWallet])

syntax is not possible with this setup, however it doesn't feel like a dealbreaker in my opinion. In that commit I'm also using @PhilippGackstatter 's add_existing_account_from_components method.

Regarding the compilation of a project, I created a small trait in order for a struct to indicate that was built from a project, here's the link.. This would replace the compile_rust_package function.

[PhilippGackstatter]

builder.add_account_from_components([BasicWallet, MyWallet])
syntax is not possible with this setup, however it doesn't feel like a dealbreaker in my opinion. In that commit I'm also using @PhilippGackstatter 's add_existing_account_from_components method.

Ah, my bad. Maybe it's best to change it to AccountComponent then:

fn add_existing_account_from_components(
        &mut self,
        auth: Auth,
        components: impl IntoIterator<Item = AccountComponent>,
    ) -> anyhow::Result<Account> {

And then it could still be relatively concise:

builder.add_account_from_components([BasicWallet.into(), MyWallet.into()])

We will have to make some decisions in that method, e.g. what account type to use by default or what storage mode, but maybe that's fine. We could add add_faucet_from_components for AccountType::*Faucet. And if users need more flexibility, they need to "fall back" to AccountBuilder + add_account_from_builder.

[lima-limon-inc]

Ah, my bad. Maybe it's best to change it to AccountComponent then:

fn add_existing_account_from_components(
        &mut self,
        auth: Auth,
        components: impl IntoIterator<Item = AccountComponent>,
    ) -> anyhow::Result<Account> {

And then it could still be relatively concise:

builder.add_account_from_components([BasicWallet.into(), MyWallet.into()])

Noted! I'll make those changes accordingly.

Regarding the third point from @greenhat's comment:

3. Making the note construction easier. See create_note_from_package that might be implemented in the miden-testing crate using a builder pattern.

Inside the miden-testing crate there exists a NoteBuilder struct which is used to create notes. It currently only supports note creation via a SourceManager, so I added this bit of code in order to enable note creation from Packages as well. I believe this should remove the need to have create_note_from_package in the compiler's testing code.