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 #1637

Merged
merged 52 commits into from
Jul 10, 2024
Merged

versioning #1637

merged 52 commits into from
Jul 10, 2024

Conversation

imabdulbasit
Copy link
Contributor

@imabdulbasit imabdulbasit commented Jun 24, 2024

This PR:

  • moves types from the sequencer crate
  • Moved SeqTypes to the types crate.
  • implements all the hotshot traits for the SeqTypes
  • moves all the helper functions, types and some traits needed for hotshot trait implementations or types
  • creates a versioned Header type in v0/header.rs and the serialization/deserialization

things to review:

  • v0/header.rs
  • Serialization and test_versioned_header_serialization() in v0/impls/header.rs
  • check that all types and implementations are organized clearly within their respective modules.

jbearer and others added 9 commits May 23, 2024 16:24
This will make it easier to keep a somewhat consistent Rust API
even as we add or change various fields in the underlying
representation of the header.
@imabdulbasit imabdulbasit changed the title Ab/ver versioning Jun 25, 2024
types/src/v0/header.rs Outdated Show resolved Hide resolved
types/src/v0/header.rs Outdated Show resolved Hide resolved
types/src/v0/header.rs Outdated Show resolved Hide resolved
types/src/v0/header.rs Outdated Show resolved Hide resolved
types/src/v0/header.rs Outdated Show resolved Hide resolved
Copy link
Member

@jbearer jbearer left a comment

Choose a reason for hiding this comment

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

Looks good, minus a few comments. I think @ggutoski should review types/src/v0/impls/block/ and types/src/v0/v0_1/block.rs, mainly just to sign off on the organization of things; I don't think any logic has changed.

Also, I think we need something like reference_tests and message_compat_tests for the new versions, so that if we make any breaking changes to any existing supported version, a test will fail

types/src/v0/header.rs Outdated Show resolved Hide resolved
types/src/v0/impls/header.rs Outdated Show resolved Hide resolved
types/src/v0/header.rs Outdated Show resolved Hide resolved
types/src/v0/impls/header.rs Outdated Show resolved Hide resolved
types/src/v0/impls/header.rs Outdated Show resolved Hide resolved
types/src/v0/impls/header.rs Outdated Show resolved Hide resolved
types/src/v0/impls/header.rs Outdated Show resolved Hide resolved
types/src/v0/impls/header.rs Outdated Show resolved Hide resolved
types/src/v0/impls/header.rs Outdated Show resolved Hide resolved
@ggutoski
Copy link
Contributor

ggutoski commented Jul 9, 2024

Looks good, minus a few comments. I think @ggutoski should review types/src/v0/impls/block/ and types/src/v0/v0_1/block.rs, mainly just to sign off on the organization of things; I don't think any logic has changed.

Every field in every struct is now pub(crate). This is a significant break of abstraction. I do not yet have a helpful suggestion on how to recover abstraction. Perhaps this break is a price worth paying. Happy to discuss further.

@jbearer
Copy link
Member

jbearer commented Jul 9, 2024

Looks good, minus a few comments. I think @ggutoski should review types/src/v0/impls/block/ and types/src/v0/v0_1/block.rs, mainly just to sign off on the organization of things; I don't think any logic has changed.

Every field in every struct is now pub(crate). This is a significant break of abstraction. I do not yet have a helpful suggestion on how to recover abstraction. Perhaps this break is a price worth paying. Happy to discuss further.

Can we not just duplicate the old directory structure under types/src/v0/v0_1, bring back the pub(in ...), and use public methods to access these types for the trait impls?

types/src/v0/v0_1/header.rs Outdated Show resolved Hide resolved
@imabdulbasit imabdulbasit linked an issue Jul 10, 2024 that may be closed by this pull request
data/header_v2.json Outdated Show resolved Hide resolved
types/src/v0/impls/header.rs Outdated Show resolved Hide resolved
Copy link
Member

@jbearer jbearer left a comment

Choose a reason for hiding this comment

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

😮‍💨

@imabdulbasit imabdulbasit merged commit 3d41c0e into main Jul 10, 2024
15 checks passed
@imabdulbasit imabdulbasit deleted the ab/ver branch July 10, 2024 17:43
@tbro tbro mentioned this pull request Jul 24, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants