-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: refactor config and add ChainConfig #108
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.
this is made to work like before: we have default values for all chains, and CLI arguments "slot_time_in_seconds" and "commitment_deadline" are optional and override the defaults.
/// The slot time duration in seconds. If provided, | ||
/// it overrides the default for the selected [Chain]. | ||
#[clap(short = 's', long, default_value_t = DEFAULT_SLOT_TIME_IN_SECONDS)] | ||
slot_time_in_seconds: u64, |
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.
Let's just do slot_time
and add a description saying it's in seconds!
#[clap(short = 'k', long)] | ||
pub(super) private_key: Option<String>, | ||
/// URL for the commit-boost sidecar | ||
#[clap(short = 'C', long, conflicts_with("private_key"))] |
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.
Didn't know conflicts_with
, sick!
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.
just ported over, work of @thedevbirb :)
@coderabbitai review |
Actions performedReview triggered.
|
WalkthroughThe changes refactor the Changes
Assessment against linked issues
Poem
Tip AI model upgrade
|
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
bolt-sidecar/src/config/signing.rs (1)
1-16
: Ensure consistency in documentation comments.The documentation comments for the fields
private_key
andcommit_boost_url
are clear and concise. Ensure that similar comments are consistently used throughout the codebase for better readability and maintainability.bolt-sidecar/src/config/chain.rs (1)
1-31
: Ensure consistency in documentation comments.The documentation comments for constants and fields are clear and concise. Ensure that similar comments are consistently used throughout the codebase for better readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- bolt-sidecar/bin/sidecar.rs (1 hunks)
- bolt-sidecar/src/builder/mod.rs (2 hunks)
- bolt-sidecar/src/builder/signature.rs (3 hunks)
- bolt-sidecar/src/config/chain.rs (1 hunks)
- bolt-sidecar/src/config/mod.rs (7 hunks)
- bolt-sidecar/src/config/signing.rs (1 hunks)
- bolt-sidecar/src/lib.rs (1 hunks)
Additional comments not posted (15)
bolt-sidecar/src/lib.rs (1)
28-28
: LGTM! Ensure consistency in imports and exports.The export statement has been updated correctly to export
ChainConfig
instead ofChain
.bolt-sidecar/src/config/chain.rs (5)
32-47
: LGTM! Ensure default values are correctly documented.The
ChainConfig
struct and its fields are well-defined with appropriate default values. The documentation comments are clear and concise.
48-57
: LGTM! Default implementation is correct.The
Default
implementation forChainConfig
is correct and aligns with the defined default values.
58-68
: LGTM! Enum definition is clear.The
Chain
enum is well-defined and uses theValueEnum
trait for command-line argument parsing.
70-119
: LGTM! Methods forChainConfig
are well-defined.The methods for
ChainConfig
are well-defined and provide necessary functionality for chain-specific configurations.
122-152
: LGTM! Test functions are well-defined.The test functions for different chain configurations are well-defined and ensure the correctness of the
ChainConfig
methods.bolt-sidecar/src/builder/signature.rs (2)
10-18
: LGTM! Ensure consistency in function signatures.The function signature for
sign_builder_message
has been updated correctly to useChainConfig
.
36-36
: LGTM! Ensure consistency in function signatures.The function signature for
verify_signed_builder_message
has been updated correctly to useChainConfig
.bolt-sidecar/src/builder/mod.rs (1)
16-16
: LGTM! Ensure consistency in struct definitions.The
LocalBuilder
struct has been updated correctly to useChainConfig
instead ofChain
.bolt-sidecar/bin/sidecar.rs (1)
41-41
: Ensure the newcommitment_deadline
method is correctly implemented.The
commitment_deadline
is now accessed throughconfig.chain.commitment_deadline()
. Verify that this method returns the expected value.Verification successful
The
commitment_deadline
method is correctly implemented.The
commitment_deadline
method inChainConfig
returns aDuration
derived from thecommitment_deadline
field, which is properly defined and initialized.
bolt-sidecar/src/config/chain.rs: pub fn commitment_deadline(&self) -> Duration
bolt-sidecar/src/config/chain.rs: commitment_deadline: u64
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `commitment_deadline` method in the `ChainConfig` struct. # Test: Search for the `commitment_deadline` method definition. Expect: A method that returns the commitment deadline. rg --type rust $'fn commitment_deadline' --context 5Length of output: 676
Script:
#!/bin/bash # Description: Verify the definition and initialization of `commitment_deadline` within the `ChainConfig` struct. # Test: Search for the `commitment_deadline` field in the `ChainConfig` struct. rg --type rust 'commitment_deadline' --context 5Length of output: 10655
bolt-sidecar/src/config/mod.rs (5)
7-9
: LGTM! TheChainConfig
module is correctly introduced and used.The
ChainConfig
module is correctly introduced and used in theOpts
andConfig
structs.
10-11
: LGTM! TheSigningOpts
module is correctly introduced and used.The
SigningOpts
module is correctly introduced and used in theOpts
struct.
59-61
: LGTM! TheChainConfig
is correctly used in theOpts
struct.The
ChainConfig
is correctly used in theOpts
struct.
98-98
: LGTM! TheChainConfig
is correctly used in theConfig
struct.The
ChainConfig
is correctly used in theConfig
struct.
203-204
: LGTM! TheChainConfig
is correctly assigned in thetry_from
implementation.The
ChainConfig
is correctly assigned in thetry_from
implementation for theConfig
struct.
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.
LGTM, good refactor!
config
module #106Summary by CodeRabbit
New Features
ChainConfig
for enhanced chain configuration management, providing defaults for commitment deadlines and slot times.Refactor
Chain
toChainConfig
across various modules for better clarity and organization.ChainConfig
.Chores
commitment_deadline
.SigningOpts
struct for signing configurations.