Remove constructors for account precondition #14117
Conversation
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
| let nonce : _ Numeric.t = Check { lower = n; upper = n } in | ||
| { accept with nonce } | ||
|
|
||
| let is_nonce (t : t) = |
There was a problem hiding this comment.
This helper function seems misleading, and in the context where it is used (account_update.ml:1449), the logic is no longer the same. In order to check if a precondition is equivalent to the old Nonce precondition helper, you need to check that all other preconditions are set to Ignore.
There was a problem hiding this comment.
I think that's what is_accept { t with nonce = Ignore } is doing, yes?
There was a problem hiding this comment.
Yes, you are correct; that wasn't obvious to me when I read the code. Maybe this deserves a very brief comment?
| Mina_numbers.Account_nonce.of_uint32 nonce | ||
| | Full _ | Accept -> | ||
| failwith "Expected a nonce for fee payer account precondition" | ||
| if Zkapp_precondition.Account.is_nonce preconditions.account then |
There was a problem hiding this comment.
This is logic is not equivalent to what it was before (see other comment).
There was a problem hiding this comment.
It is equivalent, as far as I see.
There was a problem hiding this comment.
I think the difference is that, previously Nonce constructor made sure other fields were Ignore but now, the precondition could have Check for other fields and Ignore for the nonce field in which case it is not equivalent. The function should behave more like, is_nonce_only (that was implicit before)
There was a problem hiding this comment.
My bad, as I look at the way is_nonce function is implemented, the logic is equivalent. @nholland94 could elaborate on what your expectation is here?
| } | ||
| | Accept -> | ||
| Zkapp_precondition.Account.accept | ||
| let (_ : |
There was a problem hiding this comment.
Rather than having this explicit type equality check here, why not just specify the above type to just be Mina_wire_types.Mina_base.Account_update.Account_precondition.V1.t? The type checker will ensure that this type unifies with Zkapp_precondition.Account.Stable.V2.t if needed.
There was a problem hiding this comment.
It's useful know that this type is the same as the other two types, not for the type checker, but for humans.
It might make sense to swap them between the type definition and the equality assertion.
| (* we used to have 3 constructors, Full, Nonce, and Accept for the type t | ||
| nowadays, the generator creates these 3 different kinds of values, but all mapped to t | ||
| *) | ||
| let open Zkapp_basic in | ||
| Quickcheck.Generator.variant3 Zkapp_precondition.Account.gen | ||
| Account.Nonce.gen Unit.quickcheck_generator | ||
| |> Quickcheck.Generator.map ~f:(function |
There was a problem hiding this comment.
Why does this quickcheck generator still need to explicitly generate one of these 3 variants? It seems to make more sense to just quickcheck generate each precondition field separately. The set of generated values will still contain the nonce and accept cases.
There was a problem hiding this comment.
It doesn't need to, the goal was to generate the nonce and accept cases with the same frequency as before. If we generate the components independently, those cases will be much rarer.
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
|
!approved-for-mainnet |
|
!ci-nightly-me |
|
Integration tests https://buildkite.com/o-1-labs-2/mina-end-to-end-nightlies/builds/509 |
|
All the zkapp integration tests have passed |
Remove the
Full,Nonce, andAcceptconstructors for account preconditions. The typeAccount_update.Account_precondition.tis now the same asZkapp_precondition.Account.t..Add
Zkapp_precondition.Account.noncefunction, which takes a nonce, and creates a precondition that's the same as theacceptprecondition, with the nonce field set toCheckwith the same lower and upper bound. Theis_noncepredicate returns true if the precondition has that form.In the archive db, remove the
zkapp_precondition_typeandzkapp_account_precondition_valuestables. The precondition is now saved directly to thezkapp_account_preconditiontable, eliminating a level of indirection.The change at ~2024 in
Transaction_poolis a bit subtle. The reviewer may wish to scrutinize that change closely (I've convinced myself of its correctness, for whatever that's worth).Of note: the troublesome verification key integration test passes.
The version linter failure is expected and benign.
Closes #14114