Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new postEntry pipeline module that centralizes ledger entry validation and posting logic, refactors mutations.ts to delegate to this pipeline and expose postEntryDirect as an internal mutation for testing, adds AUDIT_ONLY_ENTRY_TYPES constant for audit-only entries that skip cumulative updates, and includes comprehensive test coverage including a new postEntry.test.ts file and updates to existing tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Test Client
participant MutCtx as MutationCtx
participant Val as Validators
participant Accts as Accounts
participant SeqCtr as Sequence Counter
participant DB as Database
Client->>MutCtx: postEntry(ctx, args)
MutCtx->>Val: validateInput (amount, accounts)
Val-->>MutCtx: ✓ or Error
MutCtx->>DB: checkIdempotency (idempotencyKey)
DB-->>MutCtx: exists? or new
MutCtx->>Accts: resolveAccounts (debit, credit)
Accts->>DB: fetch account docs
DB-->>Accts: account data
Accts-->>MutCtx: accounts or ACCOUNT_NOT_FOUND
MutCtx->>Val: typeCheck (entryType, accounts, mortgage)
Val-->>MutCtx: ✓ or TYPE_MISMATCH/CORRECTION error
MutCtx->>Accts: balanceCheck (debit balance vs amount)
Accts-->>MutCtx: ✓ or INSUFFICIENT_BALANCE
MutCtx->>Val: constraintCheck (entry-type specific rules)
Val-->>MutCtx: ✓ or constraint violation
MutCtx->>SeqCtr: getNextSequenceNumber ()
SeqCtr->>DB: read sequence counter
DB-->>SeqCtr: current value
SeqCtr-->>MutCtx: next sequence
MutCtx->>DB: persist (update cumulative, insert journal entry)
DB-->>MutCtx: journal entry doc
MutCtx->>Client: return ledger_journal_entries doc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff spans multiple heterogeneous components: a new 593-line postEntry pipeline with intricate validation logic, a substantial ~290-line mutations.ts refactor with error mapping, a comprehensive 952-line test suite with diverse scenarios, and supporting documentation. While individual file changes follow consistent patterns, the logic density (particularly in postEntry.ts constraint checks, balance validation, and error handling), the breadth of affected test files, and the architectural shift from public to internal mutation exposure demand careful cross-component reasoning. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Greptile SummaryThis PR extracts the core ledger write path into a structured 9-step pipeline ( Key changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Caller (mutation)
participant PE as postEntry()
participant DB as Convex DB
Caller->>PE: postEntry(ctx, args)
PE->>PE: Step 1: validateInput()<br/>(INVALID_AMOUNT, SAME_ACCOUNT)
PE->>DB: Step 2: checkIdempotency()<br/>query by_idempotency
alt existing entry found
DB-->>PE: existing Doc
PE-->>Caller: return existing (no-op)
end
DB-->>PE: null
PE->>DB: Step 3: resolveAccounts()<br/>ctx.db.get(debitId, creditId)
DB-->>PE: debitAccount, creditAccount
PE->>PE: Step 4: typeCheck()<br/>(TYPE_MISMATCH, MORTGAGE_MISMATCH,<br/>CORRECTION_REQUIRES_CAUSED_BY*)
PE->>PE: Step 5: balanceCheck()<br/>(INSUFFICIENT_BALANCE)<br/>skipped for WORLD & AUDIT_ONLY
PE->>PE: Step 6: constraintCheck()<br/>per-entry-type strategy<br/>(INVALID_MINT_AMOUNT, MIN_FRACTION_VIOLATED,<br/>CORRECTION_REQUIRES_ADMIN, etc.)
PE->>DB: Step 7: getNextSequenceNumber()
DB-->>PE: sequenceNumber (bigint)
PE->>DB: Step 8: persist()<br/>patch cumulativeDebits/Credits<br/>(skipped for AUDIT_ONLY)<br/>insert ledger_journal_entries
DB-->>PE: entryId
PE->>DB: ctx.db.get(entryId)
DB-->>PE: Doc
PE->>PE: Step 9: nudge() [no-op]
PE-->>Caller: Doc<"ledger_journal_entries">
|
![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
This PR introduces a new centralized postEntry pipeline for the mortgage ownership ledger, refactors existing ledger mutations to route through that pipeline, and expands/updates tests to cover the new behavior and error model.
Changes:
- Added
convex/ledger/postEntry.tsimplementing a 9-step posting pipeline (validation, idempotency, balance/constraints, sequencing, persistence). - Refactored
convex/ledger/mutations.tsto remove the publicpostEntrymutation and exposepostEntryDirectas aninternalMutation, with convenience mutations calling the shared pipeline. - Updated constants/types/validators and added/updated tests to reflect the new pipeline and error handling.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-27/tasks.md | Adds ENG-27 master task checklist for the pipeline/refactor/test work. |
| specs/ENG-27/chunks/manifest.md | Adds chunk manifest for ENG-27 work breakdown. |
| specs/ENG-27/chunks/chunk-01-postentry-pipeline/tasks.md | Adds chunk 01 task checklist for the postEntry pipeline. |
| specs/ENG-27/chunks/chunk-01-postentry-pipeline/context.md | Adds detailed design/context for the 9-step pipeline and error codes. |
| specs/ENG-27/chunks/chunk-02-mutations-refactor/tasks.md | Adds chunk 02 task checklist for mutations refactor. |
| specs/ENG-27/chunks/chunk-02-mutations-refactor/context.md | Adds refactor plan/context for removing old helpers and wiring new pipeline. |
| specs/ENG-27/chunks/chunk-03-tests/tasks.md | Adds chunk 03 task checklist for test updates/new tests. |
| specs/ENG-27/chunks/chunk-03-tests/context.md | Adds testing guidance for internal mutation usage and ConvexError assertions. |
| convex/ledger/constants.ts | Renames supply/min constants and introduces AUDIT_ONLY_ENTRY_TYPES. |
| convex/ledger/types.ts | Introduces typed entry/account/source enums plus ENTRY_TYPE_ACCOUNT_MAP. |
| convex/ledger/validators.ts | Adds reservation/correction/reservation-phase validators. |
| convex/ledger/validation.ts | Updates supply invariant to use TOTAL_SUPPLY. |
| convex/ledger/postEntry.ts | New core pipeline implementation for posting ledger entries. |
| convex/ledger/mutations.ts | Refactors to call postEntry pipeline; adds postEntryDirect internal mutation; migrates errors to ConvexError. |
| convex/ledger/tests/postEntry.test.ts | Adds comprehensive pipeline-focused tests (happy paths, audit-only behavior, error codes, idempotency/sequence cases). |
| convex/ledger/tests/ledger.test.ts | Updates existing tests to call postEntryDirect and adjusts assertions for new error messages. |
| convex/ledger/tests/accounts.test.ts | Initializes sequence counter before minting in the affected test. |
| convex/demo/ledger.ts | Updates demo code to use TOTAL_SUPPLY. |
| convex/auth/tests/resourceChecks.test.ts | Fixes ledger sequence counter table naming in comments/describe blocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…path Implement the core postEntry pipeline in convex/ledger/postEntry.ts — the only code path that modifies accounts or inserts journal entries. Refactor mutations.ts to delegate all validation to the extracted pipeline. - Create 9-step pipeline: validate → idempotency → resolve → type check → balance check → constraint check → sequence → persist → nudge - All errors now structured ConvexError with specific codes - AUDIT_ONLY entry types (SHARES_RESERVED, SHARES_VOIDED) skip cumulative balance updates - Balance checks use available balance (posted - pendingCredits) - WORLD account exempt from balance constraints - Replace public postEntry with postEntryDirect internalMutation - 23 new pipeline tests covering all 9 entry types and rejection rules Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add source.actor check for CORRECTION (regression from old code) - Use getAvailableBalance for seller in transferShares constraint - Validate args.mortgageId matches account mortgageIds in typeCheck - Add explicit INSUFFICIENT_BALANCE check in SHARES_RESERVED before min position check - Keep CORRECTION preconditions (admin/causedBy/reason) in typeCheck to fire before balanceCheck, with correct ordering - Fix test regexes to match ConvexError message format - Add try/catch to getConvexErrorCode JSON.parse 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
convex/ledger/constants.ts (1)
23-32: LGTM with optional type improvement.The AUDIT_ONLY distinction correctly separates reservation tracking (journal entries only) from actual balance movements. SHARES_RESERVED and SHARES_VOIDED create audit trails without affecting cumulative balances, while SHARES_COMMITTED finalizes the transfer.
💡 Optional: Consider using EntryType for stronger type safety
Using
ReadonlySet<string>allows any string. For stricter compile-time checking:+import type { EntryType } from "./types"; + -export const AUDIT_ONLY_ENTRY_TYPES: ReadonlySet<string> = new Set([ +export const AUDIT_ONLY_ENTRY_TYPES: ReadonlySet<EntryType> = new Set<EntryType>([ "SHARES_RESERVED", "SHARES_VOIDED", -]); +] as const);This would catch typos at compile time if EntryType changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/ledger/constants.ts` around lines 23 - 32, AUDIT_ONLY_ENTRY_TYPES is currently typed as ReadonlySet<string>, which permits arbitrary strings; change its type to ReadonlySet<EntryType> to enforce compile-time safety (use the existing EntryType union/type in this module or import it if declared elsewhere), update the new Set initializer to be typed as ReadonlySet<EntryType> (or use a const assertion if needed) and ensure any usages still accept EntryType values; reference AUDIT_ONLY_ENTRY_TYPES and EntryType when making the change.
🤖 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__/postEntry.test.ts`:
- Around line 45-59: The function getConvexErrorCode should guard against
malformed JSON in e.data: wrap the JSON.parse(e.data) call in a try/catch inside
the branch that checks typeof data === "string" (within getConvexErrorCode) and
return an empty string on parse failure (or otherwise fall through to the
existing object/empty return) so a bad string doesn't throw and obscure test
failures; keep the existing ConvexError instanceof check and the subsequent
object branch unchanged.
---
Nitpick comments:
In `@convex/ledger/constants.ts`:
- Around line 23-32: AUDIT_ONLY_ENTRY_TYPES is currently typed as
ReadonlySet<string>, which permits arbitrary strings; change its type to
ReadonlySet<EntryType> to enforce compile-time safety (use the existing
EntryType union/type in this module or import it if declared elsewhere), update
the new Set initializer to be typed as ReadonlySet<EntryType> (or use a const
assertion if needed) and ensure any usages still accept EntryType values;
reference AUDIT_ONLY_ENTRY_TYPES and EntryType when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9abd916-307f-44a6-8aae-f9800a5c39fc
📒 Files selected for processing (14)
convex/ledger/__tests__/accounts.test.tsconvex/ledger/__tests__/ledger.test.tsconvex/ledger/__tests__/postEntry.test.tsconvex/ledger/constants.tsconvex/ledger/mutations.tsconvex/ledger/postEntry.tsspecs/ENG-27/chunks/chunk-01-postentry-pipeline/context.mdspecs/ENG-27/chunks/chunk-01-postentry-pipeline/tasks.mdspecs/ENG-27/chunks/chunk-02-mutations-refactor/context.mdspecs/ENG-27/chunks/chunk-02-mutations-refactor/tasks.mdspecs/ENG-27/chunks/chunk-03-tests/context.mdspecs/ENG-27/chunks/chunk-03-tests/tasks.mdspecs/ENG-27/chunks/manifest.mdspecs/ENG-27/tasks.md
Comment on lines
+45
to
+59
| function getConvexErrorCode(e: unknown): string { | ||
| expect(e).toBeInstanceOf(ConvexError); | ||
| if (!(e instanceof ConvexError)) { | ||
| throw new Error("Expected ConvexError"); | ||
| } | ||
| const data = e.data; | ||
| if (typeof data === "string") { | ||
| const parsed = JSON.parse(data) as { code?: string }; | ||
| return parsed.code ?? ""; | ||
| } | ||
| if (typeof data === "object" && data !== null) { | ||
| return (data as { code?: string }).code ?? ""; | ||
| } | ||
| return ""; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing try/catch around JSON.parse in getConvexErrorCode.
The PR description mentions "adds try/catch around JSON.parse for getConvexErrorCode", but the implementation lacks error handling. If e.data is a malformed string, JSON.parse will throw and obscure the actual test failure.
🛡️ Proposed fix to add error handling
function getConvexErrorCode(e: unknown): string {
expect(e).toBeInstanceOf(ConvexError);
if (!(e instanceof ConvexError)) {
throw new Error("Expected ConvexError");
}
const data = e.data;
if (typeof data === "string") {
- const parsed = JSON.parse(data) as { code?: string };
- return parsed.code ?? "";
+ try {
+ const parsed = JSON.parse(data) as { code?: string };
+ return parsed.code ?? "";
+ } catch {
+ return "";
+ }
}
if (typeof data === "object" && data !== null) {
return (data as { code?: string }).code ?? "";
}
return "";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function getConvexErrorCode(e: unknown): string { | |
| expect(e).toBeInstanceOf(ConvexError); | |
| if (!(e instanceof ConvexError)) { | |
| throw new Error("Expected ConvexError"); | |
| } | |
| const data = e.data; | |
| if (typeof data === "string") { | |
| const parsed = JSON.parse(data) as { code?: string }; | |
| return parsed.code ?? ""; | |
| } | |
| if (typeof data === "object" && data !== null) { | |
| return (data as { code?: string }).code ?? ""; | |
| } | |
| return ""; | |
| } | |
| function getConvexErrorCode(e: unknown): string { | |
| expect(e).toBeInstanceOf(ConvexError); | |
| if (!(e instanceof ConvexError)) { | |
| throw new Error("Expected ConvexError"); | |
| } | |
| const data = e.data; | |
| if (typeof data === "string") { | |
| try { | |
| const parsed = JSON.parse(data) as { code?: string }; | |
| return parsed.code ?? ""; | |
| } catch { | |
| return ""; | |
| } | |
| } | |
| if (typeof data === "object" && data !== null) { | |
| return (data as { code?: string }).code ?? ""; | |
| } | |
| return ""; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/ledger/__tests__/postEntry.test.ts` around lines 45 - 59, The function
getConvexErrorCode should guard against malformed JSON in e.data: wrap the
JSON.parse(e.data) call in a try/catch inside the branch that checks typeof data
=== "string" (within getConvexErrorCode) and return an empty string on parse
failure (or otherwise fall through to the existing object/empty return) so a bad
string doesn't throw and obscure test failures; keep the existing ConvexError
instanceof check and the subsequent object branch unchanged.
This was referenced Mar 17, 2026
Merged
Merged
Merged
Closed
Merged
Owner
Author
Merge activity
|
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
specs ft. ENG-27: Extract postEntry 9-step pipeline as single ledger write path Implement the core postEntry pipeline in convex/ledger/postEntry.ts — the only code path that modifies accounts or inserts journal entries. Refactor mutations.ts to delegate all validation to the extracted pipeline. - Create 9-step pipeline: validate → idempotency → resolve → type check → balance check → constraint check → sequence → persist → nudge - All errors now structured ConvexError with specific codes - AUDIT_ONLY entry types (SHARES_RESERVED, SHARES_VOIDED) skip cumulative balance updates - Balance checks use available balance (posted - pendingCredits) - WORLD account exempt from balance constraints - Replace public postEntry with postEntryDirect internalMutation - 23 new pipeline tests covering all 9 entry types and rejection rules 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 ## Release Notes * **New Features** * Added audit-only entry types (SHARES_RESERVED, SHARES_VOIDED) that create journal entries without affecting cumulative balance calculations. * Enhanced ledger entry validation with specific error codes for improved error handling. * **Tests** * Expanded test coverage for ledger entry operations, including validation scenarios, idempotency checks, and error handling cases. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
specs
ft. ENG-27: Extract postEntry 9-step pipeline as single ledger write path
Implement the core postEntry pipeline in convex/ledger/postEntry.ts — the
only code path that modifies accounts or inserts journal entries. Refactor
mutations.ts to delegate all validation to the extracted pipeline.
balance check → constraint check → sequence → persist → nudge
balance updates
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
Release Notes
New Features
Tests