test(integration): replay mainnet events and reconstruct state - #229
test(integration): replay mainnet events and reconstruct state#229gregorydemay wants to merge 8 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add public, memory-generic entry iterators to TokenBalance, OrderHistory, TradeHistory, and UserRegistry (and the shared History core) so external callers can read persisted collection entries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the snapshot download/extract/verify helpers and the URL, SHA-256, and mainnet-id constants into the shared crate so both snapshot tests reuse them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an integration test that replays the mainnet snapshot's event log and asserts the reconstructed persisted collections and order-book depth match the captured mainnet state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds an integration-test companion that replays the pinned mainnet event log with current code and verifies both persisted collections and reconstructed order books match the pinned mainnet snapshot, strengthening upgrade/back-compat regression coverage.
Changes:
- Add
replay_mainnet.rsintegration test that replays snapshot events and asserts reconstructed state matches snapshot + PocketIC order-book depth. - Hoist snapshot download/extract/verify helpers + constants into the shared
integration_testscrate for reuse across integration tests. - Expose stable-collection iterators in the canister library to enable deterministic snapshot-vs-replay comparisons.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| integration_tests/tests/replay_mainnet.rs | New integration test that replays snapshot events and cross-checks reconstructed state and order books. |
| integration_tests/tests/mainnet_snapshot.rs | Switches to shared snapshot helpers/constants from oisy_trade_int_tests. |
| integration_tests/src/lib.rs | Adds reusable snapshot download/extract/verify helpers and related constants. |
| integration_tests/Cargo.toml | Adds dev-deps needed for replay test (ic-stable-structures, oisy_trade_canister). |
| docs/src/development/specs/DEFI-2942-replay-mainnet-events.md | Spec documenting requirements and implementation approach for the replay test. |
| Cargo.lock | Locks new dev-dependency edges for integration tests. |
| canister/src/user/mod.rs | Makes registry iterators public for snapshot comparisons (needs fix for key extraction). |
| canister/src/order/trades/mod.rs | Adds public iterators over trades primary store and per-user index. |
| canister/src/order/history/mod.rs | Adds public iterators over orders primary store and per-user index. |
| canister/src/history/mod.rs | Adds reusable iterator helpers (iter_primary, iter_by_user) on shared History core. |
| canister/src/balance/token.rs | Makes balance iterator public for snapshot comparisons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🧐 VERDICT: CHANGES_REQUESTED — 0 blockers, 2 mediums, 1 nit; CI pending (hard block on READY). Review detailsCI: Requirements
Maintainability
Test-pyramid / coverage notes
Verdict rationale: two 🟠 (vacuous-R3 guard, PocketIC-load duplication) plus pending CI. Not ready. |
|
Return the domain newtype InsertionSeq from the per-user index iterators instead of a bare u64. Addresses review comment 3627740145. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract into a per-call unique directory and stage the archive download in a verified temp file that is atomically renamed into place, so concurrent tests sharing the cached archive never race. Addresses review comment 3627736995. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t tests Hoist the create-at-mainnet-id + upload + load flow into a shared load_snapshot_into_pocketic helper used by both snapshot tests, move the replay test into mainnet_snapshot.rs alongside the upgrade test, and guard the order-book cross-check against a vacuous pass when no trading pairs are reconstructed. Addresses review comments 3627739346 and 3627739251. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gregorydemay
left a comment
There was a problem hiding this comment.
🤖 🧐 VERDICT: READY — 0 blockers, 0 mediums, 1 nit; CI green. Re-review of the three first-round findings: all resolved.
Review details
First-round findings — all resolved
- R3 vacuous-pass guard ✅ —
mainnet_snapshot.rs:167-170now asserts!state.trading_pairs().is_empty()before the order-book cross-check loop, so a replay regression that drops every pair fails instead of passing vacuously. - Duplication / shared load helper ✅ — the create-at-mainnet-id → add_cycles → upload → stop → load → start sequence is hoisted into
load_snapshot_into_pocketicinintegration_tests/src/lib.rs:657and called by BOTH tests (mainnet_snapshot.rs:56and:172). No near-verbatim copy remains. iter_by_usertyping ✅ — returnsInsertionSeq(Seq<InsertionSeqMarker>) end to end throughHistory,OrderHistory, andTradeHistory; re-exported via theordermodule so the public signature is nameable.
Sanity checks
- Merged single file: the upgrade test
should_load_mainnet_snapshot_and_upgrade_to_current_wasmis intact, now sharing the same load helper; no regression.replay_mainnet.rsdeleted, both tests inmainnet_snapshot.rs. - Concurrency-safe download: sound. Archive is fetched to a per-call unique temp file, SHA-256-verified, then atomically renamed into the shared cached path; extraction goes to a per-call unique dir. Readers only ever see a fully-verified archive, and
unique_suffix()(pid + atomic counter) prevents path collisions across parallel tests. - Test-only-in-production concern from the prior shape is gone:
UserRegistry::iter_*andTokenBalance::iterare now genuine public accessors (with docs) exercised by the tests, not#[cfg(test)]shims.
Maintainability accounting
- Duplication: none remaining (shared helper resolves the R3-load copy; the two tests now diverge only in their assertions).
- Unused derives: none (no new types;
InsertionSeqMarkerre-export is load-bearing for the publicInsertionSeqalias). - Primitive-obsession parameters: cleared (the
u64insertion-sequence slip is nowInsertionSeq). - Divergent invariant handling: none found.
- Silent fallbacks: none masking success — the
let _ = remove_*calls are best-effort cleanup on paths that still surface the primary failure viapanic!.
Coverage (R1–R4)
- R1 replay-without-panic, R2 entry-by-entry collection equality (length + per-entry via
assert_entries_eq), R3 per-pair order-book depth vs PocketIC (now guarded non-empty), R4 SHA-256 pin + reject-on-mismatch — all exercised.
Nit (non-blocking)
- 🔵 The PR description still reads
Spec: docs/src/development/specs/DEFI-2942-replay-mainnet-events.md, but that spec was intentionally removed — the reference now points at a nonexistent file. Drop that line from the description.
Replay directly from the StableLog iterator instead of collecting the whole log into a Vec, and remove the per-call extraction directory once the snapshot is uploaded to PocketIC so temp dirs do not accumulate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🤖 This PR is ready for your review.
Left as a draft — marking ready, approval, and merge are yours. |
Adds the replay companion to the merged mainnet upgrade test: it takes the mainnet event log and proves the current code reconstructs exactly the state captured in the mainnet snapshot, guarding against silent decode/replay or CBOR/Candid back-compat regressions on upgrade.