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
Versioning Nat in Lite Chain and some dependencies #2213
Conversation
module V1 = struct | ||
module T = struct | ||
type t = | ||
{ staged_ledger_hash: Staged_ledger_hash.t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have stable versions of Staged_ledger_hash
and Ledger_hash
; do we really need the asserted
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because Staged_ledger_hash.t and Ledger_hash.t are derived from Pedersen_lib.Pedersen, which is not versioned as of right now.
module Stable = struct | ||
module V1 = struct | ||
module T = struct | ||
type t = {previous_state_hash: Pedersen.Digest.t; body: Body.Stable.V1.t} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use Pedersen.Digest.Stable.V1.t
here, and remove the asserted
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pedersen.Digest is not stable yet.
end | ||
|
||
type t = Stable.Latest.t = | ||
{previous_state_hash: Pedersen.Digest.t; body: Body.Stable.V1.t} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and then also use Pedersen.Digest.Stable.V1.t
here
module V1_make = Nat.V1_32_make () | ||
|
||
module Stable = struct | ||
module V1 = struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you write just module V1 = V1_make.Stable.V1
?
src/lib/lite_base/nat.ml
Outdated
@@ -15,7 +15,19 @@ module type Input_intf = sig | |||
end | |||
|
|||
module Make (M : Input_intf) = struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make
is asking for a bin_io
module, and the choice of that input gives new a particular version.
So I would write something like:
module Input_32 = struct
module Stable = struct
module V1 = struct
module T = struct
type t = ...Int32.t bin_io, version {asserted}
end
include T
end
end
where each version is a choice of the input to Make
. And then something like:
module Nat32_make = struct
module Stable = struct
module V1 = Make (Input_32.Stable.V1)
end
end
where that choice is reflected as a version in Nat32_make
. So a client of Nat32_make
that wants a versioned type would write something like Nat32_make.Stable.V1.Stable.V1.t
.
@@ -15,7 +15,19 @@ module type Input_intf = sig | |||
end | |||
|
|||
module Make (M : Input_intf) = struct | |||
include M | |||
module Stable = struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part looks good
src/lib/lite_base/nat.ml
Outdated
end | ||
|
||
module Make32 () = Make (struct | ||
type t = Core_kernel.Int32.t [@@deriving bin_io, eq, sexp, compare] | ||
module Input_32_V1 = struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above .... I think this should just be Input_32
, which can contain all possible versions.
src/lib/lite_base/nat.ml
Outdated
end | ||
end | ||
|
||
include Stable.V1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be omitted, since you'll always pass a particular version to Make
, rather than the whole module.
…oda into versioning/nat_lite_chain
Thank you for contributing to Coda! Please see
CONTRIBUTING.md
if you haven'tyet.
Explain your changes here.
Explain how you tested your changes here.
Checklist: