Feature/include network id for account update hash berk #13880
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…include-network-id-for-account-update-hash-berk
|
!ci-build-me |
added 2 commits
August 18, 2023 00:38
…include-network-id-for-account-update-hash-berk
|
!ci-build-me |
mrmr1993
reviewed
Aug 17, 2023
added 10 commits
August 18, 2023 03:51
…include-network-id-for-account-update-hash-berk
…include-network-id-for-account-update-hash-berk
|
!ci-build-me |
|
!ci-build-me |
mrmr1993
approved these changes
Aug 24, 2023
Trivo25
approved these changes
Aug 24, 2023
…include-network-id-for-account-update-hash-berk
…include-network-id-for-account-update-hash-berk
|
!ci-build-me |
|
!ci-build-me |
deepthiskumar
requested changes
Aug 25, 2023
src/app/test_executive/zkapps.ml
Outdated
| @@ -242,7 +243,8 @@ module Make (Inputs : Intf.Test.Inputs_intf) = struct | |||
| , zkapp_command_invalid_nonce | |||
| , zkapp_command_insufficient_funds | |||
| , zkapp_command_insufficient_replace_fee | |||
| , zkapp_command_insufficient_fee ) = | |||
| , zkapp_command_insufficient_fee | |||
| , zkapp_command_single_account_update ) = | |||
There was a problem hiding this comment.
nit: could you rename this to something more meaningful- zkapp_command_cross_network_replay or something?
|
|
||
| (* This test uses dummy verifiers, that's why it's not rejecting the problematic zkapp command | ||
| But this test is still useful, since it exercises the parts that doesn't requires a pickles instance | ||
| Combining together with the test that uses pickles directly, these 2 tests would make sure that |
There was a problem hiding this comment.
can you edit the comment to describe this test? What are the 2 tests here?
| @@ -4581,6 +4581,130 @@ module Make_str (A : Wire_types.Concrete) = struct | |||
| in | |||
| zkapp_command | |||
|
|
|||
| module Single_account_update_spec = struct | |||
There was a problem hiding this comment.
Can you add a comment to describe what this is for (as there are a few different specs) and when it can be used?
added 4 commits
August 25, 2023 22:40
zkapp_command_cross_network_replay
staged_ledger.ml
…include-network-id-for-account-update-hash-berk
…' of github.com:MinaProtocol/mina into feature/include-network-id-for-account-update-hash-berk
|
!ci-build-me |
|
!ci-build-me |
…include-network-id-for-account-update-hash-berk
|
!ci-build-me |
deepthiskumar
approved these changes
Aug 28, 2023
|
!approved-for-mainnet |
ghost-not-in-the-shell
deleted the
feature/include-network-id-for-account-update-hash-berk
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explain your changes:
This PR adds chain name to the account update hash in order to address the concern described here: #13759
Explain how you tested your changes:
Added a section for testing account update with different network ID in zkapps-integration-test;
also added a test in transaction_pool, but since we only have dummy verifier in unit-test, so it should pass the verification.
Checklist: