test(foreign-tx): sui inspector tests + localnet docs - #3791
Conversation
|
PR title type suggestion: This PR changes both test files and documentation/configuration files, so the type prefix should probably be Suggested title: |
Pull request overviewAdds test coverage for the Sui foreign-chain inspector (unit-style integration tests + an ignored manual live-RPC test) and completes localnet plumbing so Changes:
Reviewed changesPer-file summary
FindingsNon-blocking (nits, follow-ups, suggestions):
No blocking issues — this is pure test/docs coverage, follows the project's Given/When/Then conventions and comment guidance, and the mock design lines up with how the sibling ✅ Approved |
21e4b7e to
5665050
Compare
|
PR title type suggestion: Since this PR adds source code to Suggested title: |
anodar
left a comment
anodar
left a comment
There was a problem hiding this comment.
LGTM, dropped some optional nits.
| .context("provider returned no checkpoint height")?; | ||
| // Load-balanced providers may answer consecutive calls from different backends; probing | ||
| // slightly behind the reported tip keeps the check off the backend-sync race. | ||
| let probe_height = height.saturating_sub(CHECKPOINT_PROBE_OFFSET); |
There was a problem hiding this comment.
nit: let's do checked_sub here, current height is at 9 digits, if subtraction of small offset saturates it there's something wrong.
| }; | ||
| use foreign_chain_rpc_interfaces::sui::{Status, SuiRpcClient}; | ||
|
|
||
| const CHECKPOINT_HEIGHT: u64 = 296_112_296; |
There was a problem hiding this comment.
nit: let's make this value very obvious that it has no special meaning (even though it's close to mainnet checkpoint number). Either add comment or/and change value to 123456 (or something like that).
|
|
||
| // Then | ||
| let error = result.unwrap_err().to_string(); | ||
| assert!(error.contains("chain id mismatch"), "{error}"); |
There was a problem hiding this comment.
nit: I know this is an existing patter in this file, but string comparison in errors is brittle. Consider returning custom error type and checking that instead.
| /// from the provider's latest checkpoint. | ||
| #[derive(Clone, Copy)] | ||
| pub struct SuiVector { | ||
| pub chain_id: &'static str, |
There was a problem hiding this comment.
Please add a comment that this is expected to be base58 encoding (and maybe drop from above the struct itself).
Or preferably accept it without encoding (mainnet: 35834a8a, testnet: 4c78adac) and do encoding in the code instead. That way it's easier to search for chan id to verify it's correct.
| out: &mut Vec<ProviderResult>, | ||
| ) { | ||
| let Some(vector) = vector else { | ||
| mark_skipped("sui", cfg, &no_reference_reason(network), out); |
There was a problem hiding this comment.
nit: not introduced in your PR, but I've realized we only will mark chain as skipped if there exists chain field with empty config. If it's not present at all it won't appear in output as skipped.
We should instead at the call site do something like:
if let Some(cfg) = &fc.sui && vector = Some(vector) {
...
} else {
mark_skipped
}
|
PR title type suggestion: This PR includes source code changes in |
|
PR title type suggestion: This PR modifies source code files in |
|
@claude review |
Pull request overviewFollow-up to Changes:
Reviewed changesPer-file summary
FindingsNon-blocking (nits, follow-ups, suggestions):
No blocking issues. Substantive nits from the initial review round are addressed in ✅ Approved |
|
PR title type suggestion: This PR modifies source code files in Suggested title: |
|
PR title type suggestion: This PR modifies source code files under |
| // (not saturating): real checkpoint heights are 9 digits, so a height below the small | ||
| // offset means the provider returned something wrong — surface it instead of probing 0. |
There was a problem hiding this comment.
nit: this type of comments aren't very useful for future reader of the code. This was discussion in the PR but doesn't have to go in the comments.
|
PR title type suggestion: This PR includes significant source code changes across multiple crates (foreign-chain-inspector, foreign-chain-rpc-interfaces, node-config, etc.) alongside tests and documentation. The type should be Suggested title: |
|
PR title type suggestion: This PR spans multiple areas (tests, docs, source code, config templates), so the type prefix should probably be Suggested title: |
|
PR title type suggestion: This PR changes source code files (in Suggested title: |
There was a problem hiding this comment.
Pull request overview
Adds Sui support to the foreign-chain tooling and localnet workflow, so operators/devs can verify Sui foreign transactions via the same inspector path used by the node and can validate RPC configuration with foreign-chain-config-tester.
Changes:
- Extend localnet docs/templates and the
launch-localnet.shflow to include a Suiverify_foreign_transactionexample. - Add Sui inspector tests (mocked unit-style tests plus an ignored live-RPC manual test) to
foreign-chain-inspector. - Extend
foreign-chain-config-testerwith Sui support (golden vectors + Sui check implementation) and adjust reporting behavior to always include supported chains (including “not configured” rows).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/launch-localnet.sh | Adds a Sui verify_foreign_transaction call in the localnet smoke flow. |
| docs/localnet/mpc-configs/config.yaml.template | Adds Sui RPC provider config to the YAML template. |
| docs/localnet/mpc-config.template.toml | Adds Sui RPC provider config to the TOML template used by localnet. |
| docs/localnet/localnet.md | Documents the Sui verify_foreign_transaction invocation. |
| docs/localnet/args/verify_foreign_tx_sui.json | Adds Sui request args payload for localnet testing. |
| crates/foreign-chain-inspector/tests/sui_rpc_manual.rs | Adds an ignored live-RPC manual test for Sui event extraction. |
| crates/foreign-chain-inspector/tests/sui_inspector.rs | Adds comprehensive mocked tests for Sui inspector extraction and error handling. |
| crates/foreign-chain-config-tester/src/report.rs | Adjusts report rendering behavior (removes special-case empty-results message). |
| crates/foreign-chain-config-tester/src/main.rs | Adds Sui provider checking and “not configured” placeholder rows for absent chains. |
| crates/foreign-chain-config-tester/src/golden.rs | Adds Sui golden vectors and base58→[u8;32] decoding helper. |
| crates/foreign-chain-config-tester/src/checks.rs | Adds Sui provider check logic and introduces typed mismatch errors for assertions. |
| crates/foreign-chain-config-tester/README.md | Updates tool documentation/output examples to include Sui (and Aptos) behavior. |
| crates/foreign-chain-config-tester/Cargo.toml | Adds bs58 dependency for Sui digest handling. |
| Cargo.lock | Records the new dependency resolution for bs58. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR title type suggestion: This PR includes documentation and source code changes beyond just test files. Since it adds new testing capability along with documentation and implementation, Suggested title: |
No description provided.