Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement account code loading into transaction executor #936

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

bobbinth
Copy link
Contributor

This PR adds TransactionExecutor::load_account_code() method. It is meant to address some issues brought up in #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 #934 (comment).

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

Copy link
Contributor

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you!

@igamigo
Copy link
Collaborator

igamigo commented Oct 25, 2024

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
Copy link
Contributor Author

While I think this PR (and PRs that will build on it), will address the immediate needs of #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:

  • Code of foreign accounts is loaded via TransactionExecutor::load_account_code().
  • Code of native account, notes, and transaction script are loaded via the DataStore interface.

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) .

@bobbinth bobbinth merged commit dd01de8 into next Oct 25, 2024
8 checks passed
@bobbinth bobbinth deleted the bobbin-load-account-code branch October 25, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants