Skip to content

fix: minor cleanups from grant review (COW-996, COW-999, COW-1003)#80

Open
lgahdl wants to merge 2 commits into
developfrom
luizhatem/cow-996-999-1003-minor-cleanups
Open

fix: minor cleanups from grant review (COW-996, COW-999, COW-1003)#80
lgahdl wants to merge 2 commits into
developfrom
luizhatem/cow-996-999-1003-minor-cleanups

Conversation

@lgahdl
Copy link
Copy Markdown
Contributor

@lgahdl lgahdl commented Jun 1, 2026

Summary

Three self-contained minor fixes from the grant review, batched together since each is a single-file change with no risk of conflict.

Changes

  • COW-1003 [F2] src/utils/order-types.ts — add CirclesBackingOrder to DETERMINISTIC_ORDER_TYPES. It was already precomputed as deterministic in uidPrecompute.ts (added in COW-906) but missing from this set, causing a spurious non-deterministic log warning for every CirclesBackingOrder generator
  • COW-996 [F3] src/application/handlers/composableCow.ts — replace the reference to the nonexistent ConditionalOrderCancelled event with an accurate description of the actual on-chain cancellation detection (SingleOrderNotAuthed in C1 + C5 singleOrders() sweep)
  • COW-999 [F10] src/application/handlers/settlement.ts — remove the decodeAbiParameters block and console.log at lines 194–222 that decoded Trade log fields solely for a log line; also removes the now-unused decodeAbiParameters import from viem

How to Test

  1. pnpm test — all tests pass (includes new tests/utils/order-types.test.ts with 6 assertions guarding COW-1003)
  2. pnpm typecheck — no errors
  3. pnpm lint — no errors

Checklist

  • Tests pass locally
  • Linting passes
  • Documentation updated (if needed)
  • Breaking changes documented (if any)

Breaking Changes

None

Related Issues

Closes COW-996
Closes COW-999
Closes COW-1003

- COW-1003 [F2]: add CirclesBackingOrder to DETERMINISTIC_ORDER_TYPES; it was
  already precomputed in uidPrecompute.ts but missing from the set, causing
  spurious non-deterministic log warnings
- COW-996 [F3]: replace nonexistent ConditionalOrderCancelled event reference
  with accurate description of actual cancellation detection (SingleOrderNotAuthed
  + C5 singleOrders() sweep)
- COW-999 [F10]: remove decode-only-for-logging dead code in settlement.ts;
  decodeAbiParameters block and console.log were decoding Trade log fields solely
  for a log line, with no downstream use

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lgahdl lgahdl changed the base branch from main to develop June 2, 2026 13:52
@linear-code
Copy link
Copy Markdown

linear-code Bot commented Jun 2, 2026

COW-996

COW-999

COW-1003

@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

Documentation / CI review

COW-996 [F3] — comment in composableCow.ts introduces stale names.
The updated comment at lines ~26–29 now reads:

SingleOrderNotAuthed (C1 block handler) and the C5 singleOrders() sweep

This is the PR that renames C1→OrderDiscoveryPoller and C5→CancellationWatcher (PR #83). These two PRs will conflict if merged out of order, or if #80 lands first the stale C1 / C5 names end up in the merged codebase. The comment should use the new names already: OrderDiscoveryPoller (block handler) and CancellationWatcher. Update before or immediately after merge depending on ordering relative to #83.

COW-999 [F10] — dead decodeAbiParameters code removed: OK. No docs impact.

COW-1003 [F2] — CirclesBackingOrder added to DETERMINISTIC_ORDER_TYPES.
docs/supported-order-types.md documents the five original order types. If CirclesBackingOrder is live and being indexed, it should be listed there too. Worth checking whether that doc needs a new entry — not a blocker for this PR, but flag as a follow-on.

Overall: One actionable item (stale C1/C5 in composableCow.ts comment). No deployment.md or .env.example changes needed.

@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

Review: Minor cleanups (COW-996/999/1003)

settlement.ts — removal of decodeAbiParameters and log fields

The removed block decoded sellToken, buyToken, sellAmount, buyAmount, and orderUid from log.data purely to include them in the [COW:SETTLEMENT:TRADE] AAVE_ADAPTER_MAPPED log line. Removing that decode is fine as long as no other consumer downstream of this handler relied on those fields for correctness — and from the code they were log-only. The import of decodeAbiParameters is correctly removed too. No regression risk here.

order-types.tsCirclesBackingOrder added to DETERMINISTIC_ORDER_TYPES

The type is already in the OrderType union and has a Gnosis-only handler address (0x43866c5602b0e3b3272424396e88b849796dc608). Adding it to DETERMINISTIC_ORDER_TYPES is consistent with the comment "keep in sync with the switch in precomputeOrderUids." One thing to verify: does the precomputeOrderUids switch (wherever it lives) actually have a case "CirclesBackingOrder" branch? If it does not, the UID precompute path would silently fall through for this type while isDeterministicOrderType now returns true for it, which could cause the C5 deterministic cancellation sweeper to run on generators whose UIDs haven't been precomputed. Worth a quick cross-check before merge.

composableCow.ts — comment update only

Cosmetic; the updated description is more accurate. No code change.

Tests in tests/utils/order-types.test.ts cover all three members of the set and the negative cases — good.

Overall the changes look correct, just flag the precomputeOrderUids sync question above.

… precompute (COW-996/999/1003)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines +27 to +29
* this is rare — the standard on-chain cancellation path is detected via
* SingleOrderNotAuthed (OrderDiscoveryPoller) and the CancellationWatcher,
* both of which work correctly.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe it worth to log this on a issue

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tbh I don't think this tests are relevant. Instead, we should properly handle types related to order types and deterministic order types.

Two things that would make it better:

  • replace isDeterministicOrderType to a Record<OrderType, boolean>
  • in multiple places, order type is being typed as string while OrderType should be used

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.

2 participants