-
Notifications
You must be signed in to change notification settings - Fork 106
Feature/tx backfill fallback #933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add ENABLE_TX_BACKFILL_FALLBACK env var (default: false) - Add backfill methods to TransactionDB (getBackfillHash, setBackfill, bulkSetBackfill) - Add fallback lookup in /transaction/logs endpoint - Add POST /admin/backfill endpoint for loading backfill data When enabled, the /transaction/logs endpoint will check a fallback Redis table (backfill:<queueId>) when the primary transaction cache misses. This allows recovering transaction logs for queue IDs that were pruned from the main cache. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add isHex validation to prevent unexpected behavior if malformed data exists in the backfill table. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add BackfillEntry interface with status field ("mined" | "errored")
- Update TransactionDB to store/retrieve JSON format for backfill entries
- Add backfill fallback lookup to /transaction/status routes
- Update /transaction/logs to use new getBackfill method
- Update admin backfill schema to accept status field
- Maintain backwards compatibility for plain string tx hash entries
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughAdds Redis-backed backfill storage and admin routes to bulk-load/clear backfilled transaction hashes, plus fallback lookup logic in transaction status/log endpoints controlled by ENABLE_TX_BACKFILL_FALLBACK. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant Route as Admin Route<br/>/admin/backfill
participant DB as TransactionDB
participant Redis as Redis
Admin->>Route: POST /admin/backfill (entries: queueId,status,txHash?)
Route->>DB: bulkSetBackfill(entries)
DB->>Redis: SETNX backfill:queueId (per entry, pipelined)
Redis-->>DB: inserted/skipped results
DB-->>Route: {inserted, skipped}
Route-->>Admin: 200 OK
sequenceDiagram
actor Client
participant Route as Transaction Route<br/>(status/logs)
participant Cache as Redis Cache
participant DB as TransactionDB
participant Backfill as Redis Backfill
Client->>Route: GET /transaction/{queueId}
Route->>Cache: lookup transaction
Cache-->>Route: null
alt ENABLE_TX_BACKFILL_FALLBACK enabled
Route->>DB: getBackfill(queueId)
DB->>Backfill: GET backfill:queueId
Backfill-->>DB: BackfillEntry (or legacy string)
DB-->>Route: {status, transactionHash?}
Route->>Route: createBackfillResponse()
Route-->>Client: 200 OK (backfilled response)
else fallback disabled or no backfill
Route-->>Client: 400 TRANSACTION_NOT_FOUND
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/server/routes/admin/backfill.ts`:
- Around line 19-35: The schema allows entries with status "mined" to omit
transactionHash — update loadRequestBodySchema to enforce transactionHash when
status === "mined" by replacing the current entry object with a discriminated
union: one variant for { status: "mined", queueId: string, transactionHash:
string } and another for { status: "errored", queueId: string, transactionHash?:
string } (or omit transactionHash), using the status field as the discriminator
for entries so validation fails if a mined entry lacks transactionHash.
- Around line 7-17: The top-of-file comment for the AMEX special logic is
duplicated; remove the repeated block so there is a single explanatory comment
describing the two admin routes (loadBackfillRoute and clearBackfillRoute) and
the link to the AMEX script. Edit the comment near the definitions of
loadBackfillRoute and clearBackfillRoute to keep only one copy of the
explanatory lines (mentioning the purpose and the GitHub link) and delete the
redundant duplicate immediately following it.
🧹 Nitpick comments (4)
src/shared/db/transactions/db.ts (1)
281-288: Error handling conflates Redis errors with "key exists" results.When
erris truthy, the code incrementsskipped, but this masks actual Redis errors. A SETNX returning0(key exists) is different from a Redis error. Consider logging or surfacing errors distinctly.🔧 Proposed fix to distinguish errors from skipped entries
const results = await pipeline.exec(); + let errors = 0; for (const [err, result] of results ?? []) { - if (!err && result === 1) { + if (err) { + errors++; + } else if (result === 1) { inserted++; } else { skipped++; } } - return { inserted, skipped }; + return { inserted, skipped, errors };src/server/routes/transaction/status.ts (2)
17-82: Consider handling errored backfills and potential on-chain failures.The
createBackfillResponseassumes all "mined" transactions were successful on-chain (onchainStatus: "success"). However:
- A mined transaction could have reverted on-chain (status would be "mined" but
onchainStatusshould be "failed")- For "errored" backfills, the response returns
status: "errored"but no additional contextConsider extending BackfillEntry to capture revert status, or document the assumption that backfilled transactions are known to be successful.
💡 Optional enhancement for errored state clarity
if (backfill.status === "mined" && backfill.transactionHash) { return { ...baseResponse, transactionHash: backfill.transactionHash, onchainStatus: "success", onChainTxStatus: 1, }; } + if (backfill.status === "errored") { + return { + ...baseResponse, + errorMessage: "Transaction failed (backfill data)", + }; + } + return baseResponse;
150-162: Duplicated fallback logic across both route handlers.The backfill fallback logic is copy-pasted in both
getTransactionStatusRouteandgetTransactionStatusQueryParamRoute. Consider extracting to a shared helper to reduce duplication and ensure consistent behavior.♻️ Proposed refactor to extract shared logic
// Add helper function async function getTransactionOrBackfill( queueId: string, ): Promise<{ transaction: AnyTransaction } | { backfill: BackfillEntry } | null> { const transaction = await TransactionDB.get(queueId); if (transaction) { return { transaction }; } if (env.ENABLE_TX_BACKFILL_FALLBACK) { const backfill = await TransactionDB.getBackfill(queueId); if (backfill) { return { backfill }; } } return null; }Then use in both handlers to eliminate duplication.
Also applies to: 211-223
src/server/routes/admin/backfill.ts (1)
26-28: Missing hex format validation for transactionHash.The
transactionHashfield accepts any string, but transaction hashes should be valid hex (0x-prefixed, 66 characters). Theget-logs.tsroute validates withisHex()at read time, but rejecting invalid hashes at write time provides better feedback.🔒 Add pattern validation to schema
transactionHash: Type.Optional( - Type.String({ description: "Transaction hash (0x...). Required for mined transactions." }), + Type.String({ + description: "Transaction hash (0x...). Required for mined transactions.", + pattern: "^0x[a-fA-F0-9]{64}$", + }), ),
… comment - Use discriminated union so transactionHash is required for mined entries - Remove duplicated AMEX comment block Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changes
How this PR will be tested
Output
(Example: Screenshot/GIF for UI changes, cURL output for API changes)
PR-Codex overview
This PR introduces functionality for managing transaction backfills in the application. It adds routes for loading and clearing backfill entries, updates the environment configuration to enable backfill fallback, and enhances the transaction status retrieval logic with backfill support.
Detailed summary
loadBackfillRouteandclearBackfillRouteinsrc/server/routes/admin/backfill.ts.withRoutesinsrc/server/routes/index.tsto register new routes.ENABLE_TX_BACKFILL_FALLBACKinsrc/shared/utils/env.ts.getTransactionLogsand transaction status routes to utilize backfill data.getBackfill,getBackfillHash,setBackfill, andbulkSetBackfillmethods inTransactionDBfor managing backfill entries.BackfillEntryinterface to define backfill data structure insrc/shared/db/transactions/db.ts.getTransactionStatusRoute.Summary by CodeRabbit
New Features
Chores