feat: implement commitReservation/voidReservation mutations and reservation+concurrency test suites (ENG-33, ENG-36)#105
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Connorbelez, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors ledger mutation internals (commitReservation, voidReservation, reserveShares) to change return shapes and linkage to use reservation._id and reservation account fields; adds pre-postEntry pending-field clearing; introduces extensive new and updated tests for concurrency, reservation lifecycles, idempotency, and invariants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Greptile SummaryThis PR implements the final two legs of the two-phase reservation lifecycle — Key concerns found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant commitReservation
participant voidReservation
participant DB as Convex DB
participant postEntry
Note over Caller,postEntry: reserveShares (existing)
Caller->>DB: patch sellerAccount.pendingCredits += amount
Caller->>DB: patch buyerAccount.pendingDebits += amount
Caller->>DB: insert ledger_reservations {status: "pending"}
Note over Caller,postEntry: commitReservation (new)
Caller->>commitReservation: {reservationId, idempotencyKey, effectiveDate}
commitReservation->>DB: query by_idempotency (early return if exists)
commitReservation->>DB: get reservation → assert status == "pending"
commitReservation->>DB: patch sellerAccount.pendingCredits -= amount
commitReservation->>DB: patch buyerAccount.pendingDebits -= amount
commitReservation->>postEntry: SHARES_COMMITTED (debit=buyer, credit=seller)
postEntry->>DB: balanceCheck on seller (sees cleared pendingCredits ✓)
postEntry->>DB: update cumulativeDebits/Credits
postEntry->>DB: insert ledger_journal_entries
commitReservation->>DB: patch reservation {status:"committed", commitJournalEntryId, resolvedAt}
commitReservation-->>Caller: journalEntry
Note over Caller,postEntry: voidReservation (new)
Caller->>voidReservation: {reservationId, reason, idempotencyKey, effectiveDate}
voidReservation->>DB: query by_idempotency (early return if exists)
voidReservation->>DB: get reservation → assert status == "pending"
voidReservation->>postEntry: SHARES_VOIDED [AUDIT_ONLY] (debit=seller, credit=buyer)
postEntry->>DB: skip balanceCheck (AUDIT_ONLY)
postEntry->>DB: skip cumulative updates (AUDIT_ONLY)
postEntry->>DB: insert ledger_journal_entries
voidReservation->>DB: patch sellerAccount.pendingCredits -= amount
voidReservation->>DB: patch buyerAccount.pendingDebits -= amount
voidReservation->>DB: patch reservation {status:"voided", voidJournalEntryId, resolvedAt}
voidReservation-->>Caller: journalEntry
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds the remaining steps of the two-phase “share reservation” workflow in the Convex ledger by implementing reservation commit/void mutations and expanding the test suite to cover reservation lifecycle and invariants.
Changes:
- Added
commitReservationinternal mutation to finalize a pending reservation, clear pending fields, and post aSHARES_COMMITTEDentry. - Added
voidReservationinternal mutation to void a pending reservation, release pending fields, and post aSHARES_VOIDEDaudit entry. - Expanded and added vitest coverage for reservation lifecycle behaviors, supply invariants, and sequence ordering.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| convex/ledger/mutations.ts | Implements commitReservation and voidReservation internal mutations, including idempotency checks, pending-field updates, and reservation finalization. |
| convex/ledger/tests/reservation.test.ts | Adds tests for commit/void happy paths and state-machine edge cases (double commit/void, commit-after-void, void-after-commit, mutex lifecycle). |
| convex/ledger/tests/concurrency.test.ts | Adds broader “concurrency/invariant” style coverage: min-fraction scenarios, double-mint prevention, sequence ordering, and supply invariant checks across reserve/commit/void flows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Connorbelez
force-pushed
the
Connorbelez/eng33-lifecycle-tests
branch
from
31bbeab to
502f923
Compare
…vation+concurrency test suites (ENG-33, ENG-36) - Add commitReservation internalMutation: finalizes pending reservation by posting SHARES_COMMITTED entry, clearing pending fields, and updating reservation status to "committed" - Add voidReservation internalMutation: cancels pending reservation by posting SHARES_VOIDED entry (audit-only), releasing pending holds, and updating reservation status to "voided" - Fix balanceCheck ordering: clear pending fields before postEntry so available balance reflects the units being committed - Add 7 reservation lifecycle tests: commit/void happy paths, double-commit/void prevention, cross-state rejection, mutex lifecycle - Add 6 concurrency tests: min-fraction transfer enforcement, double-mint OCC, sequence number gap-freedom, supply invariant under mixed scenarios Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… utility, replay tests - Add entryType + reservationId validation to commitReservation/voidReservation idempotency checks, mirroring the reserveShares safety pattern - Extract getConvexErrorCode to shared testUtils.ts to eliminate duplication - Add idempotency replay tests for both commitReservation and voidReservation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
convex/ledger/__tests__/reservation.test.ts (1)
523-563: Add same-key replay coverage forcommitReservationandvoidReservation.These blocks only exercise retries with a fresh idempotency key. Since the mutation code changed the same-key replay path too, I'd add one retry per mutation that reuses the original key, asserts the same journal entry
_idcomes back, and verifies balances/pending fields stay unchanged.Also applies to: 658-697
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/ledger/__tests__/reservation.test.ts` around lines 523 - 563, Add same-key replay tests for commitReservation and voidReservation by reusing the original idempotency keys and asserting idempotent behavior: after the first successful executeCommitReservation call capture the returned journal entry id, call executeCommitReservation again with the same idempotencyKey and assert the returned journal entry _id is identical and balances/pending fields remain unchanged (use getAccount and getPostedBalance to verify). Mirror this pattern in the voidReservation test (the block around the other test at 658-697): after the first executeVoidReservation capture its journal id, call executeVoidReservation with the same idempotencyKey, assert the journal entry _id matches the original, and verify balances and reservation pending state are unchanged. Ensure you reference functions executeReserveShares, executeCommitReservation, executeVoidReservation, getAccount, and getPostedBalance when adding these assertions.convex/ledger/__tests__/concurrency.test.ts (1)
20-233: Extract the shared ledger test helpers.This harness, mutation casts, and
getConvexErrorCodeare essentially duplicated fromconvex/ledger/__tests__/reservation.test.ts. Pulling them into one helper will keep future mutation-shape changes from drifting across the suites.Based on learnings, "Extract shared logic into separate modules instead of duplicating code across multiple files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/ledger/__tests__/concurrency.test.ts` around lines 20 - 233, The test file duplicates harness, helpers and mutation-cast logic that should be extracted into a shared test helper module: create a new helper module that exports createTestHarness, asLedgerUser, SYS_SOURCE, initCounter, mintAndIssue, getAccount, the Reserve/Commit/Void types and the cast constants reserveSharesMutation, commitReservationMutation, voidReservationMutation, the executeReserveShares/executeCommitReservation/executeVoidReservation wrappers, and getConvexErrorCode; then remove the duplicated implementations from this test (and from convex/ledger/__tests__/reservation.test.ts) and replace them with imports from the shared helper, updating any tests to use the exported symbols (e.g., reserveSharesMutation, executeReserveShares, getConvexErrorCode) so mutation shapes and error-parsing stay consistent across suites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/ledger/__tests__/concurrency.test.ts`:
- Around line 299-320: The test currently awaits the first mint before starting
the second, so it doesn't exercise OCC contention; change the test to start two
concurrent calls to auth.mutation(api.ledger.mutations.mintMortgage) (e.g.,
create p1 and p2 promises with different idempotencyKey values like
"mint-double-1" and "mint-double-2") without awaiting p1 before starting p2,
then await both (Promise.allSettled or Promise.all) and assert that one of the
results is a rejection containing /ALREADY_MINTED/ while the other completed
successfully; keep references to createTestHarness, asLedgerUser, initCounter
and api.ledger.mutations.mintMortgage to locate the code.
In `@convex/ledger/mutations.ts`:
- Around line 582-590: The idempotency lookup in the ledger_journal_entries
query returns any entry with the same idempotencyKey but doesn't verify
entryType or reservationId, allowing collisions across reservations; update the
lookup in the affected branches (the one using
ctx.db.query("ledger_journal_entries").withIndex("by_idempotency", ...) and the
similar block around lines 655-663) to also filter by args.entryType and
args.reservationId (or the expected reservation id) so you only treat an entry
as a replay for the same reservation and operation (e.g., within
commitReservation and voidReservation) before returning it.
- Around line 641-646: The code currently patches reservation.status directly
using ctx.db.patch(reservation._id, { status: "committed", commitJournalEntryId:
journalEntry._id, resolvedAt: Date.now() }), which bypasses the Transition
Engine; replace that direct patch with the canonical Transition Engine call
(e.g., TransitionEngine.transitionReservation or
ReservationRepository.transitionTo) so the commit transition flows through the
state machine and audit/journaling logic — pass the journalEntry id and
resolvedAt as transition metadata so the engine records commitJournalEntryId and
timestamp; apply the same replacement for the other inline patch at the other
location (the block around lines 712-717) so all status changes go through the
Transition Engine.
---
Nitpick comments:
In `@convex/ledger/__tests__/concurrency.test.ts`:
- Around line 20-233: The test file duplicates harness, helpers and
mutation-cast logic that should be extracted into a shared test helper module:
create a new helper module that exports createTestHarness, asLedgerUser,
SYS_SOURCE, initCounter, mintAndIssue, getAccount, the Reserve/Commit/Void types
and the cast constants reserveSharesMutation, commitReservationMutation,
voidReservationMutation, the
executeReserveShares/executeCommitReservation/executeVoidReservation wrappers,
and getConvexErrorCode; then remove the duplicated implementations from this
test (and from convex/ledger/__tests__/reservation.test.ts) and replace them
with imports from the shared helper, updating any tests to use the exported
symbols (e.g., reserveSharesMutation, executeReserveShares, getConvexErrorCode)
so mutation shapes and error-parsing stay consistent across suites.
In `@convex/ledger/__tests__/reservation.test.ts`:
- Around line 523-563: Add same-key replay tests for commitReservation and
voidReservation by reusing the original idempotency keys and asserting
idempotent behavior: after the first successful executeCommitReservation call
capture the returned journal entry id, call executeCommitReservation again with
the same idempotencyKey and assert the returned journal entry _id is identical
and balances/pending fields remain unchanged (use getAccount and
getPostedBalance to verify). Mirror this pattern in the voidReservation test
(the block around the other test at 658-697): after the first
executeVoidReservation capture its journal id, call executeVoidReservation with
the same idempotencyKey, assert the journal entry _id matches the original, and
verify balances and reservation pending state are unchanged. Ensure you
reference functions executeReserveShares, executeCommitReservation,
executeVoidReservation, getAccount, and getPostedBalance when adding these
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24507227-18be-423d-bcb9-7e0c0ea51eb3
📒 Files selected for processing (3)
convex/ledger/__tests__/concurrency.test.tsconvex/ledger/__tests__/reservation.test.tsconvex/ledger/mutations.ts
Connorbelez
force-pushed
the
Connorbelez/eng33-lifecycle-tests
branch
from
29baa5e to
319996a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
convex/ledger/__tests__/reservation.test.ts (1)
71-112: Consider extracting mutation wrapper types to testUtils.ts.The typed mutation wrappers (
CommitReservationArgs,CommitReservationMutation,VoidReservationArgs,VoidReservationMutation) are duplicated between this file andconcurrency.test.ts. Consider moving them totestUtils.tsto reduce duplication and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/ledger/__tests__/reservation.test.ts` around lines 71 - 112, Extract the duplicated typed mutation wrappers into a shared test utils module: create/export the types CommitReservationArgs, CommitReservationMutation, VoidReservationArgs, VoidReservationMutation from testUtils (or add them to an existing testUtils.ts), then replace the local declarations in reservation.test.ts and concurrency.test.ts with imports of those types and cast helpers (commitReservationMutation, voidReservationMutation) to the shared types; ensure you export the types and update both files to import the types and the const casts so compilation and type-checking of commitReservation/_handler and voidReservation/_handler continue to work.convex/ledger/__tests__/concurrency.test.ts (1)
99-172: Mutation wrapper types duplicated from reservation.test.ts.These type definitions and wrapper constants are identical to those in
reservation.test.ts. As suggested in that file's review, consider consolidating these intotestUtils.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/ledger/__tests__/concurrency.test.ts` around lines 99 - 172, The duplicate mutation wrapper types and constants (ReserveSharesArgs, ReserveSharesMutation, reserveSharesMutation, CommitReservationArgs, CommitReservationMutation, commitReservationMutation, VoidReservationArgs, VoidReservationMutation, voidReservationMutation) should be extracted into a shared test utilities module (e.g., testUtils.ts); create the consolidated type declarations and the three wrapper constants there, export them, then replace the copies in this file with imports from testUtils.ts so both concurrency.test.ts and reservation.test.ts consume the single shared definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/ledger/__tests__/testUtils.ts`:
- Around line 23-25: The regex used when extracting error codes (the call to
value.match(...) that assigns to match in testUtils.ts) is missing the
IDEMPOTENT_REPLAY_FAILED token; update the pattern to include
IDEMPOTENT_REPLAY_FAILED among the alternation list (alongside INVALID_AMOUNT,
SAME_ACCOUNT, etc.) so tests detect that error code thrown by the mutations (see
occurrences in mutations.ts).
---
Nitpick comments:
In `@convex/ledger/__tests__/concurrency.test.ts`:
- Around line 99-172: The duplicate mutation wrapper types and constants
(ReserveSharesArgs, ReserveSharesMutation, reserveSharesMutation,
CommitReservationArgs, CommitReservationMutation, commitReservationMutation,
VoidReservationArgs, VoidReservationMutation, voidReservationMutation) should be
extracted into a shared test utilities module (e.g., testUtils.ts); create the
consolidated type declarations and the three wrapper constants there, export
them, then replace the copies in this file with imports from testUtils.ts so
both concurrency.test.ts and reservation.test.ts consume the single shared
definitions.
In `@convex/ledger/__tests__/reservation.test.ts`:
- Around line 71-112: Extract the duplicated typed mutation wrappers into a
shared test utils module: create/export the types CommitReservationArgs,
CommitReservationMutation, VoidReservationArgs, VoidReservationMutation from
testUtils (or add them to an existing testUtils.ts), then replace the local
declarations in reservation.test.ts and concurrency.test.ts with imports of
those types and cast helpers (commitReservationMutation,
voidReservationMutation) to the shared types; ensure you export the types and
update both files to import the types and the const casts so compilation and
type-checking of commitReservation/_handler and voidReservation/_handler
continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a228d695-b7b4-42c0-a872-494fd34173ed
📒 Files selected for processing (4)
convex/ledger/__tests__/concurrency.test.tsconvex/ledger/__tests__/reservation.test.tsconvex/ledger/__tests__/testUtils.tsconvex/ledger/mutations.ts
…vation+concurrency test suites (ENG-33, ENG-36) (#105) - Add commitReservation internalMutation: finalizes pending reservation by posting SHARES_COMMITTED entry, clearing pending fields, and updating reservation status to "committed" - Add voidReservation internalMutation: cancels pending reservation by posting SHARES_VOIDED entry (audit-only), releasing pending holds, and updating reservation status to "voided" - Fix balanceCheck ordering: clear pending fields before postEntry so available balance reflects the units being committed - Add 7 reservation lifecycle tests: commit/void happy paths, double-commit/void prevention, cross-state rejection, mutex lifecycle - Add 6 concurrency tests: min-fraction transfer enforcement, double-mint OCC, sequence number gap-freedom, supply invariant under mixed scenarios Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved idempotency behavior and stronger consistency for reservation commits/voids. * Ensured pending balances are cleared and reflected correctly when reservations finalize. * Tightened error reporting and validation for reservation and account states. * **Tests** * Added extensive concurrency and lifecycle tests covering transfers, minting, sequencing, and supply invariants. * Added utilities to extract and validate error codes in tests. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
posting SHARES_COMMITTED entry, clearing pending fields, and updating
reservation status to "committed"
posting SHARES_VOIDED entry (audit-only), releasing pending holds, and
updating reservation status to "voided"
available balance reflects the units being committed
double-commit/void prevention, cross-state rejection, mutex lifecycle
OCC, sequence number gap-freedom, supply invariant under mixed scenarios
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Tests