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

node-split poc: move IdentityStates to separate package, add metadata for identity states #6510

Open
wants to merge 4 commits into
base: node-split-poc
Choose a base branch
from

Conversation

kacpersaw
Copy link
Contributor

Description

Closes #6509

  • Moved IdentityStates to separate package called identity.
  • Added metadata for identity states.
  • Adjusted API to the new proto schema

Integration of identity package:

Refactoring identity state handling:

Updates to gRPC server:

…tity states. Adjust to latest api changes.
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 29.13043% with 163 lines in your changes missing coverage. Please review.

Project coverage is 77.2%. Comparing base (05ba7fb) to head (e6dea00).
Report is 1 commits behind head on node-split-poc.

Files with missing lines Patch % Lines
identity/state.go 0.0% 89 Missing ⚠️
identity/identity.go 26.3% 55 Missing and 1 partial ⚠️
api/grpcserver/v2alpha1/smeshing_identities.go 5.8% 16 Missing ⚠️
activation/activation.go 94.1% 1 Missing ⚠️
activation/nipost.go 96.5% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           node-split-poc   #6510     +/-   ##
================================================
- Coverage            77.3%   77.2%   -0.2%     
================================================
  Files                 341     342      +1     
  Lines               45737   45822     +85     
================================================
+ Hits                35378   35385      +7     
- Misses               8246    8324     +78     
  Partials             2113    2113             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

api/grpcserver/v2alpha1/smeshing_identities.go Outdated Show resolved Hide resolved
identity/metadata.go Outdated Show resolved Hide resolved
identity/state.go Outdated Show resolved Hide resolved
Copy link
Member

@fasmat fasmat left a comment

Choose a reason for hiding this comment

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

I find this approach quite error prone. Nothing guarantees that any State sets the corresponding metadata. Also metadata might be stored that isn't even applicable to the given state.

This could probably be improved by the use of generics, where State is a generic type and RetryingState, PoetRegisteredState etc. are the corresponding specific types.

Then the big conditional checks in the api package might also be avoidable.

@kacpersaw kacpersaw changed the title node-split: move IdentityStates to separate package, add metadata for identity states node-split poc: move IdentityStates to separate package, add metadata for identity states Dec 3, 2024
identity/state.go Outdated Show resolved Hide resolved
}

//nolint:lll
func (s *StateStorage) AllEligibilities() map[types.NodeID]map[types.EpochID]map[types.LayerID][]types.VotingEligibility {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: EpochID can be derived from LayerID so this could probably be simplified to

Suggested change
func (s *StateStorage) AllEligibilities() map[types.NodeID]map[types.EpochID]map[types.LayerID][]types.VotingEligibility {
func (s *StateStorage) AllEligibilities() map[types.NodeID]map[types.LayerID][]types.VotingEligibility {

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