Fix generator for transaction_pool tests #16285
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
georgeee
added a commit
that referenced
this pull request
Oct 24, 2024
This a regression test for mk_zkapp_commands_single_block: before the fix (that will be delivered as part of PR #16285) arrived, it was failing, and is passing after the fix. Along with testing the generator, it also checks the invariant `Fn.compose of_wire to_wire = ident`.
9 tasks
dkijania
reviewed
Nov 13, 2024
| @@ -1127,7 +1127,7 @@ let gen_zkapp_command_from ?global_slot ?memo ?(no_account_precondition = false) | |||
| ?(no_token_accounts = false) ?(limited = false) | |||
| ?(generate_new_accounts = true) ?failure | |||
| ?(max_account_updates = max_account_updates) | |||
| ?(max_token_updates = max_token_updates) | |||
| ?(max_token_updates = max_token_updates) ?(map_account_update = ident) | |||
There was a problem hiding this comment.
I'm quite sure this is managed by the code formatter
dkijania
approved these changes
Nov 13, 2024
Problem: previous generator was modifying zkapp commands after computing call forest hashes, which essentially rendered transactions incorrect, though didn't fail the test immediately. Solution: rearrangem modifications so that call forest hashes are computed after modifications to account updates take place. While the issue didn't cause any test to fail, after some unrelated change some tests of transaction_pool started failing. Another branch will also contain a "regression" unit tests that fails before the fix and succeeds after. That regression tests will essentially recompute call forest hashes and compare them to original hashes.
georgeee
force-pushed
the
georgeee/fix-generator-in-transaction_pool-tests
branch
from
6ffd500 to
3095710
Compare
|
!ci-build-me |
georgeee
deleted the
georgeee/fix-generator-in-transaction_pool-tests
branch
georgeee
added a commit
that referenced
this pull request
Jan 25, 2025
This a regression test for mk_zkapp_commands_single_block: before the fix (that will be delivered as part of PR #16285) arrived, it was failing, and is passing after the fix. Along with testing the generator, it also checks the invariant `Fn.compose of_wire to_wire = ident`.
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.
Problem: previous generator was modifying zkapp commands after computing call forest hashes, which essentially rendered transactions incorrect, though didn't fail the test immediately.
Solution: rearrange modifications so that call forest hashes are computed after modifications to account updates take place.
While the issue didn't cause any test to fail, after some unrelated change some tests of transaction_pool started failing. Another branch will also contain a "regression" unit tests that fails before the fix and succeeds after. That regression tests will essentially recompute call forest hashes and compare them to original hashes.
Explain how you tested your changes:
Checklist: