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

suggestions for PR #4834 #4854

Conversation

AlexHentschel
Copy link
Member

@AlexHentschel AlexHentschel commented Oct 20, 2023

Suggested revisions for PR #4834

Summary of central changes:

  • model/flow/protocol_state.go:

    • revised goDoc of protocol state implementation
    • changed ProtocolStateEntry.PreviousEpoch to pointer (consistent with ProtocolStateEntry.NextEpoch)
    • There should be no significant algorithmic changes. Though, I have switched the order of the if and else branches when processing an Epoch Setup event. For me, this is much more in line with the intuitive order of documentation.
  • state/protocol/protocol_state/updater.go:

    • notable updates and revisions of goDoc
    • tried to address concerns around inconsistent handling of invariances
    • updated code to work with ProtocolStateEntry.PreviousEpoch being potentially a nil pointer
  • storage/badger/protocol_state_test.go:

    • detailed goDoc revisions for test assertRichProtocolStateValidity

This PR is likely incomplete and might require tiding up some minor loose ends.

Alexander Hentschel added 2 commits October 20, 2023 01:15
• revised goDoc of protocol state implementation
• changed `ProtocolStateEntry.PreviousEpoch` to pointer (consistent with `ProtocolStateEntry.NextEpoch`)
• There should be no significant algorithmic changes. Though, I have switched the order of the if and else branches when processing an Epoch Setup event. For me, this is much more in line with the intuitive order of documentation.

state/protocol/protocol_state/updater.go:
• notable updates and revisions of goDoc
• tried to address concerns around inconsistent handling of invariances
• updated code to work with `ProtocolStateEntry.PreviousEpoch` being potentially a nil pointer

storage/badger/protocol_state_test.go:
• detailed goDoc revisions for test `assertRichProtocolStateValidity`
@durkmurder
Copy link
Member

Excellent stuff. I have made some adjustments to fix tests around root snapshots.

@durkmurder durkmurder merged commit 0dd823a into yurii/4649-prev-epoch-refactoring-attempt Oct 20, 2023
@durkmurder durkmurder deleted the alex/4649-prev-epoch-refactoring-attempt_-_suggestions branch October 20, 2023 13:31
This pull request was closed.
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.

2 participants