Skip to content

fix: wrap settlement.ts RPC calls in withTimeout, remove decode dead code (COW-991)#84

Open
lgahdl wants to merge 6 commits into
developfrom
luizhatem/cow-991-f8-wrap-settlement-rpc-calls-in-withtimeout
Open

fix: wrap settlement.ts RPC calls in withTimeout, remove decode dead code (COW-991)#84
lgahdl wants to merge 6 commits into
developfrom
luizhatem/cow-991-f8-wrap-settlement-rpc-calls-in-withtimeout

Conversation

@lgahdl

@lgahdl lgahdl commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Grant Review Finding

[F8] settlement.ts RPC calls not wrapped in withTimeout + inaccurate "negligible volume" comment — MINOR (correctness) (COW-991)

settlement.tsgetTransactionReceipt / getCode / call / readContract are not wrapped in withTimeout, unlike the block handlers (COW-921 timeout discipline is block-handler-scoped, but this event handler runs in the per-block TX too). Additionally, getTransactionReceipt runs unconditionally per settlement, before the dedup check — and the comment "Volume is negligible" is wrong: 27,469 adapters were resolved, so this is meaningful sequential unwrapped RPC during backfill.


Summary

  • Wraps all 4 RPC calls in settlement.ts (getTransactionReceipt, getCode, call, readContract) in withTimeout(BLOCK_HANDLER_RPC_TIMEOUT_MS), applying the same COW-921 timeout discipline used in block handlers
  • Removes the decode-only-for-logging decodeAbiParameters block — the only consumer was the console.log; the log now shows adapter, eoa, block, chain
  • Replaces the inaccurate "Volume is negligible" comment with the actual observed figure (27k+ adapters resolved in practice)
  • Removes unused decodeAbiParameters from viem import

Test plan

  • pnpm typecheck — passes
  • pnpm test — 19/19 passing
  • Each RPC call site has a matching withTimeout wrapper with a descriptive label
  • TimeoutError on getTransactionReceipt returns early; TimeoutError on inner calls continues to next log

🤖 Generated with Claude Code

…code (COW-991)

Applies the COW-921 timeout discipline to the Settlement event handler:
wraps getTransactionReceipt, getCode, call, and readContract in withTimeout
using BLOCK_HANDLER_RPC_TIMEOUT_MS. Removes the decode-only-for-logging
decodeAbiParameters block (F10) and replaces the inaccurate "volume is
negligible" comment with the actual observed figure (27k+ adapters).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Jun 1, 2026

Copy link
Copy Markdown

COW-991

@lgahdl lgahdl changed the base branch from main to develop June 2, 2026 13:52
@lgahdl

lgahdl commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Indexer review — settlement.ts RPC timeout wrapping

Good addition of withTimeout guards across all four RPC call sites. One timing concern worth noting:

The four calls — getTransactionReceipt, getCode, call (FACTORY), and readContract (owner) — are all guarded individually with BLOCK_HANDLER_RPC_TIMEOUT_MS (15 s). They run sequentially inside the per-trade-log loop, so in a settlement with N adapter trades the worst-case block handler wall time is 4 × 15 s × N. Even with just 2 adapter trades that's 120 s before the handler yields — well beyond any sane block cadence.

Consider either:

  1. Using a tighter per-call budget for the inner-loop calls (e.g. 5 s), reserving the full 15 s only for getTransactionReceipt which is the one call that happens once per event.
  2. Or at minimum add a comment near the loop documenting this worst-case so a future reader knows to keep N small (i.e. this is expected to be ~1 adapter per settlement in practice).

The getTransactionReceipt early-return on timeout is clean — if the receipt fetch fails, skipping the entire event is the right move.

@lgahdl

lgahdl commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Code review — settlement.ts withTimeout (COW-991)

Worst-case latency per settlement event

The handler now makes up to 4 sequential RPC calls per matching Trade log, each guarded by BLOCK_HANDLER_RPC_TIMEOUT_MS (15 s):

  1. getTransactionReceipt — once per Settlement event (outside the loop)
  2. getCode — once per Trade log whose owner is not already mapped
  3. eth_call (FACTORY selector) — once per contract-code owner
  4. readContract (owner()) — once per confirmed Aave adapter

For a settlement with N Trade logs that are all unseen contract addresses (e.g. a batch flashloan with many adapters), the sequential worst-case is 1 × 15 s (receipt) + N × 15 s (getCode) + N × 15 s (factory call) + N × 15 s (readContract). At N = 10 that is 1 + 30 = ~30.5 minutes if every call times out. The DISABLE_SETTLEMENT_FACTORY_CHECK kill switch mitigates runaway scenarios in production, but under normal operation (no timeouts) the sequential calls are fine.

Tighter timeout warranted for inner-loop calls

getCode and eth_call for the factory selector are cheap reads that should resolve in well under 1 s on a healthy node. Using the full 15 s cap for these inner-loop calls means a degraded node can hold the block handler open for 15 s × 3 × N trades before any of the guards fire. A tighter timeout — 3–5 s — for getCode and the factory eth_call would catch degraded-node stalls faster without risking false positives on healthy RPCs. The outer getTransactionReceipt and the final readContract(owner()) are more legitimately bounded by the 15 s wall.

continue vs return on inner-loop timeouts

getCode and readContract(owner()) timeout handlers use continue — correct, they skip the current Trade log and process the rest. getTransactionReceipt timeout uses return — also correct, since without the receipt there are no logs to process. No logic issues here.

Dead code removal looks correct

The removal of decodeAbiParameters import and the non-indexed Trade log field decoding (sellToken, buyToken, amounts, orderUid from log.data) is clean — those fields were unused after the PR-91 orderUid unwrap.

Summary: No bugs. One actionable recommendation: use a ~3–5 s timeout for getCode and the factory eth_call to bound per-trade latency on degraded nodes.

…ndler (COW-991)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread src/application/handlers/settlement.ts Outdated
);
} catch (err) {
if (err instanceof TimeoutError) return;
throw err;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any error is throw ponder will stop the indexing. Instead of having this here, it probably makes more sense to have a clear pool based async method as we have for the composable cow orders.

On the Anxo feedback report it mentions a decision of reserve I/O to block handlers. I think this was a misunderstanding of one of ours ADRs but it would make the system more robust.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored to pool-based: the event handler now just enqueues the tx hash into a settlementQueue table, and a new SettlementResolver:block handler drains the queue and does all RPC work — no throw anywhere, errors warn and skip.

lgahdl and others added 3 commits June 4, 2026 19:36
…r (COW-991)

The GPv2Settlement event handler now only enqueues the tx hash into
settlementQueue. A new SettlementResolver:block handler drains the queue
and performs all RPC calls (getTransactionReceipt, getCode, call, readContract).
RPC errors warn + skip rather than re-throw, so the indexer can never crash
from a transient node failure in settlement processing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…entChains

SettlementResolver now uses settlementChains.map() (chains with a flash loan
router) instead of hardcoded mainnet, consistent with develop's dynamic
chain config pattern.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@yvesfracari yvesfracari left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the documentation on how the lifecycle of this kind of order works.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lgahdl

lgahdl commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Added a lifecycle description for the flash loan adapter order flow in docs/architecture.md (settlement.ts section):

Stage 1 — GPv2Settlement:Settlement (enqueue only): The event handler writes the tx hash to settlementQueue and returns immediately. No RPC calls here — errors never crash the event handler.

Stage 2 — SettlementResolver:block (drain and resolve): Every block, this handler processes queued settlements. For each tx: fetch receipt → find Trade logs → extract owner → skip if already mapped / EOA → call FACTORY() via raw eth_call → if Aave adapter, call owner() → write ownerMapping + backfill ownerAddressType on any existing generator rows.

Also updated the overview paragraph to mention both stages and the SettlementResolver:block handler.

lgahdl added a commit that referenced this pull request Jun 8, 2026
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@jeffersonBastos jeffersonBastos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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.

3 participants