-
Notifications
You must be signed in to change notification settings - Fork 31
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
support chain spec #138
support chain spec #138
Conversation
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.
in the codebase we are mix-using *, &, ref(), deref()
, at some point we may want to standardise only one pattern, either *, & or ref(), deref()
@@ -46,6 +47,17 @@ pub fn handle_docker_init(config_path: String, output_dir: String) -> Result<()> | |||
|
|||
// config volume to pass to all services | |||
let config_volume = Volumes::Simple(format!("./{}:{}:ro", config_path, CONFIG_DEFAULT)); | |||
let chain_spec_volume = chain_spec_path.as_ref().and_then(|p| { |
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.
maybe:
let get_file_name_str = |p: &Path| -> Option<&str> {
p.file_name()?.to_str()
};
// chain_spec_volume using the helper
let chain_spec_volume = chain_spec_path.as_ref().and_then(|p| {
let file_name = get_file_name_str(p)?;
Some(Volumes::Simple(format!("{}:/{}:ro", p.display(), file_name)))
});
// chain_spec_env using the helper
let chain_spec_env = chain_spec_path.and_then(|p| {
let file_name = get_file_name_str(&p)?;
Some(get_env_val(CHAIN_SPEC_ENV, &format!("/{file_name}")))
});
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.
mm not sure this is more legible
pub fn from_env_path() -> Result<Self> { | ||
let config: Self = load_file_from_env(CONFIG_ENV)?; |
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.
wonder if we can do something like:
impl TryFrom<&'static str> for CommitBoostConfig
and then
let config = CommitBoostConfig::try_from(CONFIG_ENV)?;
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.
Yeah think we can improve in general how configs are loaded, will keep it in a separate PR
Initial support for Commit-Boost, adding for now only the PBS module (equivalent to MEV-Boost). Note that this depends on [this PR](Commit-Boost/commit-boost-client#138) being merged and released. There is sometimes some ambiguity between mev-boost (go client) and mev-boost (protocol via the Builder API), so to disambiguate within commit boost we call the PBS module the sidecar implementing the mev-boost protocol. Here I followed the existing convention as much as possible even if it sounds somewhat repetitive at times (commit-boost-mev-boost). EDIT: the blocking PR is now merged and released in `0.3.0` --------- Co-authored-by: Barnabas Busa <busa.barnabas@gmail.com> Co-authored-by: Barnabas Busa <barnabas.busa@ethereum.org>
fix #12
also needed for #90
add support for different chain specs