feat: probe Aptos providers by ledger chain id by haiyuechen-nearone · Pull Request #4069 · near/mpc

haiyuechen-nearone · 2026-08-05T15:31:46Z

Closes #4092.

Aptos reports its chain id in the ledger info every node serves at the REST root, so AptosRpcClient gains a get_ledger_info call and the inspector reads chain_id from it.

Notes for review

A 404 means different things depending on which method was called.
- get_transaction_by_hash: the transaction is absent.
- get_ledger_info: the endpoint does not serve an Aptos API.
- The meaning is modeled as traits on the response type.
- The shared types live in the foreign-chain-instpector crate root because Sui needs the same distinction for gRPC NOT_FOUND.
The transport step and the decode step fail with their own types. Splitting the parsing into two steps to identify network errors from permanent faults.

claude · 2026-08-07T19:53:37Z

Pull request overview

Adds an Aptos arm to the network-fingerprint probe. AptosRpcClient gains get_ledger_info, which GETs the REST base (/v1) and reads chain_id out of the ledger info; AptosInspector implements NetworkFingerprintInspector on top of it, and probe_all_providers now builds an AptosInspector per configured provider instead of falling through to ProbeNotImplemented. Along the way the reqwest client's request/decode steps are split into separate error variants so a body that will not decode is distinguishable from a transport failure, and a 404 is interpreted per response type (TransactionResponse → transaction absent, LedgerInfoResponse → the endpoint does not serve the Aptos API).

Changes:

AptosRpcClient::get_ledger_info + LedgerInfoResponse (partial: chain_id only) + canonical_chain_id_text; ReqwestAptosClient refactored onto a shared get_json helper and a new AptosRpcError::MalformedBody variant.
New crate-private AbsenceMeaning / HasAbsenceMeaning / ClassifyRpcOutcome in foreign-chain-inspector, replacing the inline map_err in AptosInspector::extract and giving 404 a per-response-type meaning.
NetworkFingerprintInspector for AptosInspector; probe_all_providers handles ForeignChain::Aptos; TODO(#4003) narrowed to Sui.
Unit, integration and (ignored) live-RPC tests; docs table + fingerprint-field prose updated for aptos.

Reviewed changes

Per-file summary

File	Description
crates/foreign-chain-rpc-interfaces/src/aptos.rs	Adds `LedgerInfoResponse`, `get_ledger_info`, `canonical_chain_id_text`, `AptosRpcError::MalformedBody`; factors both requests through `get_json` (bytes + `serde_json::from_slice` instead of `response.json()`).
crates/foreign-chain-inspector/src/lib.rs	Adds `pub(crate)` `AbsenceMeaning`, `HasAbsenceMeaning`, `ClassifyRpcOutcome`.
crates/foreign-chain-inspector/src/aptos/inspector.rs	Implements `NetworkFingerprintInspector`, the `HasAbsenceMeaning` impls and `ClassifyRpcOutcome for Result<T, AptosRpcError>`; `extract` now calls `.classified()`. Mock client extended with a ledger-info slot.
crates/foreign-chain-health-check/src/probe.rs	Aptos probe arm + two `probe_all_providers` tests (healthy / wrong network); `TODO(#4043)` note on the constructor coupling.
crates/foreign-chain-inspector/tests/aptos_inspector.rs	Integration tests: fingerprint read from `/v1`, and a 200 non-JSON body rejected as malformed.
crates/foreign-chain-inspector/tests/aptos_rpc_manual.rs	Ignored live-RPC test pinning mainnet fingerprint `"1"`, mirroring the starknet one.
docs/foreign-chain-transactions.md	Probe table row for aptos; normalization note; aptos added to the list of chains that read `expected_network_fingerprint`.

I did not build or run the test suite (cargo invocations were not permitted in this environment), so the notes below come from reading the code.

Findings

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

crates/foreign-chain-rpc-interfaces/src/aptos.rs:51 / crates/foreign-chain-inspector/src/aptos/inspector.rs:150 — the MalformedBody split silently reclassifies undecodable bodies on the production signing path, not just the probe. Previously response.json::<TransactionResponse>() produced a reqwest decode error → AptosRpcError::Http(_) → RpcRequestFailed, which is_transient(). Now it is MalformedRpcResponse, which is not. crates/node/src/providers/verify_foreign_tx.rs:109 builds AptosInspector for foreign-tx verification, so in FanOut::extract: two Aptos providers configured, provider A answers HTTP 200 with an HTML landing/captive-portal page while provider B returns the real transaction → A is now a substantive non-transient verdict, inspectors_split_between_success_and_failure trips, and the request fails with InspectorResponseMismatch. Before, A was dropped from the quorum and B's answer stood. This does align Aptos with how the jsonrpsee chains classify parse failures (classify_rpc_client_error → MalformedRpcResponse), so it may well be the behavior you want — but it is outside the PR's stated scope and worth stating explicitly in the description so it reads as a deliberate call rather than a side effect. (The is_timeout() split in the same match is transience-preserving — both arms were already transient — it only changes the reported ProviderFailure from Unreachable to TimedOut.)
crates/foreign-chain-rpc-interfaces/src/aptos.rs:85 — the base field doc ("REST base including the /v1 segment; the resource path is appended per request") no longer holds for every request: get_ledger_info uses the base itself as the resource and appends nothing. Suggest something like "REST base including the /v1 segment, which is the ledger-info resource itself; other resource paths are appended to it."
crates/foreign-chain-inspector/src/aptos/inspector.rs:144 — // Split timeout from rest of http errors for reporting. paraphrases the guard on the very next line (if error.is_timeout() => Timeout) without adding a why; per docs/engineering-standards.md §Write helpful code comments this is the pattern to strip. The neighbouring comments (// A body that will not decode is not transient., the 404 one) do carry information and should stay.
crates/foreign-chain-inspector/src/aptos/inspector.rs:874 — classified() is only exercised with ApiError; MalformedBody has no unit coverage on the ledger-info path. That is the realistic "endpoint serves something, just not the Aptos API" case (an nginx default page, or an HTML landing page on 200) that the new AbsenceMeaning split is meant to disambiguate from a 404, and it lands on ProviderStatus::MalformedResponse rather than RequestRejected. A Result<LedgerInfoResponse, _> built from a serde_json error would pin it cheaply; extract__should_reject_a_response_that_does_not_carry_the_resource only covers the transaction path.
crates/foreign-chain-health-check/src/probe.rs:907 — mock_ledger_info matches any GET with no path constraint, and both callers discard the returned Mock, so neither aptos probe test pins that the probe hits the REST root, nor that exactly one request was made. tests/aptos_inspector.rs:283 does pin path("/v1"), so coverage exists overall; adding mock.assert_async().await here (as the starknet tests do at probe.rs:485) would make the helper's return value earn its lifetime parameter.

Nothing else stood out: the config templates already ship expected_network_fingerprint = "1" for aptos (docs/localnet/mpc-config.template.toml:97), so no provider flips to MissingExpectedFingerprint; the fingerprint probe is not wired into node startup yet, so the probe arm itself has no production blast radius; both AptosRpcClient implementors are updated; and the intra-doc links on the new pub(crate) items resolve under cargo make check-docs' --document-private-items.

✅ Approved

pbeza · 2026-08-14T09:28:29Z

+
+    #[rstest]
+    #[case::mainnet("1", "1")]
+    #[case::padded("0002", "2")]


Optional nit: if we treat 0002 as 2, which means testnet, then perhaps it should be:

Suggested change

#[case::padded("0002", "2")]

#[case::testnet("0002", "2")]

or

Suggested change

#[case::padded("0002", "2")]

#[case::padded_testnet("0002", "2")]

pbeza · 2026-08-14T09:48:19Z

+                    ForeignChainInspectionError::RpcRequestRejected(message)
+                }
+            },
+            // Rate limits and server errors are provider hiccups → transient, so the


Nit: It was a bit confusing to me to see the comment explaining that this error is transient (until I consulted Claude), because the transient/non-transient split isn't decided in this file at all. The mapping in classified() only picks a ForeignChainInspectionError variant, while each variant's transientness is defined centrally in is_transient() in lib.rs. Same goes for:

mpc/crates/foreign-chain-inspector/src/aptos/inspector.rs

Line 148 in 3061134

// A body that will not decode is not transient.

pbeza · 2026-08-14T09:50:23Z

+            .expect("network_fingerprint should succeed");
+
+        // Then
+        assert_eq!(fingerprint.to_string(), "2");


We could reuse TESTNET_CHAIN_ID here.

pbeza · 2026-08-14T10:12:59Z

+            // Rate limits and server errors are provider hiccups → transient, so the
+            // affected provider is dropped from the quorum instead of blocking it.
+            AptosRpcError::ApiError {
+                status: 408 | 429, ..


FWIW, by Claude:

These two arms re-encode the policy that already exists as is_retryable_status in the crate root (408 | 429, or >= 500), which is reachable from this module. Collapsing them into one AptosRpcError::ApiError { status, .. } if is_retryable_status(status) arm keeps the two classifiers from drifting when one of them learns a new status.

kevindeforth

Thank you. Left a few comments.
I think some of the code-comments and tests can be improved for readability and clarity.

kevindeforth · 2026-08-17T07:48:58Z

+/// Aptos mainnet's ledger chain id, as shipped in the node config file
+/// `foreign_chains.aptos.expected_network_fingerprint`.
+const EXPECTED_NETWORK_FINGERPRINT: &str = "1";


How does this comment guarantee it won't get stale? Why is it relevant that this is set in the config? If so, do we have a DEFAULT_* parameter somewhere we could use from prod, instead of hard-coding this in the test?

No hard guarantee, but Aptos mainnet's chain id is 1 and is not supposed to ever change, so I don't expect this to get stale. "As shipped in the node config file" refers to the default config file we ship with values already filled in. The operators can freely change this value. They can for instance change this to 2 to target Aptos testnet, for a testnet deployment.

There is deliberately no DEFAULT_* in code. We wanted a clean separation between config and code, the node just runs with whatever the config file says. By that logic the test should read the config file as well, but that felt like overkill for a sanity check. Since the chain id is fixed, I think hard coding it here is fine.

We wanted a clean separation between config and code, the node just runs with whatever the config file says

Thanks for the explanation. I feel like the comment "as shipped in the node config file is at odds with that goal, but since this is in a test-file, this is not a blocker.

kevindeforth · 2026-08-17T07:51:25Z

+const EXPECTED_NETWORK_FINGERPRINT: &str = "1";
+
+#[tokio::test]
+#[ignore = "manual test to sanity check against live Aptos RPC provider"]


This test might be more useful if we could activate it with a flag, together with all the other live-RPC tests.
As it stands, I don't know how we will remember / know that it exists and periodically run it.

The flag already exists as the crate convention: every *_rpc_manual.rs test is #[ignore = "manual …"], and they all run together with cargo nextest run -p foreign-chain-inspector --run-ignored only.

Running them periodically is a fair point, but idealy I would like to retire these tests completely once the probing logic is landed and picked up in the Foreign chain config tester CLI. The periodic run should use that CLI tool to exercise the same code as used in the Tx validation path, instead of these half baked tests.

My understanding is that these are more of a "trust me bro, I did test the request code against the real provider for my change" kind of test atm.

idealy I would like to retire these tests completely once the probing logic is landed and picked up in the Foreign chain config tester CLI.

that does sound nice. Good plan 👍

The chain id lives in the ledger info at the REST root, so `AptosRpcClient` gains a call for it. The status mapping `extract` already had is shared, except for a 404, which on the root means the URL serves no Aptos API rather than a missing transaction.

`probe_chain` hands it to the inspector factory, so a chain whose client carries its own deadline cannot drift from the one the probe enforces.

Threading the deadline through the factory keeps client construction inside the probe, which is the thing to move. Leaves a TODO(#4043) where it belongs.

A 404 reads differently per endpoint, so the response type carries the verdict as an associated const and the call site passes nothing.

…esource reqwest reports both as a decode error, so the transport step and the decode step now fail with their own types: a truncated or timed out body stays transient, while a body that is not the resource is a verdict about the endpoint. Also folds the status table into `classified`, so the absence meaning is only ever read from the response type.

`ClassifyRpcOutcome::Response` now requires `HasAbsenceMeaning`, so a transport cannot classify a response type that never declared what a "not found" answer means for it. Also restores the `#[from]` conversions on `AptosRpcError` and sweeps the comments this stack added.

Name the config field the manual test's fingerprint mirrors, and drop the comments that restate the code they sit on.

…hain id docs

…entness comments

…de defaults

pbeza · 2026-08-19T16:05:16Z

Sorry, I committed to re-reviewing it today, but I won’t make it. There were more review comments and follow-up commits than I expected. I’ll try to get to it tomorrow. Sorry about that. 🙏🏼

pbeza

I skimmed through it and it LGTM. I didn’t do a deep re-review because, as discussed offline, it looks like you force-pushed some changes after the reviews, which makes it much harder to tell what changed since then. Not sure if that was caused by the automatic restack or something else.

Let’s try to avoid force-pushing once reviews are in when possible, since the diff against the commit I reviewed is pretty noisy, presumably because of the force-push: https://github.com/near/mpc/compare/30611349572c5c843cdf7443237be3a30f3746fa..a4e826bc0fd99eb930007a499f56e0050c14177f.

kevindeforth

Thank you, this looks good!

haiyuechen-nearone mentioned this pull request Aug 5, 2026

feat: probe Sui providers by genesis checkpoint digest #4070

Merged

haiyuechen-nearone changed the title ~~feat(probe): probe Aptos for its ledger chain id~~ feat: probe Aptos for its ledger chain id Aug 5, 2026

haiyuechen-nearone mentioned this pull request Aug 7, 2026

feat: probe Bitcoin provider by genesis block hash #4067

Merged

haiyuechen-nearone changed the title ~~feat: probe Aptos for its ledger chain id~~ feat(probe): identify Aptos by its ledger chain id Aug 7, 2026

This was referenced Aug 7, 2026

feat: periodically report foreign chain RPC provider health #4071

Open

feat: Starknet RPC healthcheck probing logic #4013

Merged

feat: EVM chains RPC probing logic #4049

Merged

haiyuechen-nearone force-pushed the 4003-probe-aptos-chain-id branch from 67d79d3 to 05e2c85 Compare August 7, 2026 14:35

haiyuechen-nearone force-pushed the 4003-probe-aptos-chain-id branch from 301c4ea to 672d398 Compare August 7, 2026 17:47

haiyuechen-nearone requested review from gilcu3 and removed request for gilcu3 August 7, 2026 19:42

haiyuechen-nearone self-assigned this Aug 7, 2026

haiyuechen-nearone marked this pull request as ready for review August 7, 2026 19:43

haiyuechen-nearone force-pushed the 4003-probe-aptos-chain-id branch from fbf1c4e to 1ef3e16 Compare August 7, 2026 19:43

haiyuechen-nearone changed the title ~~feat(probe): identify Aptos by its ledger chain id~~ feat: Probe Aptos by ledger chain id Aug 7, 2026

haiyuechen-nearone changed the title ~~feat: Probe Aptos by ledger chain id~~ feat: probe Aptos providers by ledger chain id Aug 7, 2026

haiyuechen-nearone force-pushed the 4003-probe-aptos-chain-id branch from 1ef3e16 to d5ee2e1 Compare August 11, 2026 11:13

haiyuechen-nearone force-pushed the 4003-probe-aptos-chain-id branch from d5ee2e1 to a8946bf Compare August 11, 2026 16:23

haiyuechen-nearone force-pushed the 4003-probe-aptos-chain-id branch from a8946bf to f6eca1f Compare August 13, 2026 19:45

Base automatically changed from 4003-probe-bitcoin-genesis-hash to main August 14, 2026 07:23

haiyuechen-nearone force-pushed the 4003-probe-aptos-chain-id branch from 0718161 to 3061134 Compare August 14, 2026 07:23

pbeza previously approved these changes Aug 14, 2026

View reviewed changes

pbeza reviewed Aug 14, 2026

View reviewed changes

kevindeforth reviewed Aug 17, 2026

View reviewed changes

haiyuechen-nearone added 11 commits August 17, 2026 14:45

fix(probe): report a slow Aptos provider as timed out

c43b7fc

refactor(probe): derive the attempt deadline once

ebadc32

`probe_chain` hands it to the inspector factory, so a chain whose client carries its own deadline cannot drift from the one the probe enforces.

docs(probe): state the 404 contract once, on the classifier

69b0c0c

Revert "refactor(probe): derive the attempt deadline once"

58d1607

Threading the deadline through the factory keeps client construction inside the probe, which is the thing to move. Leaves a TODO(#4043) where it belongs.

docs(probe): move the DI todo to where the clients are chosen

1704706

docs(probe): shorten the DI todo and fold it into the doc comment

6b7e6a1

refactor(probe): let the resource say what a not found answer means

03df8c5

A 404 reads differently per endpoint, so the response type carries the verdict as an associated const and the call site passes nothing.

docs(probe): cut the paraphrasing Aptos comments

46ae5c4

Name the config field the manual test's fingerprint mirrors, and drop the comments that restate the code they sit on.

haiyuechen-nearone force-pushed the 4003-probe-aptos-chain-id branch from 3061134 to 46ae5c4 Compare August 17, 2026 12:47

test(probe): name the Aptos test literals and drop decimal from the c…

82ad688

…hain id docs

haiyuechen-nearone dismissed pbeza’s stale review via 82ad688 August 18, 2026 15:23

haiyuechen-nearone added 2 commits August 18, 2026 17:52

refactor(probe): reuse the retryable status policy and cut the transi…

810701e

…entness comments

refactor(probe): inline the Aptos error body and drop the test of ser…

a4e826b

…de defaults

haiyuechen-nearone requested review from kevindeforth and pbeza August 19, 2026 07:46

pbeza approved these changes Aug 20, 2026

View reviewed changes

kevindeforth approved these changes Aug 20, 2026

View reviewed changes

haiyuechen-nearone added this pull request to the merge queue Aug 20, 2026

Merged via the queue into main with commit c6f8c21 Aug 20, 2026
15 checks passed

haiyuechen-nearone deleted the 4003-probe-aptos-chain-id branch August 20, 2026 11:18

	#[case::padded("0002", "2")]
	#[case::padded_testnet("0002", "2")]

Uh oh!

Conversation

haiyuechen-nearone commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes for review

Uh oh!

claude Bot commented Aug 7, 2026

Pull request overview

Reviewed changes

Findings

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbeza Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevindeforth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haiyuechen-nearone Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pbeza commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbeza left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevindeforth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

haiyuechen-nearone commented Aug 5, 2026 •

edited

Loading

pbeza Aug 14, 2026 •

edited

Loading

kevindeforth left a comment •

edited

Loading

haiyuechen-nearone Aug 18, 2026 •

edited

Loading

pbeza commented Aug 19, 2026 •

edited

Loading

pbeza left a comment •

edited

Loading