-
Notifications
You must be signed in to change notification settings - Fork 180
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
[EFM] Added a service event to update EpochExtensionViewCount
#6272
[EFM] Added a service event to update EpochExtensionViewCount
#6272
Conversation
…tructure. Updated dependent structures. Updated godoc
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/efm-recovery #6272 +/- ##
========================================================
+ Coverage 41.48% 41.49% +0.01%
========================================================
Files 2000 2003 +3
Lines 142559 142613 +54
========================================================
+ Hits 59136 59173 +37
- Misses 77249 77263 +14
- Partials 6174 6177 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Looks good. Most comments are just fixing some copy-paste errors.
After implementing service event handling I suspect that we will get similar state machines for simple "set-value" style events. To avoid boilerplate we might introduce a solution based on generics or maybe consolidate all the logic in single state machine.
I agree, I think handling all these simple "set a single primitive parameter" cases in the SetValue
state machine is a good start.
state/protocol/protocol_state/kvstore/set_value_statemachine.go
Outdated
Show resolved
Hide resolved
state/protocol/protocol_state/kvstore/set_value_statemachine_test.go
Outdated
Show resolved
Hide resolved
…est.go Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
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.
solid and very clean code 🎸 with great test coverage 🎶
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.
random side comment:
- wondering if we should change the type of
evolvingState
in line 312 toprotocol.KVStoreReader
flow-go/state/protocol/protocol_state/state/protocol_state.go
Lines 308 to 312 in 91554ed
func (s *MutableProtocolState) build( parentStateID flow.Identifier, stateMachines []protocol_state.KeyValueStoreStateMachine, serviceEvents []flow.ServiceEvent, evolvingState protocol_state.KVStoreMutator,
this would reflect that the build method does not change theevolvingState
anymore
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.
Makes sense, updated.
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.
thanks for the consolidation. very nice and concise tests now. 👏
state/protocol/protocol_state/kvstore/set_value_statemachine.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org>
#5725
Context
This PR is the last one to implement in order to close linked issue. It introduces a new service event which will be used to update the
EpochExtensionViewCount
in the KV store. Additionally, I have added a dedicated state machine which parses the service event and applies it to the KV store.After implementing service event handling I suspect that we will get similar state machines for simple "set-value" style events. To avoid boilerplate we might introduce a solution based on generics or maybe consolidate all the logic in single state machine.
Additionally refactored unit tests using generics to make future extensions to the list of the service events easier.