Repository navigation
Conversation
|
|
48ddaad to
2859072
Compare
pawanjay176
left a comment
There was a problem hiding this comment.
Thanks for the PR @0xfornax . The code looks good to me.
My concern is more about policy.
This PR effectively overwrites global builder urls, and other configs with cli params.
This means that you could accidentally delete a config by passing cli params. I think this is okay as long as this is documented. But in terms of actual users, I'm not sure how they would use this.
A potential change in policy is just appending the cli params to the config and throwing an error only if the same builder is present in both lists with different configs.
I would defer to you and @yorickdowne as to which is the better policy based on your experience with users.
|
For consistency with the rest of Lighthouse, the CLI params should be the default/fallback for the case where no value is specified in the config. I think we should keep that the same here unless there's a very good reason to change it. |
2859072 to
a2f01eb
Compare
|
Thanks for the feedback @michaelsproul and @pawanjay176! I've adjusted the precedence so it's: Per-validator YAML > global defaults YAML > CLI. |
|
Per-validator yaml likely also covers per-validator keymanager. Just pointing out to please make sure it’s at that level of precedence and doesn’t get overriden by CLI |
|
yeah keymanager writes to the config so it is one and the same |
a2f01eb to
45e7446
Compare
Issue Addressed
Implements #10185.
Proposed Changes
Adds
--builder-urls,--builder-min-bid, and--builder-max-execution-paymentfor Gloas, and wires the existing--builder-boost-factorinto the builder configuration.CLI settings will be a fallback for global YAML defaults, preserving Add per-validator builder configuration API #9864 's per-validator precedence and persistence behavior. The order is: per-validator YAML > global YAML > CLI > defaults.
AI assistance
This PR was AI-assisted, but I understand the changes and have tested them thoroughly on a local kurtosis network.