feat: probe Sui providers by genesis checkpoint digest - #4070
Conversation
8141a21 to
052f530
Compare
052f530 to
fa6f569
Compare
5aa7e47 to
5ae8dd8
Compare
Pull request overviewWires Sui into the network-fingerprint probe by implementing The probe path is still library-only ( Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
|
f2f2d16 to
90a2d58
Compare
|
@claude review |
Pull request overviewThird step of the #4003 probe stack: The blocking documentation inaccuracy from the previous round is fixed: the prose and the code comment now name only
Changes:
Reviewed changesPer-file summary
FindingsNon-blocking (nits, follow-ups, suggestions):
✅ Approved |
f3ae667 to
39b7ec2
Compare
3a4ecd9 to
888d013
Compare
888d013 to
1258f4e
Compare
`GetServiceInfo` reports the digest as base58, which is the form it is published and configured in, so nothing is normalized. Completes the probe for every chain that has an inspector.
Serve a fake `LedgerService` over gRPC so the probe tests cover Sui end to end: on its genesis digest, on another network, unreachable, and stalled. The mock HTTP server the other chains use cannot answer a gRPC call. Read the Sui `NotFound` meaning off the response type through `ClassifyRpcOutcome`, as Aptos already does, rather than overriding it at the `network_fingerprint` call site, so a further call site cannot silently inherit `TransactionNotFound`. Derive the attempt deadline once through `timeout_of`, and move the client deadline test beside `prepare_sui`.
Every caller bounds the call itself: `network_fingerprints` arms a deadline before tonic stamps the client's, and the signing flow wraps `extract` in `FOREIGN_CHAIN_INSPECTION_TIMEOUT`. The client's `DeadlineExceeded` never surfaces, so the test pinned a tonic detail no caller observes. A provider that answers `DEADLINE_EXCEEDED` itself does reach the mapping, and `classified__should_name_a_deadline_as_a_timeout` covers that without a server. The fake ledger loses the delay it only needed in order to stall.
The fake ledger now serves whatever answer a test arms, so one server reaches every verdict: on chain, on another network, service info without a chain id, a refused service, and a deadline. Each case enters through `probe_all_providers`, as the other chains' cases do, which also pins the dispatch arm rather than leaving that to the closed port case alone.
A mock that answered every RPC with a plausible status let a test that called the wrong one fail on its assertion instead of on the wrong call. Also drops Ton from the chains said to have no inspector: it has no config section, so it never reaches the probe.
Move the NotFound rationale down to the arm it explains, and drop the comments that restate the code or the fixture they sit on.
1258f4e to
1f40b8c
Compare
|
@claude review |
Pull request overviewThird step of the #4003 probe stack, and the one that completes it: I re-checked the claims in the PR body against the tree. Changes:
Reviewed changesPer-file summary
FindingsNon-blocking (nits, follow-ups, suggestions):
✅ Approved |
| #[tokio::test] | ||
| async fn probe_all_providers__should_report_sui_on_its_genesis_digest_as_healthy() { | ||
| // Given | ||
| let server = sui_on_chain(SUI_MAINNET).await; | ||
| let config = sui_only(chain_config( | ||
| Some(SUI_MAINNET), | ||
| one_provider(PROVIDER_NAME, &server.url), | ||
| )); | ||
|
|
||
| // When | ||
| let report = probe_all_providers(&config).await; | ||
|
|
||
| // Then | ||
| assert_eq!( | ||
| must_status_of(&report, ForeignChain::Sui, PROVIDER_NAME), | ||
| ProviderStatus::Healthy | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn probe_all_providers__should_report_sui_on_another_network_as_wrong_network() { | ||
| // Given | ||
| let server = sui_on_chain(SUI_TESTNET).await; | ||
| let config = sui_only(chain_config( | ||
| Some(SUI_MAINNET), | ||
| one_provider(PROVIDER_NAME, &server.url), | ||
| )); | ||
|
|
||
| // When | ||
| let report = probe_all_providers(&config).await; | ||
|
|
||
| // Then | ||
| assert_eq!( | ||
| must_status_of(&report, ForeignChain::Sui, PROVIDER_NAME), | ||
| ProviderStatus::WrongNetwork { | ||
| expected: NetworkFingerprint::new(SUI_MAINNET), | ||
| observed: NetworkFingerprint::new(SUI_TESTNET), | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn probe_all_providers__should_report_sui_service_info_without_a_chain_id_as_malformed() { | ||
| // Given | ||
| let server = sui_answering(Ok(GetServiceInfoResponse::default())).await; | ||
| let config = sui_only(chain_config( | ||
| Some(SUI_MAINNET), | ||
| one_provider(PROVIDER_NAME, &server.url), | ||
| )); | ||
|
|
||
| // When | ||
| let report = probe_all_providers(&config).await; | ||
|
|
||
| // Then | ||
| assert_eq!( | ||
| must_status_of(&report, ForeignChain::Sui, PROVIDER_NAME), | ||
| ProviderStatus::MalformedResponse | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn probe_all_providers__should_report_a_sui_provider_not_serving_the_api_as_rejected() { | ||
| // Given | ||
| let server = sui_answering(Err(Status::not_found("no such service"))).await; | ||
| let config = sui_only(chain_config( | ||
| Some(SUI_MAINNET), | ||
| one_provider(PROVIDER_NAME, &server.url), | ||
| )); | ||
|
|
||
| // When | ||
| let report = probe_all_providers(&config).await; | ||
|
|
||
| // Then | ||
| assert_eq!( | ||
| must_status_of(&report, ForeignChain::Sui, PROVIDER_NAME), | ||
| ProviderStatus::RequestRejected | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn probe_all_providers__should_report_a_slow_sui_provider_as_timed_out() { | ||
| // Given | ||
| let server = sui_answering(Err(Status::deadline_exceeded("too slow"))).await; | ||
| let config = sui_only(chain_config( | ||
| Some(SUI_MAINNET), | ||
| one_provider(PROVIDER_NAME, &server.url), | ||
| )); | ||
|
|
||
| // When | ||
| let report = probe_all_providers(&config).await; | ||
|
|
||
| // Then | ||
| assert_eq!( | ||
| must_status_of(&report, ForeignChain::Sui, PROVIDER_NAME), | ||
| ProviderStatus::TimedOut | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn probe_all_providers__should_report_an_unreachable_sui_provider() { | ||
| // Given | ||
| let config = sui_only(chain_config( | ||
| Some(SUI_MAINNET), | ||
| one_provider(PROVIDER_NAME, CLOSED_PORT_URL), | ||
| )); | ||
|
|
||
| // When | ||
| let report = probe_all_providers(&config).await; | ||
|
|
||
| // Then | ||
| assert_eq!( | ||
| must_status_of(&report, ForeignChain::Sui, PROVIDER_NAME), | ||
| ProviderStatus::Unreachable | ||
| ); | ||
| } |
There was a problem hiding this comment.
Some of these tests look very similar. Perhaps there’s an idiomatic way to shorten this with rstest? (Maybe not, which is fine too.)
…dict tests Define each chain's fingerprint constant once in its inspector module, collapse the five Sui verdict probes into one rstest table, declare the tonic codegen feature we relied on through unification, and clarify the Sui fingerprint normalization comment.
gilcu3
left a comment
There was a problem hiding this comment.
Thanks, that was a big dedup!
The branch was rebased onto latest main, dropping the commits that landed separately as #4070. This merge keeps the rebased tree and records the superseded history so the branch fast-forwards on the remote.
Closes #4093.
Notes for review
Nothing is normalized. The Sui genesis checkpoint digest is Base58, which is case sensitive and carries no prefix or padding.
NotFoundreads off the response type, through theHasAbsenceMeaningtraits feat: probe Aptos providers by ledger chain id #4069 added.DeadlineExceededis mapped toTimeoutinstead ofRpcRequestFailedby this PR. Both variants are transient, so retries and the signing path fan out are unchanged.Separate mock servers in tests. The mock HTTP server used by previous tests cannot serve gRPC requests.
Review feedback widened the diff beyond Sui. Every chain's network fingerprint constant is now defined once in its inspector module and referenced from golden values, probe tests, and manual tests.