refactor: drop reaper status param for terminal-set guard; simplify repository condition per review

Author: claude

lib/crons/gs-reaper/gs-reaper.ts

@@ -8,7 +8,7 @@ import { BLOCK_RANGE, REAPER_MAX_ATTEMPTS, DYNAMO_BATCH_WRITE_MAX, OLDEST_BLOCK_
 import { ethers } from 'ethers'
 import { CosignedPriorityOrder, CosignedV2DutchOrder, CosignedV3DutchOrder, DutchOrder, FillInfo, CosignedHybridOrder, OrderType, OrderValidation, OrderValidator, REACTOR_ADDRESS_MAPPING, UniswapXEventWatcher, UniswapXOrder } from '@uniswap/uniswapx-sdk'
 import { parseOrder } from '../../handlers/OrderParser'
-import { AVERAGE_BLOCK_TIME, getSettledAmounts } from '../../handlers/check-order-status/util'
+import { AVERAGE_BLOCK_TIME, getSettledAmounts, IS_TERMINAL_STATE } from '../../handlers/check-order-status/util'
 import { ChainId } from '../../util/chain'
 import { getRpcUrl } from '../../Config'
 import { LimitOrdersRepository } from '../../repositories/limit-orders-repository'
@@ -229,8 +229,7 @@ export class GSReaper {
           provider,
           state.chainId,
           this.log,
-          state.failedFillScanRanges,
-          this.unresolvedOrderStatus
+          state.failedFillScanRanges
         )
 
         return {
@@ -413,8 +412,7 @@ async function checkCancelledOrders(
   provider: ethers.providers.StaticJsonRpcProvider,
   chainId: number,
   log: Logger,
-  failedFillScanRanges: BlockRange[],
-  unresolvedOrderStatus: ORDER_STATUS,
+  failedFillScanRanges: BlockRange[]
 ): Promise<Record<string, OrderUpdate>> {
   const orderUpdates = { ...existingUpdates }
   const quoter = new OrderValidator(provider, chainId)
@@ -463,14 +461,10 @@ async function checkCancelledOrders(
     if (!orderUpdates[orderHash]) {
       try {
         const { order, signature, entity } = await getOrderByHash(repo, orderHash)
-        // Another writer (e.g. the check-order-status state machine) may have
-        // resolved this order since the run's GET_OPEN_ORDERS snapshot -- most
-        // importantly to FILLED, which a used nonce is also consistent with.
-        // Only resolve orders whose DB status still matches the snapshot.
-        if (entity.orderStatus !== unresolvedOrderStatus) {
-          log.info(
-            `Order ${orderHash} status is now ${entity.orderStatus} (no longer ${unresolvedOrderStatus}); skipping resolution`
-          )
+        // Another writer (e.g. check-order-status) may have resolved this order since
+        // the run's GET_OPEN_ORDERS snapshot; never re-resolve a terminal order.
+        if (IS_TERMINAL_STATE(entity.orderStatus)) {
+          log.info(`Order ${orderHash} is already terminal (${entity.orderStatus}); skipping resolution`)
           continue
         }
         // We only check for nonce used and expired for permissioned tokens

lib/repositories/generic-orders-repository.ts

@@ -15,13 +15,9 @@ import { IndexMapper } from './IndexMappers/IndexMapper'
 
 export const MAX_ORDERS = 50
 
+// aws-sdk v2 (used by dynamodb-toolbox here) sets both code and name to the error code
 function isConditionalCheckFailed(e: unknown): boolean {
-  return (
-    typeof e === 'object' &&
-    e !== null &&
-    ((e as { code?: string }).code === 'ConditionalCheckFailedException' ||
-      (e as { name?: string }).name === 'ConditionalCheckFailedException')
-  )
+  return (e as { code?: string } | null)?.code === 'ConditionalCheckFailedException'
 }
 
 // Shared implementation for Dutch and Limit orders
@@ -124,6 +120,11 @@ export abstract class GenericOrdersRepository<
         `cannot find order by hash when updating order status, hash: ${orderHash}`
       )
 
+      // FILLED is terminal; the DynamoDB condition makes the no-downgrade check atomic with the write
+      const conditions =
+        status === ORDER_STATUS.FILLED
+          ? {}
+          : { conditions: [{ attr: TABLE_KEY.ORDER_STATUS, ne: ORDER_STATUS.FILLED }] }
       await this.entity.update(
         {
           [TABLE_KEY.ORDER_HASH]: orderHash,
@@ -132,18 +133,11 @@ export abstract class GenericOrdersRepository<
           ...(fillBlock && { fillBlock }),
           ...(settledAmounts && { settledAmounts })
         },
-        // FILLED is terminal: once an order is recorded as filled, no writer may
-        // downgrade it to another status (e.g. a reaper run whose fill scan
-        // predates the fill misreading its used nonce as a cancellation).
-        // Enforced as a DynamoDB condition so the check is atomic with the write.
-        status === ORDER_STATUS.FILLED
-          ? {}
-          : { conditions: [{ attr: TABLE_KEY.ORDER_STATUS, ne: ORDER_STATUS.FILLED }] }
+        conditions
       )
     } catch (e) {
       if (isConditionalCheckFailed(e)) {
-        // The order reached FILLED between our read and this write; the update
-        // would downgrade a terminal status, so skip it rather than retry.
+        // the order was FILLED by another writer since our read; skip the downgrade rather than throw
         log.warn('skipping updateOrderStatus: order is already in terminal status FILLED', { orderHash, status })
         return
       }

test/unit/handlers/gs-reaper/gs-reaper.test.ts

@@ -352,14 +352,12 @@ describe('GSReaper', () => {
       expect(result?.orderUpdates[MOCK_ORDER_ENTITY.orderHash].status).toBe(ORDER_STATUS.CANCELLED)
     })
 
-    it('does NOT resolve an order whose DB status changed since the run snapshot', async () => {
-      // Regression (PROTO-1201): the run's order-hash snapshot is taken in
-      // GET_OPEN_ORDERS, but another writer (e.g. the check-order-status state
-      // machine) can resolve the order -- most importantly to FILLED -- before
-      // CHECK_CANCELLED validates it. A used nonce is consistent with that
-      // fill, so the reaper must re-check the CURRENT DB status and skip
-      // orders that already moved on, instead of clobbering FILLED with
-      // CANCELLED.
+    it('does NOT resolve an order whose DB status is already terminal', async () => {
+      // Regression (PROTO-1201): another writer (e.g. the check-order-status
+      // state machine) can resolve the order to FILLED between the run's
+      // GET_OPEN_ORDERS snapshot and CHECK_CANCELLED. A used nonce is
+      // consistent with that fill, so the reaper must skip already-terminal
+      // orders instead of clobbering FILLED with CANCELLED.
       await mockOrdersRepository.addOrder({
         ...MOCK_ORDER_ENTITY,
         orderStatus: ORDER_STATUS.FILLED,