I think here and for TransactionKernel::prepare_inputs and in turn TransactionAdviceInputs::new it would make sense to still have a type that represents transaction inputs with notes, otherwise we just pass the notes separately.

This would be a bit cleaner, and it would also allow that type to validate the authentication paths of the input notes (what TransactionInputs::new has done so far).

So maybe having a PartialTransactionInputs and TransactionInputs type makes sense, which would be defined as described in #401 (comment).
(We could also use different names, but I can't come up with something better than TransactionInputsWithNotes and that seems less nice than the "partial" naming).

I forgot that this is also a bit related to #1852 (comment). For that issue, we may want a type that returns more data than what would go in TransactionInputs (namely the asset witnesses), and so perhaps a different name makes sense. This would mean we no longer make that type a field in TransactionInputs (which would stay as-is), but rather inline its fields, so essentially:

// What is now TransactionInputs
pub struct TransactionExecutionInputs {
    account: PartialAccount,
    block_header: BlockHeader,
    blockchain: PartialBlockchain,
    input_notes: InputNotes<InputNote>,
}

// Would be returned by `DataStore::get_transaction_inputs`
pub struct TransactionInputs {
    account: PartialAccount,
    block_header: BlockHeader,
    blockchain: PartialBlockchain,
    // Not added in this PR.
    asset_witnesses: Vec<AssetWitness>,
}

I'm wondering if we could make TransactionExecutionInputs an internal type in the future (for now we need it across miden-lib and miden-tx, but if some functionality gets merged in the future, this could be made internal and then its name wouldn't matter as much).

@bobbinth @PhilippGackstatter I am confused about the Option<AdviceInputs> vs these AdviceInputs. Right now I have the former being passed into TransactionKernelInputs::new() but I have probably done something wrong to leave this one unused.

I have left TODO statements to cover the places where there is confusion around the advice inputs

I have peeled back the diffs to iterate from a commit that passed tests. When I move the tx_args into the input type (still named TransactionExecutionInputs for now), the tests fail here.

Relevant commit causing the failures

It seems like one of the issues is that TransactionExecutionInputs contains TransactionArgs but we still build it separately in TransactionContextBuilder::build - and then we probably don't use it in certain TransactionExecutor APIs.

My gut feeling is that maybe combining TransactionPreparationInputs and TransactionArgs into a single type may not work too well because these types become available at different points in time, but we still want to have a type that encapsulates the tx inputs and input notes, and this creates an awkward situation where we construct TransactionExecutionInputs from TransactionPreparationInputs and InputNotes but are forced to also provide a TransactionArgs, which is not usually available at that time (for instance in MockChain::get_transaction_execution_inputs).

This may be solvable in two ways:

• Either we keep three types in total: TransactionPreparationInputs, TransactionExecutionInputs and TransactionArgs separately,
• Or we try to "remove" TransactionArgs from the public API and instead only expose its functionality through TransactionExecutionInputs which would now allow users to do everything that tx args did previously: set tx scripts, auth args, note args, etc.

I would try the second approach, since removing the number of types here could be beneficial to make the API clearer, but if this turns out to be too awkward to use in practice, I'd revert to the first approach.

My gut feeling is that maybe combining TransactionPreparationInputs and TransactionArgs into a single type may not work too well because these types become available at different points in time, but we still want to have a type that encapsulates the tx inputs and input notes, and this creates an awkward situation where we construct TransactionExecutionInputs from TransactionPreparationInputs and InputNotes but are forced to also provide a TransactionArgs, which is not usually available at that time (for instance in MockChain::get_transaction_execution_inputs).

I this also applicable outside of testing scenarios? My understanding is that in "production" scenarios, TransactionArgs are built before we have TransactionInputs (or TransactionExecutionInputs). If it is only for testing purposes, maybe we can make some internal state accessible which could make things simpler.

I this also applicable outside of testing scenarios?

Probably less so, yes. I extrapolated a bit too much from the tx context which is the only non-trivial user of the TransactionExecutor API we have in base.

It seems like one of the issues is that TransactionExecutionInputs contains TransactionArgs but we still build it separately in TransactionContextBuilder::build - and then we probably don't use it in certain TransactionExecutor APIs.

My gut feeling is that maybe combining TransactionPreparationInputs and TransactionArgs into a single type may not work too well because these types become available at different points in time, but we still want to have a type that encapsulates the tx inputs and input notes, and this creates an awkward situation where we construct TransactionExecutionInputs from TransactionPreparationInputs and InputNotes but are forced to also provide a TransactionArgs, which is not usually available at that time (for instance in MockChain::get_transaction_execution_inputs).

This may be solvable in two ways:

• Either we keep three types in total: TransactionPreparationInputs, TransactionExecutionInputs and TransactionArgs separately,
• Or we try to "remove" TransactionArgs from the public API and instead only expose its functionality through TransactionExecutionInputs which would now allow users to do everything that tx args did previously: set tx scripts, auth args, note args, etc.

I would try the second approach, since removing the number of types here could be beneficial to make the API clearer, but if this turns out to be too awkward to use in practice, I'd revert to the first approach.

Depending on how we go forward with names, this needs an update in one or the other way.

Not for this PR, but we could also refactor TransactionArgs::new so that it takes AdviceInputs instead of AdviceMap. That would avoid the need for the extend call.

On second thoughts, extending the advice stack doesn't make sense from transaction args, but extending advice map or merkle store probably does make sense. I'm thinking of some custom use cases for merkle path verification, for instance. I wonder if we should refactor TransactionArgs a bit to address that. Not super pressing, though.

It's a bit awkward to carry the tx inputs and advice witness here separately and then combine them. But also, as @bobbinth mentioned, this will likely be refactored anyway, so no big deal.

I wonder if this should be just set_advice_inputs() which would reset the value of the new advice_inputs field I mentioned in one of the previous comments.

The idea here is that in the note checker's try_execute_notes(), we'd take kernel_inputs as a mutable parameter, and then replace them with the result we get back from FastProcessor::execute() (we get back advice provider there, but I believe we can convert it to advice inputs).

I removed extend_advice_inputs() from TransactionKernelInputs after adding init_advice_inputs to it. Not quite following how to alter try_execute_notes() w.r.t to this.

On second thoughts, extending the advice stack doesn't make sense from transaction args, but extending advice map or merkle store probably does make sense. I'm thinking of some custom use cases for merkle path verification, for instance. I wonder if we should refactor TransactionArgs a bit to address that. Not super pressing, though.