0xPolygonMiden / miden-base

Core components of the Polygon Miden rollup
MIT License
73 stars 45 forks source link

Implement account code loading into transaction executor #936

Closed bobbinth closed 1 month ago

bobbinth commented 1 month ago

This PR adds TransactionExecutor::load_account_code() method. It is meant to address some issues brought up in https://github.com/0xPolygonMiden/miden-base/issues/934. Specifically, it'll let the users of the executor to load foreign account code into the executor. It'll also enable associating a given set of procedures with a given account ID - and this will allow us to avoid modifying TransactionArgs as described in https://github.com/0xPolygonMiden/miden-base/issues/934#issuecomment-2438231043.

I made TransactionExecutor::load_account_code() take a mutable reference to self - @igamigo, will that be an issue for the client?

igamigo commented 1 month ago

I made TransactionExecutor::load_account_code() take a mutable reference to self - @igamigo, will that be an issue for the client?

No, I don't think it will be a problem

bobbinth commented 1 month ago

While I think this PR (and PRs that will build on it), will address the immediate needs of https://github.com/0xPolygonMiden/miden-base/issues/934, it is worth taking a deeper look at how we load account code and other data into the executor as it has become somewhat inconsistent.

The current model (after this PR) is:

There are a couple of alternatives to this:

Alternative 1

Load everything through the DataStore interface. This would mean that we'd need to add a list of account IDs to TransactionArgs and the DataStore interface` will look roughly like this:

pub trait DataStore {
    fn get_transaction_inputs(
        &self,
        account_id: AccountId,
        foreign_account_ids: &[AccountId],
        block_ref: u32,
        notes: &[NoteId],
    ) -> Result<TransactionInputs, DataStoreError>;
}

Alternative 2

Load everything via TransactionArgs. This would basically get rid of the DataStore interface for now as TransactionArgs would basically become a superset of TransactionInputs returned from the DataStore. In this scenario, the client would first make the call to the store to build TransactionInputs manually and then pass them to the executor.

The reason I say "for now" is because eventually we'll need to implement lazy loading of account storage/vault and we'll need to have something like the DataStore interface.

Alternative 3

We merge DataStore and TransactionMastStore into a single interface. Basically, it would mean that the interface would look something like this:

pub trait DataStore:  MastForestStore {
    fn get_transaction_inputs(
        &self,
        account_id: AccountId,
        foreign_account_ids: &[AccountId],
        block_ref: u32,
        notes: &[NoteId],
    ) -> Result<TransactionInputs, DataStoreError>;
}

That is, anything that implements DataStore would also need to implement MastForestStore interface (with its single get(&self, procedure_hash: &Digest) -> Option<Arc<MastForest>> method) .