Skip to content

Improve zkapp transaction regeneration #17484

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2025
Merged

Improve zkapp transaction regeneration #17484

merged 1 commit into from
Jul 11, 2025

Conversation

Geometer1729
Copy link
Member

@Geometer1729 Geometer1729 commented Jul 7, 2025 •
edited
Loading

Explain your changes:

Currently to regenerate the data for the test in src/lib/transaction/transaction_hash.ml you paste comments into utop. Unfortunately when these comments stop compiling this doesn't raise any errors in CI.

This pr moves the code to regenerate it out of the comment and into the error case, providing the regenerated value whenever the hash fails.

Explain how you tested your changes:

  • Added a test that the new value can be hashed
  • Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them
  • Closes #0000

@Geometer1729
Copy link
Member Author

!ci-build-me

3 similar comments
@Geometer1729
Copy link
Member Author

!ci-build-me

@Geometer1729
Copy link
Member Author

!ci-build-me

@Geometer1729
Copy link
Member Author

!ci-build-me

@Geometer1729 Geometer1729 marked this pull request as ready for review July 9, 2025 15:09
@Geometer1729 Geometer1729 requested a review from a team as a code owner July 9, 2025 15:09
@Geometer1729 Geometer1729 requested a review from glyh July 9, 2025 15:25
@Geometer1729
Copy link
Member Author

!ci-build-me

glyh
glyh reviewed Jul 10, 2025
View reviewed changes
src/lib/transaction/transaction_hash.ml
@@ -302,14 +302,9 @@ let%test_module "Transaction hashes" =
run_test ~transaction_id ~expected_hash

(* To regenerate:
* Run dune in this library's directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's funny we have code in comments

glyh
glyh requested changes Jul 10, 2025
View reviewed changes
Copy link
Member

@glyh glyh left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test coverage for this new app. I'm wondering why can't we just put those code in the place of the original testcase, instead of creating a new app?

src/app/generate_zkapp_txn/generate_zkapp_txn.ml Outdated
(module Mina_base.User_command.Stable.Latest)
(Zkapp_command txn)
|> Base64.encode
|> function Ok x -> x | Error _ -> ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd wrap this whole thing in a transaction_id_of_zkapp_command function returning an option.
And here I'd just unwrap the result and let it fail if it's Error _.

@glyh glyh added the enhancement Not big enough to be a feature, but is a smaller improvement label Jul 10, 2025
@Geometer1729 Geometer1729 mentioned this pull request Jul 10, 2025
Open
@Geometer1729 Geometer1729 force-pushed the brian/regenerate-transaction-hash branch from c7c2924 to 21eb945 Compare July 10, 2025 18:30
@Geometer1729
Copy link
Member Author

Please add test coverage for this new app. I'm wondering why can't we just put those code in the place of the original testcase, instead of creating a new app?

That's a much better idea, thanks

@Geometer1729
Copy link
Member Author

!ci-build-me

@Geometer1729 Geometer1729 force-pushed the brian/regenerate-transaction-hash branch from 8314fee to ea28cc3 Compare July 10, 2025 19:56
@Geometer1729
Copy link
Member Author

!ci-build-me

@Geometer1729 Geometer1729 requested a review from a team as a code owner July 10, 2025 20:18
@Geometer1729
Copy link
Member Author

!ci-build-me

@Geometer1729 Geometer1729 changed the title Move transaction regeneration to an app and fix it Improve zkapp transaction regeneration Jul 10, 2025
@Geometer1729 Geometer1729 requested a review from glyh July 10, 2025 21:04
@Geometer1729
Copy link
Member Author

!ci-build-me

glyh
glyh approved these changes Jul 11, 2025
View reviewed changes
src/lib/transaction/transaction_hash.ml
let new_zkapp_txn_hash () =
hash_command (Zkapp_command new_zkapp_txn) |> to_base58_check

let run_test ?regenerate_zkapp ~transaction_id ~expected_hash () =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it seems this optional arg has type unit option, I think it'd be better to set it to be a bool option with None indicating false.

@Geometer1729
Copy link
Member Author

!ci-build-me

@dannywillems
Copy link
Member

!ci-bypass-changelog

@dannywillems
Copy link
Member

Can you clean the merge commits before merging in compatible, please?

@Geometer1729 Geometer1729 force-pushed the brian/regenerate-transaction-hash branch from 492c2cc to e133fb4 Compare July 11, 2025 14:22
@Geometer1729
Copy link
Member Author

!ci-build-me

@Geometer1729 Geometer1729 merged commit 610b742 into compatible Jul 11, 2025
50 checks passed
@Geometer1729 Geometer1729 deleted the brian/regenerate-transaction-hash branch July 11, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glyh glyh glyh approved these changes

Assignees
No one assigned
Labels
enhancement Not big enough to be a feature, but is a smaller improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Geometer1729 @dannywillems @glyh