chore: add Arc consensus builder modifier#185
Conversation
|
The design here is sound — 1. Removing // before
#[derive(Debug, Default, Clone, Copy)]
pub struct ArcConsensusBuilder { ... }
// after
#[derive(Default)]
pub struct ArcConsensusBuilder { ... }Dropping 2. Double pub fn with_consensus_modifier<F>(mut self, modify_consensus: F) -> Self {
self.modify_consensus = Some(Box::new(modify_consensus));
self
}Calling this twice replaces the existing modifier with no warning. For a direct-construction call-site this is probably fine, but if
The debug assertion costs nothing in release builds and will catch accidental double-sets during development. 3. Test exercises a private method directly let modified = ArcConsensusBuilder::default()
.with_consensus_modifier(...)
.apply_consensus_modifier(consensus.clone()) // private
.expect("...");This is valid Rust (child modules can access private items from the parent module), but it tests the private method rather than the observable public contract. If Future consideration — modifier composability The // callers must compose modifiers themselves:
.with_consensus_modifier(|c| {
let c = modifier_a(c)?;
modifier_b(c)
}) |
|
Points 2 and 3 from the earlier review are addressed well — the One new issue in this commit, and one still-open point: New: #[test]
#[should_panic(expected = "consensus modifier already set")]
fn arc_consensus_builder_panics_on_double_modifier_in_debug() {
let _builder = ArcConsensusBuilder::default()
.with_consensus_modifier(Ok)
.with_consensus_modifier(Ok);
}
Fix: gate the test on #[test]
#[cfg(debug_assertions)]
#[should_panic(expected = "consensus modifier already set")]
fn arc_consensus_builder_panics_on_double_modifier_in_debug() {
let _builder = ArcConsensusBuilder::default()
.with_consensus_modifier(Ok)
.with_consensus_modifier(Ok);
}
Still open: From the earlier comment — removing both derives from a |
Summary
ArcConsensusBuilderconsensus modifier closure.ArcConsensus::new(ctx.chain_spec())construction path intact.API note
This intentionally removes
Clone/CopyfromArcConsensusBuilder. The builder can now hold aBox<dyn FnOnce + Send + 'static>modifier, which cannot be cloned or copied. IfArcConsensusBuilderis considered part of a released public API, this should be treated as an API compatibility note for the release.Fixes #162.
Verification
.context/pre-pr-gates/circlefin-arc-node-162-20260621T153749Z.md)cargo fmt --all --checkgit diff --check origin/main...HEADcargo test -p arc-evm-node(75 passed)cargo clippy -p arc-evm-node --all-targets -- -D warnings