Motivation

The MockChain::build_tx_context API and TransactionContextBuilder as a whole could be improved. One trigger for this is the issue described here. To summarize, the TransactionContextBuilder::extend_input_notes is ignored if TransactionInputs are already present in the builder, such as when using MockChain::build_tx_context. However, this is not obvious and in the linked case, the test was never testing what it was supposed to test, which is not good. I would take this as an opportunity to improve this core testing API to make such situations less likely to happen.

I propose the following changes.

Naming

I would propose renaming TransactionContext. "Context" is a very generic term and when I was getting to know this codebase it took me some time to understand what this represented. What it contains is the data necessary for executing a single transaction. The TransactionContext implements the DataStore trait, which enables the TransactionExecutor to fetch the transaction inputs and more data as the transaction executes, e.g. lazy load data. The tx context is used for running a single transaction execution, so it has a one-off nature.

Based on that, there are a few names I can think of that would fit:

• PreparedTransaction would indicate that this represents all necessary data for the execution of a transaction and would communicate that is is in a prepared but incompleted state, i.e. not executed. Here the focus is primarily that this is the state of a transaction prior to execution and that we can call execute on it to get an ExecutedTransaction. This does not capture any of the DataStore aspects.
• TestTransaction would indicate that this represents a transaction for testing. It does not fit as nicely into the existing ExecutedTransaction and ProvenTransaction types that we have.
• TransactionStore or something in that direction would be possible as well, given that it implements DataStore. I'm not sure it is the central aspect for the user, though.
• MockTransaction which is a bit generic, but would nicely fit into the existing Mock naming for MockChain and new types introduced below.

TransactionContextBuilder

The TransactionContextBuilder we have now serves two purposes:

• It is returned by MockChain::build_tx_context with pre-fetched transaction inputs from the mock chain it is called on.
• It can be constructed independently, most often with TransactionContextBuilder::with_existing_mock_account and then executed. In that case, a MockChain is created internally to generated some valid chain data.

These are very different functions it supports and is why its API is not very clear (see motivating issue). The problem there was that TransactionContextBuilder::extend_input_notes was called on a builder returned by MockChain::build_tx_context. Since build_tx_context already takes input notes and fetches the transaction inputs for those, the input notes passed to extend_input_notes are completely ignored.

Both functions are useful, but I think they should be separated into two APIs:

• A public API that can be used with a concrete MockChain instance to create a transaction.
• A miden-base internal API that allows for simple transaction creation by generating valid chain data when necessary (e.g. replacing usages like TransactionContextBuilder::with_existing_mock_account). These "independent" instantiations of TransactionContextBuilder do not seem to be used in miden-client or miden-node and so, most likely, it should be fine to make them internal only. This should also be much clearer for users: There is a single way to create a test chain and test transactions, i.e. using the mock chain.

Public API Changes

For this I'm assuming TransactionContext is renamed to MockTransaction. I would add a MockChain::build_transaction API which returns a builder type, e.g. MockTransactionBuilder which is pretty much the TransactionContextBuilder we have now with a few differences:

// This is equivalent to the current TxContextInput.
pub enum MockTransactionInput { ... }

pub struct MockTransactionBuilder<'chain> {
    chain: &'chain MockChain,
    // ...
}

impl<'chain> MockTransactionBuilder<'chain> {
    pub fn new(chain: &'chain MockChain, input: impl Into<MockTransactionInput>) -> Self { todo!() }

    pub fn authenticated_input_note(mut self, note_id: NoteId) -> Self { todo!() }
    pub fn unauthenticated_input_note(mut self, note: Note) -> Self { todo!() }
    
    // More builder-style methods.

    pub fn build(self) -> anyhow::Result<MockTransaction> { todo!() }
}

impl MockChain {
	pub fn build_transaction<'chain>(
		&'chain self,
		input: impl Into<TxContextInput>,
	) -> MockTransactionBuilder<'chain> {
		MockTransactionBuilder::new(self, input)
	}
}

• The inputs to new are a MockTransactionInput, which "dereferences" into an Account, the same way it does now. A transaction does not necessarily need to consume notes and so requiring notes isn't necessary. This is contrary to MockChain::build_tx_context which requires authenticated and unauthenticated note parameters. This is just an ergonomic improvement.
• It holds a reference to a MockChain. This is necessary so that, contrary to the current build_tx_context, it can call MockChain::get_transaction_inputs in build when all input notes are known.
• It does not have the APIs of the TransactionContextBuilder that no longer make sense, or they are repurposed. For instance, extend_input_notes would still make sense (but slightly renamed).

Usage would then look like this:

let mut builder = MockChain::builder();
let account = builder.add_existing_mock_account(Auth::IncrNonce)?;
let note0 = builder.add_public_p2any_note(account.id(), [FungibleAsset::mock(100)])?;
let note1 = builder.add_public_p2any_note(account.id(), [FungibleAsset::mock(200)])?;
let chain = builder.build()?;

let tx: ExecutedTransaction = chain
	.build_transaction(account)
	.authenticated_input_note(note0.id())
	.unauthenticated_input_note(note1)
	.build()?
	.execute_blocking()?;

Internal API Changes

To replace TransactionContextBuilder usages where we don't want to instantiate a MockChain first, we can create a second builder API similar to TransactionContextBuilder that internally builds on top of MockChain + MockTransactionBuilder. Maybe this could just be named TestTransactionBuilder.

One alternative would technically be to use the MockChain for these purposes, but we do have a lot of tests that just need some valid chain data to execute against, and don't care about the exact data. For those, having such a convenient builder makes sense.

Conclusion

The primary upside of this are improved ergonomics with the MockTransactionBuilder and a clearer to use API.

The downside is that we duplicate some builder methods like MockTransactionBuilder::authenticated_input_note and TestTransactionBuilder::authenticated_input_note, but those are trivial methods, so seems fine.

Also, overall, I'm not even sure if the client or node care too much about this change, because their usage of build_tx_context seems to be very limited (one usage of this API each) and the impact of this change on the MockChain itself is just one API change.
This relates to #1913 and how we want users to use the mock chain when the client is involved: Do they use MockRpcApi or do they use MockChain + MockChain::build_transaction? If it is sufficient for users to test against MockRpcApi, we could even make the entire MockChain::build_transaction and MockTransactionBuilder API private too.

I'm curious what you think about this! cc @igamigo @mmagician @drahnr

Lazy Loading Extension

With lazy loading, there is no need for foreign accounts to be manually passed as inputs into the TransactionExecutor. Instead, they are loaded on-demand from the DataStore. This enables the client to fetch foreign accounts from the actual chain, during transaction execution, which is very convenient. The current TransactionContext (= MockTransaction) implements DataStore but needs all foreign account inputs upfront, because it cannot fetch them from the chain.

What we could with the above changes is to pass a reference to the MockChain from the MockTransactionBuilder on to MockTransaction. That would allow it to fetch foreign account inputs from the chain as well, which would be quite nice. Note that, similar to the client, this only works for accounts whose state is public.

I'm a bit on the fence about this. On one hand, this is super convenient for users since they don't have to fetch data upfront, but on the other hand also introduces some extra complexity, which is something I usually try to avoid for testing tools.

I'm curious what you think about this. I'm assuming the client can already fetch data for foreign accounts with a MockRpcApi, whether we have this in the MockTransaction or not, so it probably doesn't really care about this.

In any case, this could be a separate follow-up to the above changes.