Skip to content

refactor: decompose combined retrieval into orthogonal components#64

Merged
suchapalaver merged 1 commit into
mainfrom
refactor/decompose-combined-calculator
May 26, 2026
Merged

refactor: decompose combined retrieval into orthogonal components#64
suchapalaver merged 1 commit into
mainfrom
refactor/decompose-combined-calculator

Conversation

@suchapalaver

Copy link
Copy Markdown
Contributor

Summary

CombinedCalculator is now a thin orchestration façade over four focused submodules. The public surface (constructors and calculate_combined_data_* methods) and the partial-failure metadata callers depend on are unchanged on the wire; this is an internal layout change that exposes smaller seams for future work. Closes #24.

What's in the diff

  • src/retrieval/transfer_log_scanner.rs (new) — owns the Transfer filter build, the LogScanner invocation, and the raw-RpcLogLogBatchEntry decode loop. The decode loop is its own pub(crate) fn so it is unit-testable without a provider.
  • src/retrieval/tx_receipt_enricher.rs (new) — owns eth_getTransactionByHash + eth_getTransactionReceipt fetching (parallel via tokio::join!), the bounded serial-retry loop, and the zkSync permissive raw-decode fallback. Carries the process_log_for_combined_data span at the same boundary the original did. Migrates the should_attempt_permissive_tx_decode predicate test.
  • src/retrieval/gas_extractor.rs (new) — pure extract_gas_and_amount free function plus the TransactionGasData helper. No I/O; new unit tests cover the missing-tx, missing-receipt, transport-error, and gas-price-override arms.
  • src/retrieval/failure.rs (new) — shared collect_error_chain, transport_error_string, build_lookup_attempt, build_lookup_failure, lookup_request_failed, log_combined_data_skip. Unit tests cover the pure helpers.
  • src/retrieval/calculator.rs — slimmed from ~1600 to ~750 lines (production code now ~280); reads as orchestration. All existing integration tests (zkSync raw fallback, tx/receipt failure→partial, fallback recovery, zero-fallback config, zkSync missing-access-list happy path) stay here and continue to gate the wired pipeline.
  • src/retrieval/mod.rs — declares the four new submodules; no new public re-exports.

Reviewers might want to start at the new calculator.rs and trace into the components.

Acceptance check (from #24)

  • calculator.rs is substantially smaller and primarily wires components together — production code dropped from ~795 to ~280 lines.
  • Transfer log decoding can be tested without fetching transactions — decode_transfer_logs in transfer_log_scanner.rs with four synthetic-log tests.
  • Tx/receipt fallback behaviour can be tested without scanning logs — the enricher exposes enrich_batch and retry_failed directly; the predicate test moved with the predicate.
  • Gas extraction can be tested without scanning logs — gas_extractor.rs has five unit tests against synthetic inputs.
  • Existing combined retrieval tests pass — all seven integration tests in calculator.rs plus the new component-level unit tests are green across --all-features, default, and --no-default-features.
  • examples/zksync_combined_probe.rs still compiles — cargo check --examples --all-features clean.
  • Error chains and metadata remain at least as informative as before — failure construction routes through the shared failure module unchanged; the pass/stage/attempts accumulation behaviour matches.

Test plan

  • cargo test --all-features — 494 lib tests + integration tests + doctests pass
  • cargo test (default features) — 491 lib tests + integration tests + doctests pass
  • cargo test --no-default-features — 491 lib tests + integration tests + doctests pass
  • cargo clippy --all-targets --all-features -- -D warnings clean
  • cargo fmt --check clean
  • cargo check --examples --all-features clean
  • reuse lint — 104/104 files compliant

`CombinedCalculator` is now a thin orchestration façade over four
focused submodules: `TransferLogScanner` decodes Transfer events out of
`eth_getLogs`, `TxReceiptEnricher` fetches transactions and receipts
(carrying the zkSync permissive raw-decode fallback and the bounded
serial-retry loop), `extract_gas_and_amount` is a pure function that
turns each (tx, receipt) pair into a `GasAndAmountForTx`, and the
shared `failure` module owns lookup-attempt/lookup-failure construction
and the operator-facing skip log. The public surface — `new`,
`with_config`, `calculate_combined_data_with_adapter`, and the
Ethereum/Optimism convenience methods — and the partial-failure
metadata callers depend on are unchanged on the wire.

The motivation is that combined retrieval was previously a ~1600-line
file mixing scan, lookup, gas extraction, retry policy, aggregation,
and tracing. Each step is now independently unit-testable: the scanner
decode loop runs against synthetic logs without a provider, the gas
extractor exercises every (tx-present, receipt-present, transport
error) arm with synthetic inputs, and the failure helpers cover the
error-chain walk and transport-source extraction directly. The
existing end-to-end integration tests around batch + serial + zkSync
behaviour stay in `calculator.rs` and continue to gate the wired
pipeline.

Closes #24.
@suchapalaver

Copy link
Copy Markdown
Contributor Author

Self-review pass

Pre-commit /code-review (5 finder angles + sweep) and post-PR /pr-review, with findings triaged below.

# Source Finding Status
1 /code-review Angle A — line-by-line scan No findings
2 /code-review Angle B — removed-behavior auditor (tracing spans, partial-failure accumulation, zkSync two-attempt structure, serial_lookup_fallback_attempts == 0, record_fallback_attempts ordering, test-coverage preservation, helper-function migration) No findings
3 /code-review Angle C — cross-file tracer (public API callers, examples/zksync_combined_probe.rs, src/lib.rs re-exports) No findings
4 /code-review Angle D — Rust pitfalls (async, generics, Arc::clone, borrow-across-await, visibility) No findings
5 /code-review Angle E — wrapper/façade correctness (forwarding, argument order, span placement, metadata-recording order) No findings
6 /code-review Sweep — second-tier footguns + SPDX + #[allow] survivals + test-fixture queue counts No findings
7 /pr-review #[allow(clippy::too_many_arguments)] on log_combined_data_skip could disappear behind a small LookupContext struct Skipped — pre-existing shape, no failure scenario, doesn't meet the standalone-issue bar
8 /pr-review lookup_request_failed constructs an intermediate RetrievalError only to drive transport_error_string/collect_error_chain Skipped — defensible (keeps error-rendering pipeline uniform), no correctness concern

No blockers, no follow-up issues filed. The PR shipped with a single atomic commit; the two /pr-review observations were code-shape nits without concrete failure scenarios, so they don't pass the standalone-issue bar.

@suchapalaver suchapalaver merged commit 9356e24 into main May 26, 2026
11 checks passed
@suchapalaver suchapalaver deleted the refactor/decompose-combined-calculator branch May 26, 2026 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decompose CombinedCalculator into an orthogonal retrieval pipeline

1 participant