Skip to content

Add type annotations for top level functions in Mina_base.Zkapp_call_… #17087

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
Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add type annotations for top level functions in Mina_base.Zkapp_call_… #17087

wants to merge 1 commit into from

Conversation

GarmashAlex
Copy link

@GarmashAlex GarmashAlex commented Apr 27, 2025 •
edited
Loading

…forest_base

Explain your changes:
*

  • Added type annotations to all top-level functions in Mina_base.Zkapp_call_forest_base to improve code readability and help OCaml better infer types.
  • Replaced generic wildcards (_) with specific type variables throughout the codebase.
  • Restructured function definitions to consistently place type annotations before the implementation.
  • Improved the cons_aux function to use Option.value for the default parameter.
  • Added proper type constraints for functions using existential types.

Explain how you tested your changes:
*

  • The changes are purely type annotations that don't modify functionality.
  • The existing code structure remains unchanged.
  • The OCaml compiler checks the type annotations at compile time, ensuring they are correct and compatible with the existing code.

Checklist:

  • 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

@GarmashAlex GarmashAlex requested a review from a team as a code owner April 27, 2025 15:55
glyh
glyh requested changes Apr 28, 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.

A couple of problems:

  • Our convention is to avoid in-place type annotation when possible, and I suggest you do annotations in a separate interface file
  • It seems your 1 commit does a couple of stuffs, could you separate them so it's more reviewable?
  • Last but not least, we don't accept PR against develop, do you mind rebase your work on compatible?

Thank you :)

glyh
glyh reviewed Apr 28, 2025
View reviewed changes
src/lib/mina_base/zkapp_call_forest_base.ml
@@ -350,7 +351,7 @@ module Shape = struct
type t = Node of (I.t * t) list [@@deriving quickcheck]
end

let rec shape (t : _ t) : Shape.t =
let rec shape (t : ('a, 'b, 'c) t) : Shape.t =
Copy link
Member

@glyh glyh Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mind add some meaning type param names here(and other places) instead of ('a, 'b, 'c)? Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glyh glyh glyh requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add type annotations for top level functions in Mina_base.Zkapp_call_forest_base
2 participants
@GarmashAlex @glyh