Skip to content

chore: add Arc consensus builder modifier#185

Open
westkite1201 wants to merge 2 commits into
circlefin:mainfrom
westkite1201:chore/162-arc-consensus-builder
Open

chore: add Arc consensus builder modifier#185
westkite1201 wants to merge 2 commits into
circlefin:mainfrom
westkite1201:chore/162-arc-consensus-builder

Conversation

@westkite1201

@westkite1201 westkite1201 commented Jun 21, 2026

Copy link
Copy Markdown

Summary

  • Adds an optional ArcConsensusBuilder consensus modifier closure.
  • Keeps the default ArcConsensus::new(ctx.chain_spec()) construction path intact.
  • Adds unit coverage that verifies the modifier is invoked, its returned consensus is used, and accidental double modifier registration is caught in debug builds.

API note

This intentionally removes Clone/Copy from ArcConsensusBuilder. The builder can now hold a Box<dyn FnOnce + Send + 'static> modifier, which cannot be cloned or copied. If ArcConsensusBuilder is considered part of a released public API, this should be treated as an API compatibility note for the release.

Fixes #162.

Verification

  • Duplicate gate: PASS (.context/pre-pr-gates/circlefin-arc-node-162-20260621T153749Z.md)
  • cargo fmt --all --check
  • git diff --check origin/main...HEAD
  • cargo test -p arc-evm-node (75 passed)
  • cargo clippy -p arc-evm-node --all-targets -- -D warnings

@osr21

osr21 commented Jun 21, 2026

Copy link
Copy Markdown

The design here is sound — FnOnce + Send + 'static is the right bound (FnOnce matches the builder being consumed by build_consensus, Send + 'static are required for async node startup), the manual Debug impl is the only valid approach for a Box<dyn FnOnce> field, and the type alias makes the otherwise-unreadable bound navigable. A few points worth addressing before merge:


1. Removing Clone and Copy is a semver-breaking change

// before
#[derive(Debug, Default, Clone, Copy)]
pub struct ArcConsensusBuilder { ... }

// after
#[derive(Default)]
pub struct ArcConsensusBuilder { ... }

Dropping Clone and Copy from a pub struct is a breaking change under semver — any downstream code that clones or copies the builder will no longer compile. The removal is correct (a Box<dyn FnOnce> is neither Clone nor Copy), but if ArcConsensusBuilder is part of a released public API this should be called out explicitly (changelog entry, major version bump, or at minimum a comment in the PR description). If the struct was previously only compiled internally and never shipped in a release, worth confirming that and noting it here.


2. Double with_consensus_modifier silently discards the first modifier

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 ArcConsensusBuilder ever flows through a plugin or component layer where multiple parties might independently call with_consensus_modifier, the first modifier disappears silently. Two low-cost mitigations:

  • Doc comment: "Calling this more than once replaces the previous modifier. To apply multiple transformations, compose them into a single closure before passing."
  • Debug assertion: debug_assert!(self.modify_consensus.is_none(), "consensus modifier already set — previous modifier will be discarded");

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 apply_consensus_modifier is ever refactored or inlined, the test will need to change even if the public behaviour is unchanged. A test against build_consensus would be more robust, though that requires constructing a BuilderContext — if that's too heavyweight, an alternative is to make apply_consensus_modifier pub(crate) and document it as an internal seam, making the visibility explicit rather than implicit.


Future consideration — modifier composability

The FnOnce bound means the API supports exactly one modifier per builder instance. If this becomes an extension point for downstream node customisations (e.g., different teams layering monitoring, validation, or replay-protection wrappers), callers will have to manually compose closures themselves. Not a blocker for this PR, but worth a note in the doc comment if external composability is a goal:

// callers must compose modifiers themselves:
.with_consensus_modifier(|c| {
    let c = modifier_a(c)?;
    modifier_b(c)
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(refactor): Implement ArcConsensusBuilder closure customization

2 participants