feat: surface inner-frame revert data and implement check_mesh#71
Open
crazywriter1 wants to merge 1 commit into
Open
feat: surface inner-frame revert data and implement check_mesh#71crazywriter1 wants to merge 1 commit into
crazywriter1 wants to merge 1 commit into
Conversation
…rs check NativeTransferHelper.relay now propagates inner call revert data via inline assembly when requireSuccess is true, so blocklist errors like ERR_BLOCKED_ADDRESS surface in tests instead of a generic "Relay reverted". NativeFiatToken.test.ts updated to assert ERR_BLOCKED_ADDRESS for the blocked-address inner frame case (resolves the FIXME). crates/test/checks/src/mesh.rs replaces the todo!() placeholder with a net_peerCount-based peer count check (gossipsub mesh isn't exposed; this is the closest available proxy for healthcheck purposes).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses two unrelated but small in-scope items that surfaced during a repository sweep for
FIXME/todo!()markers:tests/localdev/NativeFiatToken.test.ts—// FIXME no error message for address blocked?The denylist scenarios in
NativeFiatTokenrely onNativeTransferHelper.relayto forward a call into a denylisted address. With the previous helper implementation the inner revert reason was swallowed and replaced with a generic"Relay reverted", so the assertions could only check for that generic string instead of the real on-chain reason ("Blocked address"/ERR_BLOCKED_ADDRESS).NativeTransferHelper.relaynow propagates the inner call's revert data verbatim whenrequireSuccess == true, and the affected tests now assertERR_BLOCKED_ADDRESS(which already exists intests/helpers/NativeCoinControl.ts).crates/test/checks/src/mesh.rs—pub async fn check_mesh(...) { todo!() }The function was exported and reachable as a public API of
arc-checks, so any caller wiring it up would have panicked at runtime. Execution JSON-RPC does not expose gossipsub mesh state, so this PR implementscheck_meshas a documented best-effort proxy usingnet_peerCount(devp2p peer count) plus an explicit skip / not verifiable path when the method is missing or disabled, instead of leaving a panic in the codebase.Changes
1.
contracts/src/mocks/NativeTransferHelper.solrelay(address target, uint256 amount, bool requireSuccess, bytes calldata data)no longer collapses an inner revert into"Relay reverted". WhenrequireSuccessistrueand the inner call reverts, the function now re-emits the original revert payload via inline assembly:Behavior summary:
success == truesuccess == false,requireSuccess == falsesuccess == false,requireSuccess == truerevert("Relay reverted")(generic)revert(<inner returndata>)(propagated)This means callers and tests can now distinguish why the inner frame failed (blocked address, balance, etc.) instead of seeing a single shape for every failure. No production code reads
"Relay reverted"; this helper is only used by tests undertests/localdevandtests/simulation.2.
tests/localdev/NativeFiatToken.test.tsReplaces the placeholder assertion at the
// FIXME no error message for address blocked?site with the existingERR_BLOCKED_ADDRESSregex fromtests/helpers/NativeCoinControl.ts:ERR_BLOCKED_ADDRESS(/Blocked address/) is already used throughout the same file for direct (top-level) blocklist hits; this PR makes the inner-framerequireSuccess=truecase assert the same shape, so denylist behavior is verified end-to-end whether the rejection happens at the top frame or inside a relayed sub-call. TheFIXMEcomment is removed.3.
crates/test/checks/src/mesh.rsReplaces the
todo!()placeholder with a workingcheck_meshthat:(node_name, rpc_url)in the supplied list.expected_peers.passed = trueskip-style result with"mesh: no expected peers configured for this node (skipped)". This keeps callers passing a partial config from being treated as a failure.net_peerCount(5 s request timeout, asyncreqwest::Client).CheckResult:count >= expected.len()→ pass ("net_peerCount=N >= expected M (devp2p peer count as connectivity proxy)").arc-checkswhere missing introspection surfaces degrade gracefully, same wayadmin_*based checks behave).The module documentation explicitly calls out that this is a proxy for the missing libp2p gossipsub mesh introspection. If/when a dedicated RPC (or a different surface like Prometheus metrics) is added, this function can be tightened without breaking existing callers, because the public signature is unchanged:
Note:
check_meshis currently exported but has no callers in this repo. This PR does not wire it into a binary or smoke job; it only ensures the function is implementable and would not panic if invoked by an external tool or a future check pipeline.Why one PR?
Both changes share a single small theme — replace placeholder/error-eating code paths with something that actually carries the underlying signal (revert reason in one case, peer-count signal in the other) — and the diffs are narrow and isolated. Touching all of them in one PR avoids two near-empty branches. If you'd prefer split PRs (Solidity helper + tests vs. Rust check), I'm happy to break it up — let me know.
Files touched
contracts/src/mocks/NativeTransferHelper.soltests/localdev/NativeFiatToken.test.tsERR_BLOCKED_ADDRESSinstead of/Relay reverted/crates/test/checks/src/mesh.rstodo!()withnet_peerCount-based checkTesting
forge build --sizes— clean. Warnings (selfdestruct, payable fallback, unused params) are pre-existing in upstream files unrelated to this PR.forge test -vvv— 279 passed, 0 failed, 1 skipped (pre-existingtestEthCodeHashskip).cargo fmt --all -- --check— clean.cargo clippy -p arc-checks --all-targets -- -D warnings— clean. The newcheck_meshonly depends onreqwest,serde,serde_json,url,color_eyre— all already inarc-checks.make smoke-reth— ran locally on WSL2 (Windows 11). The Reth localdev suite has a number of pre-existing flakes / environment-dependent failures on this local setup that are unrelated to this PR (e.g.genesis: Memo (genesis-placed),genesis: Multicall3From (genesis-placed),ProtocolConfig implementation (salt=0)address mismatch, several mock-CL timing timeouts). The same failure set appears againstorigin/mainwith no local changes applied, so they are not introduced here. The CI smoke job should be the source of truth for those.Non-goals / out of scope
"Relay reverted". The new revert payload is the inner call's exact revert data, which is what most tests/inspectors actually want anyway.Compatibility
NativeTransferHelperis a test mock undercontracts/src/mocks/; no production contract depends on it.check_meshis currently exported but unused inside this repository; behavior is additive.