fix: harden coordinator state and deployment validation - #246
Conversation
|
@karagozemin is attempting to deploy a commit to the karagoz's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@emrekayat Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
There was a problem hiding this comment.
🟡 Changes recommended
The new rate limiter can be disabled by NaN env parsing and the “idempotent duplicate” checks can misclassify duplicates as conflicts due to case-sensitive comparisons, plus the limiter map is currently unbounded in memory.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens coordinator lifecycle handling, public diagnostic endpoints, and deployment/secret validation to better resist stale events, abusive polling, and malformed hash material.
Changes:
- Reject stale/conflicting coordinator lifecycle events while allowing exact duplicates to be idempotent.
- Add per-client rate limiting for health/readiness routes with stable 429 responses.
- Add deployment code-hash validation + a script/test harness for comparing observed deployed hashes against manifest evidence; tighten preimage/hashlock validation in SDK + simulators.
File summaries
| File | Description |
|---|---|
| scripts/verify-deployed-hashes.mjs | Adds a CLI entrypoint wrapper for deployed hash verification. |
| scripts/validate-deployments.mjs | Extends deployment manifest validation to include optional contract code-hash fields. |
| scripts/deployed-hash-verifier.test.mjs | Adds deterministic unit tests for hash normalization and mismatch reporting. |
| scripts/deployed-hash-verifier.mjs | Introduces a pure comparison function + CLI for verifying deployed code hashes vs manifest. |
| packages/sdk/test/secrets.test.ts | Adds coverage for validating full preimage/hashlock pairing. |
| packages/sdk/src/secrets/index.ts | Adds SecretHashAlgorithm and stricter runtime validation + assertPreimageMatchesHashlock. |
| e2e/sim.ts | Validates preimage format before simulation claim paths to fail fast on malformed input. |
| coordinator/test/snapshot.test.ts | Updates snapshot fixtures to reflect updated timelock ordering/values. |
| coordinator/test/readiness.test.ts | Adds tests for stable rate limiting behavior on readiness diagnostics. |
| coordinator/test/order-transitions.test.ts | Adds helper tests for monotonic status comparisons and updates fixture timelocks. |
| coordinator/test/order-service.test.ts | Adds tests for idempotent duplicates vs conflicting stale events. |
| coordinator/src/state-machine/order-machine.ts | Introduces monotonic status ranking helpers for stale transition detection/comparison. |
| coordinator/src/services/order-service.ts | Implements idempotent duplicate handling and stale/conflicting event rejection for locks/secrets. |
| coordinator/src/server/routes/orders.ts | Adds an /orders/:id/transitions endpoint for transition history retrieval. |
| coordinator/src/server/routes/health.ts | Wires health/readiness routes through the readiness rate limiter. |
| coordinator/src/server/readiness-rate-limit.ts | Adds an in-process per-client limiter returning stable 429 + rate-limit headers. |
| coordinator/src/server/app.ts | Configures readiness limiter via environment variables. |
| coordinator/src/persistence/orders-repo.ts | Adjusts end-of-file layout around transition derivation (no functional issues spotted in the moved/retained metrics method). |
Review details
Suppressed comments (2)
coordinator/src/services/order-service.ts:208
- The idempotent-duplicate check compares
txHash/resolverwith strict string equality. Both are typically hex/base32 identifiers that are case-insensitive in practice, so casing differences can cause a valid duplicate event to be rejected as a conflict.
order.dstOrderId === input.orderId &&
order.dstLockTx === input.txHash &&
order.dstLockBlock === input.blockNumber &&
order.dstTimelock === input.timelock &&
order.resolverAddress === input.resolver;
coordinator/src/services/order-service.ts:240
- Same as the lock handlers:
txHashis compared with strict equality when deciding if a secret event is an exact duplicate. If the same tx hash is represented with different hex casing, it will be incorrectly treated as a conflict.
if (order.status === "secret_revealed") {
if (order.preimage === preimage && order.secretRevealedTx === txHash) return;
throw new StaleOrderEventError(`conflicting secret event for ${publicId}`);
}
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export function createReadinessRateLimiter(options: ReadinessRateLimitOptions = {}): RequestHandler { | ||
| const limit = Math.max(1, Math.floor(options.limit ?? 30)); | ||
| const windowMs = Math.max(1_000, Math.floor(options.windowMs ?? 60_000)); | ||
| const now = options.now ?? (() => Date.now()); | ||
| const buckets = new Map<string, { startedAt: number; count: number }>(); |
| const now = options.now ?? (() => Date.now()); | ||
| const buckets = new Map<string, { startedAt: number; count: number }>(); | ||
|
|
||
| return (req: Request, res: Response, next: NextFunction) => { | ||
| const key = req.ip || req.socket.remoteAddress || "unknown"; | ||
| const timestamp = now(); | ||
| const current = buckets.get(key); | ||
| const bucket = !current || timestamp - current.startedAt >= windowMs | ||
| ? { startedAt: timestamp, count: 0 } | ||
| : current; | ||
|
|
||
| bucket.count += 1; | ||
| buckets.set(key, bucket); | ||
|
|
| const sameEvent = | ||
| order.srcOrderId === input.orderId && | ||
| order.srcLockTx === input.txHash && | ||
| order.srcLockBlock === input.blockNumber && | ||
| order.srcTimelock === input.timelock; |
|
Kod ve ilgili testler PR dalında geçiyor (coordinator: 144/144 test, TypeScript build; deployed-hash testleri 3/3). Ancak dal mevcut |
Fixes #238
Fixes #237
Fixes #240
Fixes #241