refactor: extract foreign-chain-health-check library from config-tester

[haiyuechen-nearone]

Part of #3764. The plan (stacked, merge bottom-up):

1. refactor: extract foreign-chain-health-check library from config-tester #3797 (this PR) — extract a shared foreign-chain-health-check library from the config-tester CLI
2. feat(node): probe foreign-chain RPC providers on startup #3848 — probe every configured provider on node startup + per-provider logs
3. feat(node): expose foreign-chain RPC provider health metric #3849 — per-chain configured / healthy Prometheus counts
4. feat(node): surface healthy provider counts on /debug/node_config #3850 — healthy counts on /debug/node_config

Moves the golden vectors, per-provider checks, and check_all_providers out of the CLI into a new production crate, so the node can reuse them without depending on the CLI (which pulls in clap/serde_yaml/toml). The extraction itself is behavior-neutral.

Notes for reviewers:

• The CLI keeps arg parsing, config-shape detection, and table rendering.
• To keep clap out of the library, Network is parsed via a FromStr impl (dedicated ParseNetworkError) instead of clap::ValueEnum, and the CLI takes --network as Option<Network> directly. Accepted values are unchanged (mainnet/testnet); the only user-visible difference is the invalid-value error text (now expected one of: mainnet, testnet).

[claude]

Pull request overview

Extracts the provider-probing logic, golden vectors, and per-chain checks out of the foreign-chain-config-tester CLI into a new foreign-chain-health-check library so the MPC node can reuse them without pulling in clap/serde_yaml/toml. Network gets a FromStr impl (plus ParseNetworkError) instead of clap::ValueEnum, and ProviderResult/Status move to a shared results module. The CLI keeps arg parsing, config-shape detection, and table rendering.

Changes:

• New crates/foreign-chain-health-check crate exposing check_all_providers, Network, ProviderResult, Status.
• Move checks.rs, golden.rs, and the run/prepare/mark helpers out of config-tester/src/main.rs into the new crate.
• CLI now imports from the library; drops bs58, http, hex, foreign-chain-*, and test-only deps.
• Network becomes clap-free (parsed via FromStr); CLI arg switches from value_enum to value_name.

Reviewed changes

Per-file summary

File	Description
Cargo.toml, Cargo.lock	Register the new workspace member and dependency.
crates/foreign-chain-health-check/Cargo.toml	New crate manifest with the deps the extracted code needs.
crates/foreign-chain-health-check/src/lib.rs	New library entry point: check_all_providers plus private orchestration helpers.
crates/foreign-chain-health-check/src/network.rs	Network enum with FromStr / ParseNetworkError, no clap dep.
crates/foreign-chain-health-check/src/results.rs	ProviderResult / Status moved from report.rs.
crates/foreign-chain-health-check/src/checks.rs	Renamed from the CLI; only trivial import changes.
crates/foreign-chain-health-check/src/golden.rs	Renamed from the CLI; Network moved out to network.rs.
crates/foreign-chain-config-tester/Cargo.toml	Drop deps now provided by the new library.
crates/foreign-chain-config-tester/src/main.rs	Slims to CLI wiring; delegates to check_all_providers.
crates/foreign-chain-config-tester/src/report.rs	Re-exports ProviderResult/Status from the new crate.
crates/foreign-chain-config-tester/src/config.rs	Import Network from the new crate.

Findings

Blocking (must fix before merge):

• crates/foreign-chain-health-check/src/lib.rs:44-83 -- This is not a behavior-neutral extraction (contrary to the PR description). The old run() had an else-branch calling mark_not_configured for every chain (base, bnb, arbitrum, polygon, hyper_evm, abstract, bitcoin, starknet, aptos, sui, ethereum, solana) -- see the deleted lines 182-241 of config-tester/src/main.rs. The new check_all_providers keeps that branch only for sui. Result: the CLI-rendered table previously included a - not configured row for every absent chain; now it silently omits every unconfigured chain except sui. Operators lose a signal they had been getting (I forgot to configure X). The sui-only exception is also inconsistent within the function itself. Two reasonable fixes:

  • Restore parity (matches behavior-neutral): re-add the else branch for every chain, symmetric with the old code.
  • Move the list-absent-chains responsibility to the CLI: have the library only report configured providers (drop the sui special case too), and have foreign-chain-config-tester/src/report.rs synthesize placeholder rows for chains not present in results. Then update the PR description -- this is no longer a pure refactor.

  Either way, the current asymmetry should not land.

• crates/foreign-chain-health-check/src/lib.rs tests (check_all_providers__should_report_absent_sui_as_not_configured, check_all_providers__should_report_pass_fail_and_skip_in_one_run) -- these were relaxed to match the new one-chain-only behavior. Once the behavior above is fixed, restore the stronger assertion the old test carried in spirit (all configured-but-absent chains should still appear).

Non-blocking (nits, follow-ups, suggestions):

• crates/foreign-chain-health-check/src/lib.rs:6-7 -- pub mod network; / pub mod results; together with the pub use network::... / pub use results::... at line 27-28 gives every type two importable paths (foreign_chain_health_check::Network and foreign_chain_health_check::network::Network). The CLI already uses only the re-exports; making the modules private (mod network; / mod results;) removes the dual paths.
• crates/foreign-chain-health-check/src/lib.rs:77 -- the comment // Configured but not yet supported by the node. reads as paraphrase now that the see verify_foreign_tx/sign.rs pointer was dropped. Either restore the pointer (why it is unsupported: sign-path does not handle it yet) or drop the comment entirely per engineering-standards Write helpful code comments.
• crates/foreign-chain-health-check/src/network.rs:12 -- pub const ALL is exposed publicly but the only in-crate use is the private labels() helper. Consider keeping it pub(crate) unless a downstream consumer is anticipated.
• crates/foreign-chain-health-check/src/checks.rs:150-153 -- the doc comment on check_sui now describes the probe as a transaction from a recent checkpoint (a few behind the tip). The magic few is CHECKPOINT_PROBE_OFFSET = 10; linking the constant removes the vague wording.

Warning: Issues found

[anodar]

Please have a look at Blocking findings from claude and either address or explain why it doesn't apply.

In this particular case, I believe it's false positive:

The new check_all_providers keeps that branch only for sui. Result: the CLI-rendered table previously included a - not configured row for every absent chain; now it silently omits every unconfigured chain except sui.

clearly other chains have same exact branch for emitting unconfigured chain log.

[anodar]

Why drop value_enum? Auto-detection of contract doesn't always work (e.g. dev clusters), and with value_enum it would probably show possible values [mainnet | testnet].

[haiyuechen-nearone]

I wanted to keep the new foreign-chain-health-check crate completely free of clap. Since Network is moved into the new crate, deriving value_enum would pull clap into the new crate as well. CLI crate is also not allowed to implement value_enum from the outside either in the current dependency setup.

I made sure that the custom error string still lists the valid values, and its generated based on the code values so it won't drift.

I do see that this is quite a lot of code to achieve a worse version of what clap already offers. Should I instead pull in clap as optional dependency in foreign-chain-health-check?

[anodar]

You can avoid pulling clap in that library crate by having separate CLI enum in main.rs e.g. NetworkArg and then implement from trait for Network (type from foreign chain health check lib) to convert CLI Network Arg type into it.

[haiyuechen-nearone]

I think pulling in optional clap dependency is better here and implemented that 58380d5. I feel having a separate CLI enum does allow using clap's CLI features, but additionally need to introduce more code to make sure the CLI enum and the lib enum are in sync.

[anodar]

with a fixed golden request

Not for SUI though.

[gilcu3]

is it not possible for SUI, or just not yet implemented?

[anodar]

Not possible for SUI.

mpc/crates/foreign-chain-config-tester/README.md

Line 10 in 2b539b7

the result against a known-good value. Sui is the exception: its providers prune

Sui is the exception: its providers prune
transactions after a few weeks, so the check instead verifies the provider's
chain identity and inspects a transaction from its latest checkpoint. 

[haiyuechen-nearone]

Added comments about SUI being an exception

[haiyuechen-nearone]

Please have a look at Blocking findings from claude and either address or explain why it doesn't apply.

That review did apply to an earlier version, and the blockers were addressed in c689402

Do we only trigger Claude review on the first publish?

[anodar]

Do we only trigger Claude review on the first publish?

you can re-trigger it using @claude review.

[claude]

Pull request overview

Extracts the provider-probing logic, golden vectors, and per-chain checks out of foreign-chain-config-tester into a new foreign-chain-health-check library so the MPC node can reuse them without pulling in clap/serde_yaml/toml. Adds a FromStr impl (with dedicated ParseNetworkError) for Network so the shared crate stays free of clap, and moves ProviderResult / Status into a shared results module. The CLI keeps arg parsing, config-shape detection, and table rendering.

Re-review after c689402: the previously-flagged blocking issues (per-chain not_configured parity, module visibility, pub(crate) const ALL, restored verify_foreign_tx/sign.rs reference, CHECKPOINT_PROBE_OFFSET doc link) have all been addressed. Behavior of the CLI table matches the previous version.

Changes:

• New crates/foreign-chain-health-check exposing check_all_providers, Network, ParseNetworkError, ProviderResult, Status.
• Moves checks.rs, golden.rs, and the run_* / prepare_* / mark_* helpers into the new crate.
• CLI now delegates to check_all_providers; drops bs58, hex, http, foreign-chain-*, and test-only deps.
• Network becomes clap-free (parsed via FromStr returning ParseNetworkError); CLI arg switches from value_enum to value_name.

Reviewed changes

Per-file summary

File	Description
Cargo.toml, Cargo.lock	Register new workspace member and dependency.
crates/foreign-chain-health-check/Cargo.toml	New crate manifest (adds thiserror, drops CLI-only deps).
crates/foreign-chain-health-check/src/lib.rs	Library entry point; check_all_providers orchestrates per-chain probes and placeholder rows.
crates/foreign-chain-health-check/src/network.rs	Network enum + FromStr + ParseNetworkError.
crates/foreign-chain-health-check/src/results.rs	ProviderResult / Status moved out of report.rs.
crates/foreign-chain-health-check/src/checks.rs	Renamed from CLI; trivial import changes + check_sui doc-link tweak.
crates/foreign-chain-health-check/src/golden.rs	Renamed from CLI; Network moved to network.rs; sui doc points at check_sui.
crates/foreign-chain-config-tester/Cargo.toml	Drop deps now owned by the new library.
crates/foreign-chain-config-tester/src/main.rs	Slims to arg parsing + delegation to check_all_providers.
crates/foreign-chain-config-tester/src/report.rs	Re-uses ProviderResult / Status from the new crate.
crates/foreign-chain-config-tester/src/config.rs	Imports Network from the new crate.

Findings

Non-blocking (nits, follow-ups, suggestions):

• crates/foreign-chain-health-check/src/network.rs:20-32 — ParseNetworkError's #[error(...)] template calls Network::labels(), which allocates a String (Vec + join) on every Display/to_string call. Not hot, but for a two-variant enum a static "expected one of: mainnet, testnet" would be simpler and allocation-free. If you keep the dynamic form, at least ensure it stays consistent with Network::ALL — worth a tiny unit test.
• crates/foreign-chain-health-check/src/network.rs — new module with a FromStr impl and a dedicated error type has no direct unit test. Per [engineering-standards.md §Add tests], a test asserting "mainnet".parse::<Network>() / "testnet".parse::<Network>() succeed and something like "Mainnet"/"foo" yields ParseNetworkError would lock in the CLI-visible contract (the invalid-value text is described in the PR body as the one user-visible change) and prevent silent regressions on casing.
• crates/foreign-chain-health-check/src/lib.rs:42-108 — 12 hand-rolled if let Some(cfg) = &fc.<chain> { … } else { mark_not_configured(...) } blocks. Since the follow-up PRs (feat(node): probe foreign-chain RPC providers on startup #3848–feat(node): surface healthy provider counts on /debug/node_config #3850) will read the same set of chains for logging and metrics, consider whether ForeignChainsConfig should expose an iterator (or a for_each_chain visitor) so future additions don't require touching every consumer. Out of scope for this refactor; flagging for the stack.
• crates/foreign-chain-health-check/src/lib.rs:52-83 — run_evm / run_bitcoin / run_starknet / run_aptos / run_sui are awaited sequentially, and each iterates its providers sequentially inside. This is fine for the CLI, but once the node calls check_all_providers on startup (feat(node): probe foreign-chain RPC providers on startup #3848) this becomes N chains × M providers × per-request timeout of blocking wall time. Consider FuturesUnordered / join_all per chain, and possibly across chains, when wiring the node path.
• crates/foreign-chain-health-check/src/checks.rs:150-153 — the doc reads "runs the real inspector over a transaction [CHECKPOINT_PROBE_OFFSET] checkpoints behind the tip." The bracketed intra-doc link inserted into an English sentence renders oddly ("over a transaction 10 checkpoints…" would read cleaner if the constant were mentioned by name, e.g. "from a checkpoint [CHECKPOINT_PROBE_OFFSET] behind the tip"). Purely a wording nit.

✅ Approved

[gilcu3]

Thank you!