[rosetta] Audit, test, and restructure aptos-rosetta

Cargo.toml

@@ -891,6 +891,11 @@ wasm-bindgen = "0.2.100"
 wasm-bindgen-futures = "0.4.50"
 which = "4.2.5"
 whoami = "1.5.0"
+# Pinned to 0.6.4: 0.6.5 raised its `futures` floor to ^0.3.31, which the pinned
+# aptos-labs/futures-rs backport fork (0.3.30) cannot satisfy. Allowing 0.6.5 pulls
+# a second, upstream futures 0.3.32 into the graph, bypassing the fork patch and
+# deprecating `UnboundedReceiver::try_next` under `-D warnings` across consensus.
+wiremock = "=0.6.4"
 # This allows for zeroize 1.6 to be used. Version 1.2.0 of x25519-dalek locks zeroize to 1.3.
 x25519-dalek = { git = "https://github.com/aptos-labs/x25519-dalek", rev = "b9cdbaf36bf2a83438d9f660e5a708c82ed60d8e" }
 z3tracer = "0.8.0"

crates/aptos-rosetta/Cargo.toml

@@ -25,6 +25,7 @@ aptos-runtimes = { workspace = true }
 aptos-sdk = { workspace = true }
 aptos-types = { workspace = true }
 aptos-warp-webserver = { workspace = true }
+async-trait = { workspace = true }
 bcs = { workspace = true }
 clap = { workspace = true }
 futures = { workspace = true }
@@ -38,3 +39,7 @@ serde_json = { workspace = true }
 tokio = { workspace = true }
 url = { workspace = true }
 warp = { workspace = true }
+
+[dev-dependencies]
+mockall = { workspace = true }
+wiremock = { workspace = true }

crates/aptos-rosetta/README.md

@@ -45,7 +45,8 @@ Here are some exceptions:
 
 All transactions are parsed from the events provided by the AptosFramework.  There are a few exceptions to this that use the transaction payload, but only for errors.
 
-Block hash is `<chain_id>:<block_height>` and not actually a hash.
+Block hash is `<chain_id>-<block_height>` and not actually a hash. See
+`docs/SPEC_DEVIATIONS.md` §2.
 
 ### Constructing transactions
 

crates/aptos-rosetta/docs/BEHAVIOR_CHANGES.md

@@ -0,0 +1,128 @@
+# aptos-rosetta — Behavior Changes (Rewrite Changelog)
+
+This file records every **intentional** behavior change made while rewriting
+`aptos-rosetta` for legibility. The rewrite policy is *"fix quirks and bugs, but
+document every change."*
+
+Rules for this file:
+
+- Wire-visible structure (routes, JSON field names, error `code`/`retriable`
+  values) stays identical **unless an entry below says otherwise**.
+- Every entry that changes wire output MUST have a corresponding updated golden
+  test committed in the same change.
+- Deviations from the Rosetta spec that are *preserved* (not changed) live in
+  [`SPEC_DEVIATIONS.md`](./SPEC_DEVIATIONS.md), not here.
+
+Status legend: **PLANNED** (agreed, not yet done) · **DONE** (implemented +
+golden updated) · **WONTFIX** (documented, left as-is).
+
+---
+
+## Error module (`src/error.rs`)
+
+### BC-1 — Fix typo'd error messages · DONE
+The Rosetta spec requires the `message` for a given error `code` to be stable.
+These messages contain typos/grammar issues. The codes are unchanged; only the
+human-readable strings change.
+
+| code | variant | before | after |
+|---|---|---|---|
+| 14 | `NodeIsOffline` | `This API is unavailable for the node because he's offline` | `This API is unavailable because the node is offline` |
+| 25 | `BlockNotFound` | `Block is missing events` | `Block not found` |
+| 33 | `CoinTypeFailedToBeFetched` | `Faileed to retrieve the coin type information, please retry` | `Failed to retrieve the coin type information, please retry` |
+
+Risk: an integrator string-matching on `message` (rather than `code`) would
+break. Rosetta explicitly ties semantics to `code`, so this is considered safe.
+
+### BC-2 — Fix `InvalidTransactionUpdate` mis-mapping · DONE
+`From<RestError>` maps `AptosErrorCode::InvalidTransactionUpdate` to
+`ApiError::InvalidInput` (code 28) instead of `ApiError::InvalidTransactionUpdate`
+(code 29) (`src/error.rs:305-307`). After the fix it maps to
+`InvalidTransactionUpdate` (code 29).
+
+Risk: wire-visible — the returned `code` changes 28 → 29 for this node error.
+This is a genuine bug fix; documented here because it is technically a wire
+change.
+
+### BC-3 — Single source of truth for error tables · DONE
+`code()`, `message()`, `details()`, `retriable()`, and `all()` are currently four
+parallel `match` blocks whose ordering disagrees (e.g. `GasEstimationFailed` is
+declared 10th but has code 16). Consolidate into one table/definition so the
+number, message, retriable flag, and details accessor for each variant live
+together. **No wire change** — the emitted `code`/`message`/`retriable` values
+stay exactly the same (verified by the error golden tests); only the internal
+structure changes.
+
+---
+
+## Block cache naming (`src/lib.rs`, `src/block.rs`)
+
+### BC-4 — Rename `BlockRetriever`/`block_cache` · DONE
+Renamed the `RosettaContext.block_cache` field and `block_cache()` accessor to
+`block_retriever`, and corrected the misleading "cache" doc comments. No caching
+was added (the retriever still hits the node every call) and there is no wire
+change; this is internal-only. Decision: rename (not add a cache) — a real cache
+can come later if profiling shows it's needed.
+
+`BlockRetriever` is described as a cache but performs no caching
+(`src/lib.rs:143`, `src/block.rs:167-172`). Rename to reflect reality (e.g.
+`BlockRetriever` stays but the `block_cache` field/accessor become
+`block_retriever`), or add a real cache. Internal-only; **no wire change**.
+Decision on "rename vs. add caching" to be recorded here when made.
+
+---
+
+## Operation types (`src/types/misc.rs`)
+
+### BC-6 — `update_commission` missing from `OperationType::all()` · DONE
+`OperationType` has 15 variants but `OperationType::all()`
+(`src/types/misc.rs:150-168`) lists only 14 — `UpdateCommission` is omitted. As a
+result `/network/options` advertises 14 operation types and never lists
+`update_commission`, even though the type is parsed and produced elsewhere. Add
+`UpdateCommission` to `all()`.
+
+Risk: wire-visible — `network/options.allow.operation_types` grows from 14 to 15
+entries. Pinned at 14 today by
+`test::handlers::network_options_matches_documented_deviations`; that assertion
+flips to 15 in the same change that fixes `all()`.
+
+---
+
+## On-chain typo (`src/types/move_types.rs`)
+
+### BC-5 — `update_commision` misspelling · WONTFIX
+`UPDATE_COMMISSION_FUNCTION = "update_commision"` matches the on-chain entry
+function name and cannot change without a framework change
+(`src/types/move_types.rs:54-56`). Left as-is; kept in `SPEC_DEVIATIONS.md §13`.
+If the on-chain name is ever corrected, handle both spellings.
+
+---
+
+## Module reorganization (`src/types/objects.rs`, `src/construction.rs`)
+
+### BC-7 — Split large modules into submodules · DONE (internal-only, no wire change)
+The two largest files were split into focused submodules behind thin re-export
+parents, so every `crate::types::*` / `crate::construction::*` path resolves
+exactly as before:
+
+- `src/types/objects.rs` (3034 lines) → `objects/{currency,operation,transaction,internal_op}.rs`.
+- `src/construction.rs` (1561 lines) → `construction/{routes,combine,derive,hash,metadata,parse,payloads,preprocess,submit,helpers}.rs`.
+
+All items moved verbatim; the only source changes were per-submodule `use` blocks
+and widening a handful of cross-submodule helpers to `pub(crate)`. **No wire
+change** — routes, JSON, and error codes are byte-identical, verified by the full
+characterization suite (41 tests) staying green. Also included: `common.rs`
+legibility doc comments citing `SPEC_DEVIATIONS.md` (no code/behavior change).
+
+Citations in `SPEC_DEVIATIONS.md` were repointed to the new submodules.
+
+---
+
+## Phase 2 outcome
+
+Beyond the entries above, Phase 2 was **behavior-preserving**. The wire-visible
+changes are exactly BC-1 (error message typos), BC-2 (`InvalidTransactionUpdate`
+code 28→29), and BC-6 (`update_commission` added to `network/options`); BC-3/4/7
+are internal-only, BC-5 is WONTFIX. No operation-ordering, fee-attribution,
+FA-resolution, construction-flow, or error-path changes were introduced by the
+rewrite.

crates/aptos-rosetta/docs/COVERAGE.md

@@ -0,0 +1,69 @@
+# aptos-rosetta — Test Coverage Baseline (Phase 1)
+
+Measured with `cargo llvm-cov -p aptos-rosetta --summary-only` after Phase 1
+(characterization suite). This is the baseline the Phase 2 rewrite must not
+regress.
+
+## How to reproduce
+
+```bash
+cargo install cargo-llvm-cov --locked   # one-time
+rustup component add llvm-tools-preview  # one-time
+cargo llvm-cov -p aptos-rosetta --summary-only
+```
+
+## Baseline (line coverage)
+
+| File | Lines cover | Notes |
+|---|---|---|
+| `network.rs` | 100% | fully characterized (routes + options/list/status) |
+| `block.rs` | ~95% | route + block-building + retriever |
+| `types/identifiers.rs` | ~80% | identifier construction/parsing |
+| `error.rs` | ~76% | full error table pinned (`test/errors.rs`) |
+| `common.rs` | ~72% | currency/block-index/BlockHash/Y2K helpers |
+| `account.rs` | ~56% | base coin balance + secondary store + stake helpers |
+| `types/misc.rs` | ~54% | op types, statuses, stake balance parsing |
+| `construction.rs` | ~29% | offline round trips + submit; **staking/delegation parse/preprocess branches deferred** |
+| `types/objects.rs` | ~26% | FA txn parsing + transfer/create-account InternalOperation; **staking/delegation txn parsing deferred** |
+| `node_client.rs` | ~35% | prod impl exercised via wiremock; several delegators only hit in e2e |
+| **overall** | **~50%** | includes non-unit-target files below |
+
+Excluded from the meaningful denominator (not unit-test targets):
+
+- `client.rs` (0%) — the outbound `RosettaClient` used by the CLI / smoke tests,
+  not the server. Exercised by `testsuite/smoke-test/src/rosetta.rs`.
+- `main.rs` (~2%) — the CLI binary; argument plumbing, exercised by `verify_tool`
+  and manual runs.
+- `types/requests.rs`, `types/move_types.rs` (0%) — pure data/serde structs,
+  exercised transitively wherever they serialize.
+
+## What is deliberately deferred (and why it's safe)
+
+The two big gaps — the **staking and delegation** paths in `construction.rs`
+(`parse_set_operator_operation`, `parse_*_delegation_*`, `fill_in_operator`,
+`simulate_transaction`/`construction_metadata` online path) and
+`types/objects.rs` (`from_transaction` for staking events, `InternalOperation`
+extract/payload for the 11 staking op types) — are:
+
+1. **Covered end-to-end** by `testsuite/smoke-test/src/rosetta.rs`
+   (`test_delegation_pool_operations`, `test_transfer`, staking helpers) against a
+   real `LocalSwarm` node.
+2. **Scheduled for unit coverage during Phase 2**, when `objects.rs` and
+   `construction.rs` are split into focused submodules — each new submodule lands
+   with its own unit tests, which is the natural point to add the
+   staking/delegation cases with the `MockNodeClient` seam now in place.
+
+The Phase 1 suite already characterizes: every endpoint at least once (offline
+via routes, online via `MockNodeClient`), the complete 36-entry error table, the
+full offline construction round trip (preprocess→payloads→parse→combine→hash) for
+APT transfer and create-account, and the real `RestNodeClient` HTTP path
+(success + error mapping) via `wiremock`.
+
+## Test inventory (`src/test/`)
+
+- `mod.rs` — FA transaction parsing (mint/transfer/fee-payer/storage-refund).
+- `errors.rs` — full error-table golden + HTTP-500 + uniqueness.
+- `handlers.rs` — offline network endpoints via routes; mock-driven online helpers.
+- `construction.rs` — offline construction round trips + rejections + derive.
+- `online.rs` — network/status, block, account/balance, submit via `MockNodeClient`.
+- `wire.rs` — real `RestNodeClient` over `wiremock` (parse + error propagation).