feat: probe Aptos providers by ledger chain id - #4069
feat: probe Aptos providers by ledger chain id#4069haiyuechen-nearone wants to merge 11 commits into
Conversation
haiyuechen-nearone
changed the title
haiyuechen-nearone
changed the title
67d79d3 to
05e2c85
Compare
301c4ea to
672d398
Compare
fbf1c4e to
1ef3e16
Compare
haiyuechen-nearone
changed the title
haiyuechen-nearone
changed the title
Pull request overviewAdds an Aptos arm to the network-fingerprint probe. Changes:
Reviewed changesPer-file summary
I did not build or run the test suite ( FindingsNon-blocking (nits, follow-ups, suggestions):
Nothing else stood out: the config templates already ship ✅ Approved |
1ef3e16 to
d5ee2e1
Compare
d5ee2e1 to
a8946bf
Compare
a8946bf to
f6eca1f
Compare
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.
0718161 to
3061134
Compare
| /// Error from the Aptos REST API client. | ||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum AptosRpcError { | ||
| #[error("HTTP request failed: {0}")] | ||
| Http(#[from] reqwest::Error), | ||
| #[error("Aptos API returned HTTP {status}: {body}")] | ||
| ApiError { status: u16, body: String }, | ||
| #[error("failed to decode the Aptos API response: {0}")] | ||
| MalformedBody(#[from] serde_json::Error), | ||
| } |
There was a problem hiding this comment.
Is this an exhaustive list of Aptos RPC errors? If not, it might be worth adding #[non_exhaustive], although I’m not 100% sure that’s the best practice here.
| async fn get_json<T: DeserializeOwned>(&self, url: Url) -> Result<T, AptosRpcError> { | ||
| let response = self.client.get(url).send().await?; | ||
| let status = response.status(); | ||
| if !status.is_success() { | ||
| let body = response.text().await.unwrap_or_default(); | ||
| return Err(AptosRpcError::ApiError { | ||
| status: status.as_u16(), | ||
| body, | ||
| }); | ||
| } | ||
|
|
||
| let body = response.bytes().await?; | ||
| Ok(serde_json::from_slice(&body)?) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this could be shortened a bit:
| async fn get_json<T: DeserializeOwned>(&self, url: Url) -> Result<T, AptosRpcError> { | |
| let response = self.client.get(url).send().await?; | |
| let status = response.status(); | |
| if !status.is_success() { | |
| let body = response.text().await.unwrap_or_default(); | |
| return Err(AptosRpcError::ApiError { | |
| status: status.as_u16(), | |
| body, | |
| }); | |
| } | |
| let body = response.bytes().await?; | |
| Ok(serde_json::from_slice(&body)?) | |
| } | |
| } | |
| async fn get_json<T: DeserializeOwned>(&self, url: Url) -> Result<T, AptosRpcError> { | |
| let response = self.client.get(url).send().await?; | |
| let status = response.status(); | |
| if !status.is_success() { | |
| return Err(AptosRpcError::ApiError { | |
| status: status.as_u16(), | |
| body: response.text().await.unwrap_or_default(), | |
| }); | |
| } | |
| Ok(response.json().await?) | |
| } |
|
|
||
| #[rstest] | ||
| #[case::mainnet("1", "1")] | ||
| #[case::padded("0002", "2")] |
There was a problem hiding this comment.
Optional nit: if we treat 0002 as 2, which means testnet, then perhaps it should be:
| #[case::padded("0002", "2")] | |
| #[case::testnet("0002", "2")] |
or
| #[case::padded("0002", "2")] | |
| #[case::padded_testnet("0002", "2")] |
| #[test] | ||
| fn deserialize_ledger_info__should_ignore_the_fields_the_probe_does_not_read() { | ||
| // Given | ||
| let json = serde_json::json!({ | ||
| "chain_id": 1, | ||
| "epoch": "13", | ||
| "ledger_version": "1234", | ||
| "node_role": "full_node", | ||
| }); | ||
|
|
||
| // When | ||
| let parsed: LedgerInfoResponse = serde_json::from_value(json).unwrap(); | ||
|
|
||
| // Then | ||
| assert_eq!(parsed.chain_id, 1); | ||
| } |
There was a problem hiding this comment.
I think this tests serde rather than our code. IIUC, serde ignores unknown JSON fields by default (unless you add #[serde(deny_unknown_fields)]).
| mock_ledger_info(&server, APTOS_TESTNET).await; | ||
| let config = ForeignChainsConfig { | ||
| aptos: Some(chain_config( | ||
| Some("1"), |
There was a problem hiding this comment.
Nit: we could reuse "1" below in the // Then section if we extract it into a variable.
| ForeignChainInspectionError::RpcRequestRejected(message) | ||
| } | ||
| }, | ||
| // Rate limits and server errors are provider hiccups → transient, so the |
There was a problem hiding this comment.
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:
| .expect("network_fingerprint should succeed"); | ||
|
|
||
| // Then | ||
| assert_eq!(fingerprint.to_string(), "2"); |
There was a problem hiding this comment.
We could reuse TESTNET_CHAIN_ID here.
| // 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, .. |
There was a problem hiding this comment.
FWIW, by Claude:
These two arms re-encode the policy that already exists as
is_retryable_statusin the crate root (408 | 429, or>= 500), which is reachable from this module. Collapsing them into oneAptosRpcError::ApiError { status, .. } if is_retryable_status(status)arm keeps the two classifiers from drifting when one of them learns a new status.
2 participants
Closes #4092.
Aptos reports its chain id in the ledger info every node serves at the REST root, so
AptosRpcClientgains aget_ledger_infocall and the inspector readschain_idfrom 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.foreign-chain-instpectorcrate root because Sui needs the same distinction for gRPCNOT_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.