-
Notifications
You must be signed in to change notification settings - Fork 47
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
Refactor tx kernel events errors #819
base: next
Are you sure you want to change the base?
Conversation
b77fad9
to
a200964
Compare
a200964
to
b9937fe
Compare
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 a few comments line. I think the biggest issue is that moving storage events into api.masm
would break how faucets work. I don't have a great solution to this yet - but also, we should probably modify a test or two to make sure these errors get caught (or confirm that there are no errors with the current changes).
#exec.memory::get_input_vault_root_ptr | ||
#push.{ACCOUNT_ID_FUNGIBLE_FAUCET_ON_CHAIN} | ||
#exec.asset_vault::get_balance | ||
#push.{FUNGIBLE_ASSET_AMOUNT} assert_eq | ||
|
||
# assert the faucet storage has been updated | ||
push.{FAUCET_STORAGE_DATA_SLOT} | ||
exec.account::get_item | ||
push.{expected_final_storage_amount} | ||
assert_eq | ||
#push.{FAUCET_STORAGE_DATA_SLOT} | ||
#exec.account::get_item | ||
#push.{expected_final_storage_amount} | ||
#assert_eq |
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.
This should probably be un-commented?
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.
Indeed, I forgot to work on this. The test also passes when un-commented.
# note: storage slot is known here, so set_item_raw can be used | ||
add exec.account::get_faucet_storage_data_slot exec.account::set_item_raw dropw |
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 am not sure this is going to work. If I'm understanding the flow correctly:
account::set_item_raw
will not fireSTORAGE_ON_AFTER_SET_ITEM
event.- This means that
Host::on_account_storage_after_set_item()
won't get executed and theAccountDeltaTracker
won't be updated. - This means that changes to the account storage for the faucet reserved slot won't be included in account delta.
We should probably modify one of the tests to test for this (i.e., when we mint a new asset, account storage delta should include the update incrementing the total number of assets minted).
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.
One potential way to handle this is to introduce new events which get fired in mint_asset
and burn_asset
procedures in api.masm
. Then handlers of these events would modify the account delta tracker accordingly.
Though, this may be introducing too much complexity.
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 was thinking about the same. If this change ("firing all events from api.masm
") is purely aesthetics, we should not introduce this extra complexity. If the change adds readability to the transaction kernel, we should make the change.
One problem is that in api.masm
, we don't have the information that we want to update yet. To update the account_storage_delta
in the correct way, we need the TOTAL_ISSUANCE
, which we compute at a later stage. So, to fire all events in api.masm
, we need to rewrite the way how the faucet works.
# note: storage slot is known here, so set_item_raw can be used | ||
sub exec.account::get_faucet_storage_data_slot exec.account::set_item_raw dropw |
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.
Similar comment to the one above and this is also relevant to the other changes in this file.
# emit event to signal that an account storage item is being updated | ||
padw loc_loadw.1 loc_load.0 emit.ACCOUNT_STORAGE_AFTER_SET_ITEM_EVENT drop dropw | ||
# => [OLD_VALUE] |
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.
nit: I would probably move drop dropw
to line 203 below and update the comments accordingly.
# => [index, KEY, NEW_VALUE, OLD_MAP_ROOT, OLD_VALUE, ...] | ||
|
||
# emit event to signal that an account storage map item is being updated | ||
emit.ACCOUNT_STORAGE_AFTER_SET_MAP_ITEM_EVENT drop dropw dropw |
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.
Similar nit as above.
Closes #768
This PR moves (almost) all events into
api.masm
. All slot-check are now in the subsequent files fromapi.masm
Two open questions about events that are not in
api.masm
yet:The event
ACCOUNT_PUSH_PROCEDURE_INDEX_EVENT
seems to be needed inaccount.masm
or we need to rewrite this testmiden-base/miden-tx/src/tests/kernel_tests/test_account.rs
Lines 506 to 510 in 13724e9
Some events are still emitted in
tx.masm
regarding note creation. I can move those toapi.masm
, too.Let me know what you think @bobbinth, then I can finalize the PR and we can merge this week.