Remove asset_witnesses field from TransactionInputs

[bobbinth]

This is an alternative to #2274. It relies on the newly introduced TransactionInputs::read_vault_asset_witnesses(). This does not include any new tests (which should probably be added), but all existing tests seem to pass.

This approach could probably be used to remove most of the fields from TransactionInputs - merging everything into the advice_inputs field addressing the #1286.

The above would also remove the need for TransactionAdviceInputs - or more precisely, result in merging of TransactionAdviceInputs and TransactionInputs - which I think would be a good outcome.

[PhilippGackstatter]

Looks good to me!

I think we can return an error if the fee asset witness is not found (see comment).

Also, this does not add skipping the get_vault_asset_witnesses call if they are already present, but we can do this separately (maybe we should leave the issue open or create a new one?)

[PhilippGackstatter]

I think the approach from #2099 more or less strongly requires that we extend both self.advice_inputs and self.tx_args.advice_inputs. I'm actually not sure how necessary this is (I find this pretty hard to think about now), but it seems it would be more consistent. So I would add the function added in #2274 for this:

/// Extends the advice inputs with the provided inputs.
///
/// This extends both the internal advice inputs and the transaction arguments' advice inputs,
/// ensuring that `self.advice_inputs` is always a subset of `self.tx_args.advice_inputs()`.
(pub) fn extend_advice_inputs(&mut self, advice_inputs: AdviceInputs) {
    self.advice_inputs.extend(advice_inputs.clone());
    self.tx_args.extend_advice_inputs(advice_inputs);
}

And then here do:

let mut tx_adv = TransactionAdviceInputs::default();
witnesses.into_iter().for_each(|witness| tx_adv.add_asset_witness(witness));
self.extend_advice_inputs(tx_adv.into_advice_inputs());

This reuses the existing asset witness -> advice input conversion and we don't duplicate it.

[bobbinth]

I think the approach from #2099 more or less strongly requires that we extend both self.advice_inputs and self.tx_args.advice_inputs.

I'm not sure this is needed because TransactionInputs::set_advice_inputs() should already take care of this. But I agree the whole structure is confusing. I hope that with the structure I described in #1286 (comment), we'd be able to clarify the structure somewhat.

[PhilippGackstatter]

I'm not sure this is needed because TransactionInputs::set_advice_inputs() should already take care of this.

Technically yes, but it's not obvious that set_advice_inputs is called later (only after tx execution) and so this leaves this "invariant" temporarily broken. Not sure if this is a real problem, and I think it's fine to address this in #1286 or #2293.

Nit: I would prefer using TransactionAdviceInputs here so we use the lower-level leaf -> advice inputs conversion implementation (TransactionAdviceInputs::add_asset_witness) rather than move it outside that type.

[bobbinth]

Nit: I would prefer using TransactionAdviceInputs here so we use the lower-level leaf -> advice inputs conversion implementation (TransactionAdviceInputs::add_asset_witness) rather than move it outside that type.

I've actually removed TransactionAdviceInputs::add_asset_witness() in this PR since it was no longer needed. I think we can bring it back if needed depending on how we address #1286 and #2293.

[PhilippGackstatter]

The fee asset should always be in the advice inputs if things are going correctly: Even if we add logic to skip the pre-fetching of asset witnesses if they are already present, prepare_tx_inputs should basically guarantee that by the end of it, the fee asset witness is in tx_inputs.advice_inputs. So, I think we should return the error if the fee asset witness cannot be found:

// extract the fee asset amount from the transaction inputs
let fee_asset_witnesses = tx_inputs
    .read_vault_asset_witnesses(vault_root, [fee_asset_vault_key].into())
    .map_err(|source| TransactionExecutorError::FailedToReadFeeAssetWitness {
        native_asset_id,
        source,
    })?;
// This could be made nicer with a better TransactionInputs API as you mentioned in another comment.
let fee_asset_witness = fee_asset_witnesses
    .into_iter()
    .next()
    .expect("read_vault_asset_witnesses should have returned an error if the asset was not found");
match fee_asset_witness.find(fee_asset_vault_key) {
    Some(Asset::Fungible(fee_asset)) => fee_asset.amount(),
    Some(Asset::NonFungible(_)) => {
        return Err(TransactionExecutorError::FeeAssetMustBeFungible);
    },
    // If the witness does not contain the asset, its balance is zero.
    None => 0,
}

with a new error variant:

#[error(
    "failed to read fee asset witness with account ID {native_asset_id} from transaction inputs"
)]
FailedToReadFeeAssetWitness {
    native_asset_id: AccountId,
    source: TransactionInputsExtractionError,
},

[bobbinth]

I did something similar in 168f865 - but let me know if you'd prefer your approach.

[bobbinth]

Also, this does not add skipping the get_vault_asset_witnesses call if they are already present, but we can do this separately (maybe we should leave the issue open or create a new one?)

Ah - yes! I missed that. Now this implements this too.

[PhilippGackstatter]

Looks good to me!

[PhilippGackstatter]

I'm not sure this is needed because TransactionInputs::set_advice_inputs() should already take care of this.

Technically yes, but it's not obvious that set_advice_inputs is called later (only after tx execution) and so this leaves this "invariant" temporarily broken. Not sure if this is a real problem, and I think it's fine to address this in #1286 or #2293.

Nit: I would prefer using TransactionAdviceInputs here so we use the lower-level leaf -> advice inputs conversion implementation (TransactionAdviceInputs::add_asset_witness) rather than move it outside that type.