refactoring how proposer settings load into validator client #13645
Conversation
james-prysm
requested review from
kasey,
nalepae,
terencechain,
prestonvanloon,
saolyn and
rkapka
and removed request for
terencechain
config/proposer/loader.go
Outdated
| "github.com/urfave/cli/v2" | ||
| ) | ||
|
|
||
| type SettingsType int |
config/proposer/loader.go
Outdated
| type ProposerSettingsLoader interface { | ||
| Load(cliCtx *cli.Context) (*validatorService.ProposerSettings, error) | ||
| } |
There was a problem hiding this comment.
Interface that has only one implementation is kind of useless
There was a problem hiding this comment.
I can take it out for now, was going to make changes with it when #13541 is addressed
config/proposer/loader.go
Outdated
| LoadMethods []SettingsType | ||
| ExistsInDB bool | ||
| Db iface.ValidatorDB |
There was a problem hiding this comment.
These can be lowercase (functionally-wise it's probably irrelevant since the type is not exported anyway)
config/proposer/loader.go
Outdated
| Load(cliCtx *cli.Context) (*validatorService.ProposerSettings, error) | ||
| } | ||
|
|
||
| type proposerSettingsLoader struct { |
There was a problem hiding this comment.
| type proposerSettingsLoader struct { | |
| type settingsLoader struct { |
Type name should not start with package name
config/proposer/loader.go
Outdated
| } | ||
|
|
||
| // ProposerSettingsLoaderOption sets additional flag checks that affect the proposer settings | ||
| type ProposerSettingsLoaderOption func(cliCtx *cli.Context, psl *proposerSettingsLoader) error |
There was a problem hiding this comment.
| type ProposerSettingsLoaderOption func(cliCtx *cli.Context, psl *proposerSettingsLoader) error | |
| type SettingsLoaderOption func(cliCtx *cli.Context, psl *proposerSettingsLoader) error |
Type name should not start with package name
| return p | ||
| } | ||
|
|
||
| func (po *ProposerOption) ToConsensus() *validatorpb.ProposerOptionPayload { |
|
|
config/proposer/loader.go
Outdated
| func (psl *SettingsLoader) processProposerSettings(loadedSettings, dbSettings *validatorpb.ProposerSettingsPayload) *validatorpb.ProposerSettingsPayload { | ||
| dbOnly := false | ||
| if loadedSettings == nil && dbSettings == nil { | ||
| return nil |
There was a problem hiding this comment.
This case is not covered by any test.
There was a problem hiding this comment.
i don't think this is possible since it'd probably only happen on db only but it should exist in the db... can take it out if we want but also think it's fine to keep it in
| @@ -136,6 +147,35 @@ type ProposerOption struct { | |||
| BuilderConfig *BuilderConfig | |||
| } | |||
|
|
|||
| // Clone creates a deep copy of proposer option | |||
| func (po *ProposerOption) Clone() *ProposerOption { | |||
| if po == nil { | |||
There was a problem hiding this comment.
Is it even possible to enter into this function if po == nil?
There was a problem hiding this comment.
probably not i think... don't remember by we added those checks
There was a problem hiding this comment.
nvm it's used on fee recipient APIs reverted this...
| } | ||
|
|
||
| func (po *ProposerOption) ToConsensus() *validatorpb.ProposerOptionPayload { | ||
| if po == nil { |
There was a problem hiding this comment.
Is it even possible to enter into this function if po == nil?
There was a problem hiding this comment.
happens on key manager APIs
There was a problem hiding this comment.
You'r right, I just learned in go it was possible to do:
f.bar()even i f is nil.
Like:
==> Should we add receiver nilness check in each method?
Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
thing still need to be exported here for db and api use as well as tests |
What type of PR is this?
Other
What does this PR do? Why is it needed?
the proposer settings is a configuration that is saved on the validator client both in memory and in the DB to provide configuration information such as fee recipients or configuring a validator to use a mev builder. The current implementation is littered with inconsistent checks and hopefully this will improve readability.
fixes #13797