-
Notifications
You must be signed in to change notification settings - Fork 67
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
fix(validator set): use comet compatible API for validators set wrt hash #1159
base: main
Are you sure you want to change the base?
Conversation
func (s Sequencer) TMValidator() *types.Validator { | ||
return &s.val | ||
} | ||
|
||
func (s Sequencer) TMValidators() []*types.Validator { | ||
return []*types.Validator{s.TMValidator()} | ||
} | ||
|
||
func (s Sequencer) TMValset() (*types.ValidatorSet, error) { | ||
return types.ValidatorSetFromExistingValidators(s.TMValidators()) | ||
} |
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.
what do you think about removing dependencies between methods? otherwise you may end up refactoring multiple methods after changing one of them. also such deps generate additional function usages. for me it's a bit confusing when i want to promptly iterate through all usages and i see a giant list with deep call hierarchy.
func (s Sequencer) TMValidator() *types.Validator { | |
return &s.val | |
} | |
func (s Sequencer) TMValidators() []*types.Validator { | |
return []*types.Validator{s.TMValidator()} | |
} | |
func (s Sequencer) TMValset() (*types.ValidatorSet, error) { | |
return types.ValidatorSetFromExistingValidators(s.TMValidators()) | |
} | |
func (s Sequencer) TMValidator() *types.Validator { | |
return &s.val | |
} | |
func (s Sequencer) TMValidators() []*types.Validator { | |
return []*types.Validator{&s.val} | |
} | |
func (s Sequencer) TMValset() (*types.ValidatorSet, error) { | |
return types.ValidatorSetFromExistingValidators([]*types.Validator{&s.val}) | |
} |
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.
decline
not important
anyways you supposed to implement stuff in terms of other things. e.g. if TmValidator() changed then TMValidators and TmValset does not change
If you make TmValset and Tmvalidators also do TmValidator from scratch then you change 3x if that calculation changes
I guess it should work, I haven't e2e tested it
PR Standards
Opening a pull request should be able to meet the following requirements
--
PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A
Close ##1134
<-- Briefly describe the content of this pull request -->
For Author:
godoc
commentsFor Reviewer:
After reviewer approval: