fix(cli): refuse ambiguous EVM network selection (devnet manifest vs mainnet default)#158
fix(cli): refuse ambiguous EVM network selection (devnet manifest vs mainnet default)#158Nic-dorman wants to merge 9 commits into
Conversation
… (V2-744)
Port-range parsing ("12000" / "12000-12004") lived in ant-cli while the
PortRange type lives in ant-core, so every frontend had to reimplement
it. Parse via the standard FromStr trait next to the type instead; the
CLI now just calls .parse().
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every frontend passing env vars to AddNodeOpts needs the same KEY=VALUE parsing the CLI hand-rolled; expose it as AddNodeOpts::parse_env_vars. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(V2-742) The explicit-peers > devnet-manifest > bootstrap_peers.toml priority logic (with manifest-peer socket-addr filtering) lived in ant-cli, but it's business logic every frontend needs. Move it to config::resolve_bootstrap_peers; the no-source case is now a typed Error::NoBootstrapPeers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ant node add / ant node reset hand-rolled reqwest POSTs against the daemon while every other endpoint goes through ant_core::node::daemon::client. Add client::add_node and client::reset and use them from the CLI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ant node start/stop --service-name read node_registry.json directly to translate name -> ID even while the daemon (the registry's owner) is running, so the daemon could mutate the registry between the CLI's read and the API call. Add client::resolve_node_id_by_name, which resolves through GET /nodes/status, and drop the CLI's direct registry read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CliProgress (node add, unicode bar to stdout) and CliUpdateProgress (self-update, plain percent to stderr) were near-identical impls of the same trait. Keep one CliProgress in ant-cli/src/progress.rs, writing to stderr like the rest of the progress UI. Also fixes: ant node add --json no longer interleaves download-progress output with the JSON result (NoopProgress in JSON mode, matching ant update). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e308d08 to
04cd79a
Compare
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Reviewed current head 4ba6f8d276ab7c8008708e9fe6cd21229a1deed2, including the one-commit EVM-selection delta separately from #157. The EVM resolver logic looks sound: mainnet remains the default without manifest EVM config, the ambiguous manifest case fails before payment, explicit presets override loudly, local validates manifest values, and read-only file/chunk operations do not resolve EVM. All 12 CI checks are green; focused EVM resolver tests and cargo check --all-targets also pass locally.
Hold this until #157 is corrected and merged/rebased. This PR currently targets main and carries all seven #157 commits, including the same unrelated vendor/**/.cache, lock and .orig artifacts and the empty-manifest resolve_bootstrap_peers -> Ok([]) contract issue described on #157. If #158 merged first, it would merge those issues too.
Please also update the PR body: it says this is stacked on closed #155 and that CI will run after retargeting, but the live PR targets main, is mechanically stacked on #157, and CI is already green. Once #157 lands with the two fixes, rebase/refresh #158 so its GitHub diff is the intended EVM-only delta.
No blocker was found in #158's own EVM-selection code.
…tifacts resolve_bootstrap_peers now returns NoBootstrapPeers when a selected manifest is empty or every entry is filtered out, instead of Ok([]) — a selected manifest never falls back to the public config. Also removes accidentally committed vendor cache/.orig files and ignores them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t-core (V2-743, V2-471) --evm-network defaulted to arbitrum-one unconditionally, so a user passing --devnet-manifest with a custom EVM block (but not --evm-network local) silently paid against the mainnet vault address on the manifest's chain: the tx 'succeeds' as a no-op against an EOA, gas is spent, ANT allowance goes to a useless spender, and every chunk PUT then fails median-payment verification. The failure reads like a payment-protocol bug, not a flag bug (V2-471). - string->EvmNetwork resolution (incl. manifest evm-block parsing for 'local') moves to ant_core::config::resolve_evm_network with typed errors, per the V2-189 audit (finding #5) — frontends share one implementation - the default stays arbitrum-one EXCEPT when a devnet manifest carrying an evm block is loaded: that combination now errors (Error::EvmNetworkAmbiguous) and asks for an explicit choice, so the trap path fails loudly before any tokens move while plain mainnet usage is unchanged - selecting a preset while the manifest carries an EVM block warns that the manifest's EVM config is ignored - ant-cli drops its direct reqwest dependency (last use was the moved URL parse) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4ba6f8d to
243a462
Compare
|
Rebased onto the corrected #157 head (ec6d5d5), so this branch now carries the vendor-artifact removal and the Holding this until #157 merges, then I'll rebase onto |
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Re-reviewed current head 243a462fb625c28a69756984c1e0f13d891ed886. The branch now has corrected #157 head ec6d5d5d7992cd1a1f2296dec26568137b3d1076 as its exact parent, and the sole EVM-selection delta is semantically unchanged from the previously reviewed commit. The inherited artifact/bootstrap findings are resolved, and no blocker was found in the EVM logic.
Verified locally on this exact head: cargo fmt --all -- --check, four bootstrap resolver tests, six EVM resolver tests, and cargo check --all-targets all pass. The EVM behavior remains correct for default mainnet, ambiguous manifest config, explicit presets, local, malformed manifest values, and read-only operations.
Current CI is 10/12 green with no failures; macOS and Ubuntu E2E remain in progress. Merge #157 first, then rebase/refresh this branch onto main so the final diff is EVM-only. Subject to that ordering and the remaining E2E jobs finishing green, code review is clear.
2 participants
Summary
Fixes the silent-payment-failure trap around
--evm-networkand completes audit finding #5 (EVM resolution belongs in ant-core). Targetsmain, mechanically stacked on #157 — the GitHub diff currently includes #157's commits. Merge #157 first, then this branch will be rebased so its diff is the EVM-only delta. CI is green on the stacked branch.The trap:
--evm-networkdefaulted toarbitrum-oneunconditionally, so--devnet-manifestwith a custom EVM block but without--evm-network localsilently paid against the hardcoded mainnet vault address on the manifest's chain. On Sepolia that address is an EOA: the tx "succeeds" as a no-op, gas is spent, the ANT allowance goes to a useless spender, and every subsequent chunk PUT fails median-payment verification — looking like a payment-protocol bug rather than a flag bug.Changes:
EvmNetworkresolution (including manifestevm-block parsing forlocal) moves toant_core::config::resolve_evm_networkwith typed errors (EvmNetworkAmbiguous,UnsupportedEvmNetwork,EvmManifestRequired,InvalidEvmManifest), so every frontend shares one implementation. (EvmNetworkandDevnetManifestare foreign re-exports, so the resolver is a free function inant_core::confignext toresolve_bootstrap_peersrather than an inherent method.)arbitrum-one— except when a devnet manifest carrying anevmblock is loaded. That one combination (the only path where the trap fires) now errors and asks for an explicit choice:localto use the manifest's EVM config, or a preset to override it.reqwestdependency — the moved URL parse was the last use.Testing
resolve_evm_network(mainnet default with no manifest / manifest-without-evm, ambiguous error when manifest has an evm block, preset mapping,localhappy path with URL/address round-trip, missing manifest/evm block, unknown name, bad manifest values); full workspace suite passes; clippy/fmt clean.local+ manifest → resolves;mainnet→ unsupported error;localwithout manifest → manifest-required error.🤖 Generated with Claude Code