Add feerecipient and gas_limit routes #4511
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
g11tech
force-pushed
the
g11tech/keymanager-fee-gas
branch
from
d281210
to
38d5150
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
wemeetagain
reviewed
Sep 8, 2022
wemeetagain
reviewed
Sep 8, 2022
wemeetagain
reviewed
Sep 8, 2022
wemeetagain
reviewed
Sep 8, 2022
wemeetagain
reviewed
Sep 8, 2022
wemeetagain
reviewed
Sep 8, 2022
dapplion
reviewed
Sep 9, 2022
| @@ -152,6 +161,12 @@ export const validatorOptions: ICliCommandOptions<IValidatorCliArgs> = { | |||
| type: "string", | |||
| }, | |||
|
|
|||
| flushKeymanagerProposerConfigs: { | |||
| description: "If keymanager's persisted proposer configs are found, delete them all!", | |||
There was a problem hiding this comment.
This is a very strange flag, is this really necessary?
There was a problem hiding this comment.
this to to clear keymanager proposer configs, although name can be better 🙂 , any suggestions?
There was a problem hiding this comment.
What's the usecase? There's the delete API for that right?
There was a problem hiding this comment.
the use case is you used keymanager api to make proposer data updates, and now want to discard it and switch to proposerFileConfig. Since now proposerFileConfig flag will error since we don't accept proposerFileConfig if there is keymanager persisted proposer data, this flag can be used to discard and clean keymanagaer persisted configs.
There was a problem hiding this comment.
removed the flag! and recommended manual deletion in case of conflict 👍
dapplion
reviewed
Sep 9, 2022
dapplion
reviewed
Sep 9, 2022
dapplion
reviewed
Sep 9, 2022
| builder.enabled !== undefined || | ||
| builder.gasLimit !== undefined | ||
| ) { | ||
| proposerConfig = {graffiti, strictFeeRecipientCheck, feeRecipient, builder}; |
There was a problem hiding this comment.
Does the ValidatorStore really need to know about the idea of a proposerConfig? Feels like it's the API role to construct this object from the other methods
There was a problem hiding this comment.
ummm i don't find the idea of ValidatorStore knowing about the proposer config odd as we also pass proposer data in the addSigner now.
Other getters anyway return defaults if the that particular property is not assigned.
dapplion
reviewed
Sep 9, 2022
| valProposerConfig = parseProposerConfig(args.proposerSettingsFile, defaultConfig); | ||
| } else { | ||
| valProposerConfig = {defaultConfig} as ValidatorProposerConfig; | ||
| } |
There was a problem hiding this comment.
Consider moving to a function below the handler getValProposerConfig(args): ValidatorProposerConfig
There was a problem hiding this comment.
you mean separate out this parseProposerConfig section from getProposerConfigFromArgs?
dapplion
reviewed
Sep 9, 2022
wemeetagain
reviewed
Sep 11, 2022
g11tech
force-pushed
the
g11tech/keymanager-fee-gas
branch
from
7d4b097
to
908f13f
Compare
dapplion
approved these changes
Sep 12, 2022
There was a problem hiding this comment.
Looks good thanks for those sweet e2e tests ❤️
Some comments that are addressable post merge:
- Should builder.enabled and builder.gasLimit be flattened in ProposerConfig?
- The logic of getProposerConfig says that if some property is set everything else won't be applied defaults. Is that expected behaviour? This function is only used in the API but not in the validator store to perform duties right?
ummm actually, i should rename Else, individual gets of I like your idea idea on flattening the builder params 👍 , how about |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add feerecipient and gas_limit routes
Passes the oapi spec test checker locally (since the opai release json is not available, constructed it locally, have asked for an alpha release on keymanager api so the api spec tests check can be enabled)
Closes #4392 Closes #4393
TODO:
- [ ] Enable oapi spec tests (once the keymanager alpha release is done making the json asset avaiilable to be fetched)can be followed up in a separate PR as not show stopperproposerSettingsFileprovidedproposerSettingsFileif there is proposer data saved in the keymanagar proposer dir unless provided with a flush flag:--flushKeymanagerProposerConfigsin which case the directory is deleted at startup