Skip to content
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.

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

Split Global_slot.t into since-genesis and since-hard-fork types #13143

Merged
merged 51 commits into from
Jun 1, 2023

Conversation

psteckler
Copy link
Member

@psteckler psteckler commented May 2, 2023

Split Mina_numbers.Global_slot.t into Global_slot_since_genesis.t and Global_slot_since_hard_fork.t. Add new type Global_slot_span.t for vesting periods, also used as for an argument to add and sub. Although all these types as an unsigned 32-bit integer, none is compatible with the others, so they can't be confused in the code.

The existing uses of Global_slot.t have been changed to the ...since_genesis.t type. Some of them very likely need to become uses of ...since_hard_fork.t.

For V1 Signed_commands, introduce Global_slot_legacy, which is identical to the old Global_slot.

There were places in the code where slots were being used both as slots, as such, and as spans. In this PR, that's handled by converting the values via uint32. Probably that code should be cleaned up to make those roles distinct.

Closes #13058.

@psteckler psteckler requested review from a team as code owners May 2, 2023 22:45
@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

Copy link

@jdsteinhauser jdsteinhauser left a comment

Choose a reason for hiding this comment

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

That was a huge PR! Looks good, :shipit:

@rbonichon
Copy link
Contributor

rbonichon commented May 3, 2023

@psteckler Would you be able to open issues wrt to spans/slots uses and wrt to this remark

Some of them very likely need to become uses of ...since_hard_fork.t.

@psteckler
Copy link
Member Author

psteckler commented May 3, 2023

@psteckler Would you be able to open issues

#13154 #13155

@psteckler
Copy link
Member Author

!ci-build-me

Copy link
Contributor

@rbonichon rbonichon left a comment

Choose a reason for hiding this comment

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

Few minor suggestions. LGTM

Comment on lines 95 to 96
T.sub t.slot_number
(Mina_numbers.Global_slot_span.of_uint32 @@ T.to_uint32 t'.slot_number)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I get why we need the conversion for t' and not for t as they have the same type so I was expecting this treatment to be symmetric (old code had no conversion whatsoever)

Copy link
Member Author

@psteckler psteckler May 3, 2023

Choose a reason for hiding this comment

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

T.sub takes a slot and a span, so t'.slot_number needed to be converted to a span.

T.sub used to take a slot and another slot, but I think it makes more sense to subtract a span, in the same way Time does.

But ( - ) isn't used anywhere, so I'm going to remove it. Its uses were replaced by diff_slots, which takes the slots from two ts, and returns a span.

Comment on lines 899 to 902
let min_vesting_end =
Global_slot_span.to_uint32 vesting_period
|> Unsigned.UInt32.succ |> Global_slot_since_genesis.of_uint32
in
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how many times this pattern occur but is it worth having succ in Global_slot_span too?

Copy link
Member Author

@psteckler psteckler May 3, 2023

Choose a reason for hiding this comment

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

It's there already. But this is an instance where a span is converted to a slot via uint32. The succ could be done in the span, the uint32, or the slot, and I chose the uint32. I'll change it to do it in the span.

@psteckler
Copy link
Member Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor

!ci-build-me

@mitschabaude
Copy link
Contributor

!ci-build-me

@mitschabaude
Copy link
Contributor

!ci-build-me

@mitschabaude
Copy link
Contributor

!ci-build-me

@deepthiskumar
Copy link
Member

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@deepthiskumar
Copy link
Member

!approved-for-mainnet

@deepthiskumar
Copy link
Member

The following version change from mainnet type is actually to retain the old global slot type. Rest of the version changes are for V2 types or V1 types introduced for zkApps


In file: src/lib/mina_base/signed_command_payload.ml
--
  | Versioned types changed at path: Common.Stable.V1.t
  | Was: type t = (Currency.Fee.Stable.V1.t, Public_key.Compressed.Stable.V1.t, Token_id.Stable.V1.t, Account_nonce.Stable.V1.t, Global_slot.Stable.V1.t, Memo.Stable.V1.t) Poly.Stable.V1.t[@@deriving bin_io]
  | Now: type t = (Currency.Fee.Stable.V1.t, Public_key.Compressed.Stable.V1.t, Token_id.Stable.V1.t, Account_nonce.Stable.V1.t, Global_slot_legacy.Stable.V1.t, Memo.Stable.V1.t) Poly.Stable.V1.t [@@deriving bin_io]

@deepthiskumar deepthiskumar merged commit a6fee43 into berkeley Jun 1, 2023
@deepthiskumar deepthiskumar deleted the fix/ctors-for-global-slot-types branch June 1, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants