-
Notifications
You must be signed in to change notification settings - Fork 89
feat(validator): FPI support in data store #1493
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
base: next
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized recently that we should probably be using std::futures::ready instead of async move to create futures for sync code. Though the difference is really just when the bulk of the work gets done.
// This
return std::futures::ready(x);
// Instead of this
async move {
// Only sync code
return x;
}
bobbinth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left just one small nit inline.
Also, would be good to get a review from @igamigo. Maybe we can run client integration tests against this branch to confirm that transactions that do FPI calls can indeed be accepted by the node.
I tried running the tests from miden-client |
| if self.tx_inputs.account().id() == account_id { | ||
| // Get asset witnessess from native account. | ||
| Result::<Vec<_>, _>::from_iter(vault_keys.into_iter().map(|vault_key| { | ||
| match self.tx_inputs.account().vault().open(vault_key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The native account's witnesses are handled just like the ones for foreign accounts, i.e. they are added to the advice inputs on lazy loading, but not to the PartialVault. Any asset witnesses that are present in the partial vault at the beginning are also added to the advice inputs.
So, the advice inputs contain both the pre-provided asset witnesses and those fetched during lazy loading, and so we should read from the advice inputs and not from the partial vault.
The need for this should go away with 0xMiden/miden-base#2298 completely, at which point this could be replaced with unimplemented!.
Because of these subtle differences, I still think it would be good to add a test for this, e.g. one that has an FPI call, reads a storage map item lazily and has input notes with some assets, so the pre-fetch is executed.
And then all of these annoying details should be abstracted away once we have a re-execution API exposed in base directly: 0xMiden/miden-base#2293.
Context
The Validator's
TransactionInputsDataStoredoes not handle foreign accounts for the purposes of the following functions ofDataStoretrait:get_foreign_account_inputs()get_foreign_account_inputs()get_vault_asset_witnesses()NOTE: This will remain in draft until miden-base has the
TransactionIniputs::read_foreign_account_inputs()functionality merged into next.A followup PR will be made to add similar functionality to the
NtxDataStore.Relates to #1387 and #1305.
Changes
TransactionInputsDataStoreto handle foreign accounts throughself.tx_inputs.