Navigation Menu

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

Versioning Nat in Lite Chain and some dependencies #2213

Merged
merged 7 commits into from Apr 15, 2019

Conversation

wu-s-john
Copy link
Contributor

Thank you for contributing to Coda! Please see CONTRIBUTING.md if you haven't
yet.

Explain your changes here.

Explain how you tested your changes here.

Checklist:

  • Tests were added for the new behavior
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them:

module V1 = struct
module T = struct
type t =
{ staged_ledger_hash: Staged_ledger_hash.t
Copy link
Member

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?

Copy link
Contributor Author

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}
Copy link
Member

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.

Copy link
Contributor Author

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}
Copy link
Member

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
Copy link
Member

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 ?

@@ -15,7 +15,19 @@ module type Input_intf = sig
end

module Make (M : Input_intf) = struct
Copy link
Member

@psteckler psteckler Apr 11, 2019

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
Copy link
Member

Choose a reason for hiding this comment

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

this part looks good

end

module Make32 () = Make (struct
type t = Core_kernel.Int32.t [@@deriving bin_io, eq, sexp, compare]
module Input_32_V1 = struct
Copy link
Member

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.

end
end

include Stable.V1
Copy link
Member

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.

@wu-s-john wu-s-john added the ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR label Apr 15, 2019
@mergify mergify bot merged commit aa397d3 into master Apr 15, 2019
@mergify mergify bot deleted the versioning/nat_lite_chain branch April 15, 2019 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants