test(ledger-suite-orchestrator): drop the StateMachine test fixture - #10956
Conversation
Nothing in rs/ethereum consumes the old StateMachine-based fixture (lib.rs's LedgerSuiteOrchestrator, flow.rs, universal_canister.rs) now that both LSO and cketh integration tests run on PocketIC. Delete it and the now-unused ic-state-machine-tests, ic-types, and ic-management-canister-types-private dependencies, keeping only the environment-agnostic helpers (wasm loaders, init-arg builders, supported_erc20_tokens/usdc/usdt, constants) that the pocket_ic module and cketh still import at their existing paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Removes the legacy ic-state-machine-tests-based test fixture from rs/ethereum/ledger-suite-orchestrator/test_utils, leaving the existing PocketIC fixture as the only supported test harness and cleaning up now-unused dependencies.
Changes:
- Deleted the StateMachine-based
flowanduniversal_canisterfixtures and removed the StateMachineLedgerSuiteOrchestratorhelper implementation fromtest_utils/src/lib.rs. - Kept the environment-agnostic helpers (wasm loaders, init-arg builders, token helpers/constants) at their existing crate-root paths.
- Dropped unused dependencies (
ic-state-machine-tests,ic-types,ic-management-canister-types-private,assert_matches) from Cargo + Bazel, and updatedCargo.lock.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rs/ethereum/ledger-suite-orchestrator/test_utils/src/universal_canister.rs | Deleted the StateMachine-based universal canister helper (PocketIC version remains under pocket_ic/). |
| rs/ethereum/ledger-suite-orchestrator/test_utils/src/lib.rs | Removed StateMachine fixture exports/impls; retained shared helper APIs/constants used by PocketIC + ckETH. |
| rs/ethereum/ledger-suite-orchestrator/test_utils/src/flow.rs | Deleted the StateMachine-based flow/assertion helpers (PocketIC equivalents remain under pocket_ic/). |
| rs/ethereum/ledger-suite-orchestrator/test_utils/Cargo.toml | Removed unused StateMachine-era dependencies. |
| rs/ethereum/ledger-suite-orchestrator/test_utils/BUILD.bazel | Removed unused Bazel deps corresponding to the deleted fixture. |
| Cargo.lock | Reflected the dependency removals for ic-ledger-suite-orchestrator-test-utils. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🤖🧐 VERDICT: CHANGES_REQUESTED — 0 blockers, 0 mediums, 3 nits; CI pending ( Review detailsVerdict rationaleNothing in the diff needs to change. The three questions a deletion PR has to answer1. Was anything still needed deleted? No. Conversely, every deleted symbol is gone repo-wide: 2. Was anything dead kept? Three pre-existing dead deps — see the inline nit on 3. The sweep claims — one holds as stated, one needs restating. Grep sweep: holds. Bazel sweep: the substantive criterion is met, but the plan's literal wording is unachievable. Maintainability rundown
TestingPure deletion of unreferenced code, so no behaviour changes and no new tests are warranted; the guarantee needed is that the surviving suites are unaffected, which they are. What I ran
|
…-drop-state-machine
…-drop-state-machine
|
✅ No security or compliance issues detected. Reviewed everything up to b509d80. Security Overview
Detected Code Changes
|
mbjorkqvist
left a comment
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks for the cleanup @gregorydemay!
Since you're removing dependencies anyway, I believe the following could also be removed (they were already unused before this PR though):
ic-http-types(in bothCargo.tomlandBUILD.bazel)proptest(also in both)ic-test-utilities-load-wasminCargo.tomlonly
dfinity#10949) Migrates the ledger-suite-orchestrator (LSO) integration tests from the internal `ic-state-machine-tests` harness to PocketIC, following the pattern already used for the EVM-RPC canister (dfinity/evm-rpc-canister#294). This is DEFI-2262: a pure refactor of the test harness — same tests, same coverage, same semantics. `CkErc20Setup` in `rs/ethereum/cketh` embeds LSO's fixture and shares a `StateMachine` with it, so flipping LSO's fixture in place would break cketh in the same commit. To keep this change single-component and the tree green, this PR **adds** a PocketIC fixture in LSO's `test_utils` alongside the existing StateMachine one (precedent: `rs/ledger_suite/icp/test_utils`'s `state_machine_helpers`/`pocket_ic_helpers` coexisting) and migrates only `ledger-suite-orchestrator/tests/tests.rs` to it. The StateMachine fixture stays untouched at its current paths and is still exercised by `cketh`; it is deleted only in the last PR of this stack, once cketh no longer needs it. ## Summary of changes - New `pocket_ic` module in `ledger-suite-orchestrator/test_utils` (`LedgerSuiteOrchestrator`, `AddErc20TokenFlow`, `ManagedCanistersAssert`, `UniversalCanister`, `new_pocket_ic()`), mirroring the existing StateMachine-based types and method names one-for-one so later PRs in the stack only need to swap import paths. - `new_pocket_ic()` builds a system subnet (LSO tests assert exact cycle balances, which only hold on a non-charging subnet) with canister-execution rate limiting disabled (LSO repeatedly installs/upgrades ledger, index and archive canisters). - Metrics assertions now go through `ic-metrics-assert`'s `PocketIcHttpQuery` (feature `pocket_ic`) instead of a hand-rolled `CanisterHttpQuery` impl. - `tests/tests.rs` fully migrated to the new fixture. - `Cargo.toml`/`BUILD.bazel` updated accordingly; `test_utils` keeps `ic-state-machine-tests` for now (still needed by the old fixture). ## Coverage - All spec requirements from the PR are covered: PocketIC fixture added alongside the old one, `tests/tests.rs` fully migrated, old fixture/paths untouched, `cketh` still builds against the untouched fixture. ## 📚 PR stack 1. **dfinity#10949 — test(ledger-suite-orchestrator): migrate integration tests to PocketIC** 👈 you are here 2. dfinity#10950 — test(cketh): deduplicate signed-transaction literals in integration tests 3. dfinity#10955 — test(cketh): migrate integration tests to PocketIC 4. dfinity#10956 — test(ledger-suite-orchestrator): drop the StateMachine test fixture 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…-drop-state-machine
…-drop-state-machine
b509d80
into
ic_DEFI-2262_3_cketh-pocket-ic
…rals in integration tests (dfinity#10950) ## Summary Small preparatory cleanup for the cketh integration tests ahead of the PocketIC migration (DEFI-2262). No framework change here — tests still run on `ic-state-machine-tests` and stay green. - Deduplicated the inline raw-transaction and transaction-hash string literals in `minter/tests/cketh.rs` that were byte-identical to the `DEFAULT_WITHDRAWAL_TRANSACTION` / `DEFAULT_WITHDRAWAL_TRANSACTION_HASH` constants already exported by `cketh/test_utils`. Since PR 3 regenerates these signatures for the PocketIC fixture, keeping every occurrence tied to the single constant means the regeneration only needs to touch one place instead of risking a missed or inconsistent literal. ## 📚 PR stack 1. dfinity#10949 — test(ledger-suite-orchestrator): migrate integration tests to PocketIC 2. **dfinity#10950 — test(cketh): deduplicate signed-transaction literals in integration tests** 👈 you are here 3. dfinity#10955 — test(cketh): migrate integration tests to PocketIC 4. dfinity#10956 — test(ledger-suite-orchestrator): drop the StateMachine test fixture 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
🤖 This PR was closed as merged by mistake — it was not reviewed or intentionally merged. What happened: while fixing an unrelated review comment on #10955, I committed on this branch instead of #10955's and pushed the result to Where the content lives now: in #10955, unchanged. Since the merge cannot be undone, the ledger-suite-orchestrator fixture deletion stays where it landed rather than being moved. #10955's description has been updated to cover it, so the change is still described and reviewable — just under a different PR number than planned. The replacement PR I had opened (#11122) is closed as unnecessary, and the unrelated PocketIC server lifetime fix that caused this has been moved out of #10955 into its own PR, #11123. Apologies for the noise — the mistake was mine, not a change in plan. |
## 📚 PR stack 1. ~~dfinity#10949~~ — test(ledger-suite-orchestrator): migrate integration tests to PocketIC (merged) 2. ~~dfinity#10950~~ — test(cketh): deduplicate signed-transaction and transaction-hash literals (merged) 3. **dfinity#10955 — test(cketh): migrate integration tests to PocketIC** 👈 you are here 4. ~~dfinity#10956~~ — test(ledger-suite-orchestrator): drop the StateMachine test fixture — **its content is part of this PR**, see below Follow-up, based on this PR: dfinity#11123 (PocketIC server lifetime). ## Summary Flips `rs/ethereum/cketh/test_utils` and the minter's `cketh.rs`/`ckerc20.rs` integration tests from `ic-state-machine-tests` onto the sync `pocket_ic` client, following the pattern established for the ledger-suite-orchestrator in dfinity#10949. The minter is now initialized against the fiduciary subnet's `"key_1"` ECDSA key (StateMachine's `"master_ecdsa_public_key"` isn't available on PocketIC's fiduciary subnet), which changes the minter's derived Ethereum address. Every signature-derived constant is regenerated as a consequence: `MINTER_ADDRESS`, `DEFAULT_WITHDRAWAL_TRANSACTION(_HASH)`, `DEFAULT_CKERC20_WITHDRAWAL_TRANSACTION(_HASH)`, the r/s of the default ERC20 signed transaction, the per-account deposit address in `should_record_address_to_deposit`, and the resubmitted-transaction literal in `should_resubmit_new_transaction_with_same_max_fee_per_gas_when_price_increased`. Canister-http mocking is rebuilt around PocketIC's `get_canister_http()` / `mock_canister_http_response()`, preserving the legacy `"Http body exceeds size limit of {n} bytes."` reject that the minter's response-size check matches on. `tests/dump_stable_memory.rs` and `tests/deposit_from_cex_demo.rs` are untouched. ## Also included: dropping the StateMachine fixture from the orchestrator This was meant to be a separate PR (dfinity#10956) reviewed on top of this one. It was marked merged by mistake — see [the note there](dfinity#10956 (comment)) — and since GitHub cannot reopen a merged PR, its content stays here rather than being shuffled again. With the ckETH minter's tests on PocketIC, nothing in `rs/ethereum` uses the orchestrator's StateMachine-based fixture, so it is deleted along with the now-unused `ic-state-machine-tests`, `ic-types`, `ic-management-canister-types-private` and `assert_matches` dependencies. The environment-agnostic helpers the `pocket_ic` module and cketh still import (wasm loaders, init-arg builders, `usdc()`/`usdt()`, constants) stay at their existing paths. After this PR, no Rust target under `rs/ethereum` compiles against `ic-state-machine-tests`. ## Behavioural differences PocketIC forces, and how they are handled PocketIC differs from StateMachine in two ways the tests are sensitive to. Both are handled in the fixture, in one place each, rather than worked around per test. **Time advances kill the outcalls in flight.** PocketIC fails canister-http requests once they cross `CANISTER_HTTP_TIMEOUT_INTERVAL`; StateMachine left them pending indefinitely. The requests also stay listed until a round processes the failures, so a stub could bind to a request that was already dead and see its response discarded. Worse, a scraping cycle awaiting such a request keeps holding its `TimerGuard`, so the firing due at the new time is dropped as `AlreadyProcessing` and the query the test expects is never issued. `CkEthSetup::advance_time` therefore fails those outcalls up front and deterministically, doing what the advance would have done anyway. It is a no-op when nothing is in flight, because a fixture that has executed no round is expected to trigger the first scraping cycle itself. **A stop request takes effect only in the following round.** The outcalls the minter's timers issue in that last running round become visible after it, so `stop_minter` ticks once before draining them. The block number stubs also now match on `["finalized", false]`, which is what the scraping cycle actually queries — unconstrained, they could be consumed by the block height refresh timer's query for the latest block. ## Runtime Measured against master with identical in-test instrumentation on both sides: **+5% (ckETH) and +11% (ckERC20)** of wall-clock at default parallelism (+15%/+20% single-threaded). Fixture-light tests are indistinguishable from master; the difference tracks the number of IC rounds a test drives, since PocketIC runs out of process. The dominant cost in both harnesses is tests that rebuild the whole fixture several times within one `#[test]`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Completes the DEFI-2262 PocketIC migration for
rs/ethereum: with the ckETH minter's integration tests now running on PocketIC, nothing inrs/ethereumstill uses the ledger-suite-orchestrator's StateMachine-based test fixture. This PR deletes it, along with the now-unusedic-state-machine-tests,ic-types,ic-management-canister-types-private, andassert_matchesdependencies, leaving PocketIC as the orchestrator's sole test harness.LedgerSuiteOrchestrator/flow/universal_canisterfixture fromledger-suite-orchestrator/test_utils; thepocket_icmodule (added in test(ledger-suite-orchestrator): migrate integration tests to PocketIC #10949) is now the crate's only fixture.pocket_icmodule and cketh still import at their existing paths (wasm loaders, init-arg builders,usdc()/usdt(), constants).ic-state-machine-tests,ic-types,ic-management-canister-types-private, andassert_matchesdependencies fromCargo.toml/BUILD.bazel.📚 PR stack
🤖 Generated with Claude Code