test: MockChain, CodeExecutor, TransactionContext proposals and general improvements

[igamigo]

• Previously, there were a series of functions created for mocking objects (such as non_fungible_faucet_2()). Most of these were moved to their own object's impl blocks. The remainders were left as helpers for specific tests, like mock_executed_transaction(), mainly because they assume too much of the use-case and wouldn't be of much help being a struct function. The kernel tests on miden-tx were moved to be a submodule of the tests module as well.

• Introduces 3 structs:

  • CodeExecutor is a helper struct for running arbitrary code on Hosts. This struct is meant to group and replace the run_* functions. It implements a fluent API for simpler use, and so now something like this:

            let process = run_within_tx_kernel(
                "",
                &code,
                StackInputs::new(vec![account_id.into()]).unwrap(),
                MemAdviceProvider::default(),
                None,
            )
            .unwrap();

  would look like this:

          let process = CodeExecutor::new_with_kernel(MemAdviceProvider::default())
              .stack_inputs(StackInputs::new(vec![account_id.into()]).unwrap())
              .run(&code)
              .unwrap();

  • MockChain was simplified and repurposed for simulating a blockchain for tests, essentially replacing mock_chain_data(). Since most mocking problems stem from not being able to simulate relations between objects correctly, I think having a good way to relate all entities in a struct can eventually solve most problems. It's not very complete but it serves for simulating blocks and transforming Notes into InputNotes for executing transactions (and exposes an API for getting valid TransactionInputs). There's a MockChainBuilder as well.
  • TransactionContext is meant to replace the mock_inputs_* family of functions. These functions assumed a specific set of use-cases, and were related to other mock objects such as the MockDataStore or functions such as mock_executed_transaction. For any time a tests would call mock_inputs(), for example, notes would be mocked and transformed into InputNote even when the tests weren't making use of notes. So the idea of TransactionContext is to encapsulate these possible use-cases but also expose a way of creating more hand-made scenarios. It also has a builder struct for simple initialization, although it's not the cleanest API/structure right now (planning to review this next alongside MockDataStore).

As a result of this, a decent amount of (sometimes duplicate) code was removed. The API is not necessarily less verbose than it was, but it hopefully makes some more sense semantically. I'm not sure about the names of these structs, or even if the structs themselves make sense, so it is all up for discussion.

[mFragaBA]

Looks good! Left some minor comments.

[mFragaBA]

we should probably assert that all the requested notes are contained within self.tx_inputs.input_notes().

[mFragaBA]

was there a reason we changed from Felt::ONE to Felt::ZERO? Was there a reason it was one instead of zero before?

[bobbinth]

Thank you! Looks good! This is not an exhaustive review (I mostly looked at CodeExecutor and TransactionContext structs) but I did leave some comments inline.

Overall, I like the idea of CodeExecutor and TransactionContext, especially for granular tests (i.e., tests which test some narrow functionality). As you've noted, I think we can improve the APIs - but this can be done in follow-up PRs.

I haven't looked too deeply into the MockChain structs, but it seems like there may be a lot of obsolete code there. On the other hand, I wonder if we should make the MockChain a more central component of higher level tests (i.e., tests which execute entire transaction flow).

For example, something like:

let chain = MockChain::new();

// this could return something like MockFungibleFaucet(Account)
let pol_faucet = chain.add_fungible_faucet("POL");

let asset = pol_faucet.mint(100);

// this would create a basic wallet account but without requiring signature
// for authentication
let alice = chain.add_wallet_account(Auth::NoAuth);

// similar to alice - but the account would be "new" (i.e., nonce=0)
let bob = chain.add_new_wallet_account(Auth::NoAuth);

// this would create a P2ID note and add it to the chain
let note = chain.add_p2id_note(&alice, &bob, asset, NoteType::OffChian);

// build transaction context for consuming note by bob's account
let tx_context = chain.build_tx_context(&bob)
    .add_input_note(note)
    .build();

let executed_tx = tx_context.execute().unwrap();

The above is just a sketch - so, could be missing some detail - but I think having something like this, would simplify higher-level tests quite a bit.

[bobbinth]

These two functions seem to be used exclusively in epilogue tests (and they always seem to be used together). I haven't thought it through yet, but I'm wondering if there is a better way of handling these tests without this general structure (could also be done in a follow-up PR).

[bobbinth]

It doesn't seems like this function is currently being used anywhere?

[bobbinth]

Why this method is needed? Shouldn't all of the properties be set via the TransactionContextBuilder?

[bobbinth]

Maybe not for this PR, but the this method seems rather confusing to me. We are creating a pre-defined set of notes in a pretty opaque way (i.e., I need to read and understand what mock_notes() function is doing and then how AssetPreservationStatus is used in various places to figure out what's going on.

We should probably refactor this to get rid of AssetPreservationStatus and potentially this method altogether.

[bobbinth]

Looks good! Thank you! As mentioned in the previous review - there are still quite a few things we can refactor/improve here, but given the amount of changes here, I think it makes sense to merge it sooner rather than later.