-
Notifications
You must be signed in to change notification settings - Fork 527
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
Check verification key hash in apply
#12498
Check verification key hash in apply
#12498
Conversation
!ci-build-me |
!ci-build-me |
!ci-build-me |
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 to me!
(mk_account_update_body | ||
(Proof (Zkapp_account.dummy_vk_hash ())) | ||
Call token_owner Token_id.default (3 * account_creation_fee) ) | ||
(mk_account_update_body Signature Call token_owner Token_id.default |
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.
can we use a test zkapp here instead of changing the authorization? or write another similar test with Proof auth?
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.
There's a bug in this test, if we use Proof
, because the corresponding ledger account has None
for the zkapp
field, hence no vk hash, so the new vk hash check fails.
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'm able to patch the ledger so the token funder has a zkapp with the dummy verification key, allowing Proof
for this sub-test. (It would require a bit more change to do the same for the token owner.)
(Account.verification_key_hash a) | ||
(Account_update.verification_key_hash account_update)) | ||
in | ||
assert_ ~pos:__POS__ matching_verification_key_hashes ; |
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.
Can you call Local_state.add_check
instead of raising. We also need a new failure to identify this error
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.
Done.
!ci-build-me |
!ci-build-me |
When calling
Zkapp_command_logic.apply
, for each account update, check its verification key hash against the hash in the account in the local state.For the
Zkapps_examples
tests, each account update is given a 0 verification key hash. We patch those account updates with the hash from the corresponding account in the test ledger, if that ledger is given, and with any hashes from keys that areSet
in preceding account updates.Closes #12392.