Conversation
Contributor
|
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. 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds a Binding Documents feature: docs, JSON Schema, types, BindingDocumentService (create/read/sign), GraphQL schema+resolvers, tests, DB serialization tweak, dev-sandbox UI changes, and ecurrency group-overdraft support (entity, migration, services, controllers, client). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQL as GraphQL Server
participant BDS as BindingDocumentService
participant DB as DbService
participant Neo4j
Client->>GraphQL: createBindingDocument(input, X-ENAME)
GraphQL->>BDS: createBindingDocument(input, eName)
BDS->>BDS: normalizeSubject & validate payload
BDS->>DB: storeMetaEnvelope(ontology=BINDING_DOCUMENT_ONTOLOGY, value)
DB->>Neo4j: CREATE metaEnvelope node
Neo4j-->>DB: metaEnvelopeId
DB-->>BDS: stored envelope
BDS-->>GraphQL: { id, bindingDocument }
GraphQL-->>Client: CreateBindingDocumentPayload
sequenceDiagram
participant Client
participant GraphQL as GraphQL Server
participant BDS as BindingDocumentService
participant DB as DbService
participant Neo4j
Client->>GraphQL: createBindingDocumentSignature(metaEnvelopeId, signature, X-ENAME)
GraphQL->>BDS: addCounterpartySignature(input, eName)
BDS->>DB: getMetaEnvelopeById(metaEnvelopeId)
DB->>Neo4j: MATCH envelope
Neo4j-->>DB: envelope data
DB-->>BDS: bindingDocument
BDS->>BDS: validate signer rules, append signature
BDS->>DB: updateMetaEnvelopeById(metaEnvelopeId, updatedValue)
DB->>Neo4j: UPDATE envelope
Neo4j-->>DB: confirmation
DB-->>BDS: updated envelope
BDS-->>GraphQL: bindingDocument
GraphQL-->>Client: CreateBindingDocumentSignaturePayload
sequenceDiagram
participant Client
participant GraphQL as GraphQL Server
participant BDS as BindingDocumentService
participant DB as DbService
participant Neo4j
Client->>GraphQL: bindingDocuments(type?, pagination, X-ENAME)
GraphQL->>BDS: findBindingDocuments(eName, options)
BDS->>DB: findMetaEnvelopesByOntology(BINDING_DOCUMENT_ONTOLOGY, pagination)
DB->>Neo4j: MATCH binding-document envelopes
Neo4j-->>DB: MetaEnvelopeConnection
DB-->>BDS: connection
BDS->>BDS: filter by type (if provided)
BDS-->>GraphQL: MetaEnvelopeConnection<BindingDocument>
GraphQL-->>Client: bindingDocuments result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 5❌ Failed checks (4 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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: 12
🧹 Nitpick comments (5)
infrastructure/evault-core/src/core/db/schema.ts (1)
91-101: Deserialization fallback is sound; minor comment inaccuracy.The
try/catchcorrectly handles both non-JSON strings and non-string inputs (e.g. a raw JS array, which coerces to"a,b"and throws). The identity fallback on line 100 is the right behavior in both cases.The comment
// Not JSON, return as-is(line 98) is slightly inaccurate — the fallthrough also fires whenJSON.parsesucceeds but returns a non-array (e.g.JSON.parse("123")→123). Consider:// Not a JSON array; use schema deserializer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/core/db/schema.ts` around lines 91 - 101, Comment text is slightly inaccurate: the catch block and fallthrough cover both non-JSON input and JSON that is not an array. Update the inline comment inside the array deserialization branch (the block checking if type === "array") to accurately state that non-array JSON and non-JSON inputs should fall through to the schema deserializer; specifically replace "// Not JSON, return as-is" with something like "// Not a JSON array; use schema deserializer" so it matches the behavior of JSON.parse + schemaType.deserialize.infrastructure/evault-core/src/services/BindingDocumentService.spec.ts (1)
30-34: Remove the redundantdriver.close()call.
DbService.close()already callsdriver.close()internally, so calling it again on line 32 is unnecessary. Since the driver lifecycle is managed by the service, close it only throughdbService.close().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/BindingDocumentService.spec.ts` around lines 30 - 34, The afterAll teardown currently calls driver.close() redundantly; remove the explicit driver.close() call and rely on DbService.close() to manage the driver lifecycle. Edit the afterAll block that invokes dbService.close(), driver.close(), and container.stop() so it only awaits dbService.close() and container.stop(), leaving dbService.close() as the single place that closes the driver.infrastructure/evault-core/src/core/protocol/typedefs.ts (2)
144-149: GraphQL enum values usesnake_caseinstead of the conventionalSCREAMING_SNAKE_CASE.GraphQL convention is to use uppercase enum values (e.g.,
ID_DOCUMENT,PHOTOGRAPH,SOCIAL_CONNECTION,SELF). The current lowercase values work but may surprise API consumers. If you keep them lowercase, ensure the mapping between GraphQL enum values and the TypeScriptBindingDocumentTypestring literals is consistent throughout (which it currently is).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/core/protocol/typedefs.ts` around lines 144 - 149, The GraphQL enum BindingDocumentType currently uses snake_case values (id_document, photograph, social_connection, self); change these to SCREAMING_SNAKE_CASE (ID_DOCUMENT, PHOTOGRAPH, SOCIAL_CONNECTION, SELF) to follow GraphQL conventions, and update any corresponding TypeScript mappings or string literal uses that reference BindingDocumentType to match the new enum names (search for BindingDocumentType and any string compares/usages to adjust them consistently).
184-199:bindingDocumentsquery returnsMetaEnvelopeConnection— return type mismatch with domain intent.The query is documented as retrieving binding documents, but it returns
MetaEnvelopeConnection, which wrapsMetaEnvelopenodes (containingparsed: JSON). A client expecting typedBindingDocumentobjects would instead receive genericMetaEnvelopeenvelopes and need to dig intoparsedfor the binding document data.Consider introducing a
BindingDocumentEdgeandBindingDocumentConnectiontype for a cleaner API surface, or at minimum document that the binding document payload is innode.parsed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/core/protocol/typedefs.ts` around lines 184 - 199, The bindingDocuments query currently returns MetaEnvelopeConnection (wrapping MetaEnvelope nodes with parsed: JSON) which mismatches the documented intent; introduce a proper GraphQL connection by adding BindingDocumentEdge and BindingDocumentConnection types (edges.node should be BindingDocument), change the bindingDocuments return type from MetaEnvelopeConnection to BindingDocumentConnection, and update the resolver that builds the connection (where MetaEnvelope nodes are produced) to map each MetaEnvelope.parsed into a BindingDocument node (or wrap parsed into the new edge.node) so clients receive typed BindingDocument objects; alternatively, if you cannot change types now, update the bindingDocuments documentation to state that the payload lives in node.parsed (MetaEnvelope.parsed) and adjust resolvers to ensure parsed contains a valid BindingDocument shape.infrastructure/evault-core/src/core/protocol/graphql-server.ts (1)
1201-1204: Redundant case-insensitive header lookup.Per the Fetch API spec,
Headers.get()is already case-insensitive, sorequest.headers.get("x-ename")andrequest.headers.get("X-ENAME")will return the same value. The fallback on line 1203 is unnecessary but harmless.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/core/protocol/graphql-server.ts` around lines 1201 - 1204, The code performs a redundant case-insensitive header lookup for eName; replace the two calls with a single call to request.headers.get("x-ename") (or "X-ENAME") and keep the null fallback if needed so eName is set from request.headers.get(...) only; update the assignment to use the single get call (referencing the eName variable and request.headers.get) and remove the duplicate header.get() invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/W3DS` Basics/Binding-Documents.md:
- Around line 37-106: The docs claim to close issue `#792` but omit several
required types and behaviors: add documentation entries and notes for the
missing PASSPHRASE binding type (describe that passphrases are non-readable and
stored in a Protected Zone partition, only comparable via Protected Zone
functions), update the social_connection (aka FRIEND) entry to state that FRIEND
bindings are invalid until countersigned by the subject and document the
countersign workflow (include the required counter-signature field and
validation step), add a canonical revocation field and explain revocation status
semantics for all binding types, and document audit logging for binding document
operations (writes, countersigns, revocations, overrides) clarifying whether
these use existing /logs infrastructure or additional audit events; if this PR
intentionally covers only a subset, explicitly state the scope and leave issue
`#792` open.
In `@docs/docs/W3DS` Basics/glossary.md:
- Line 207: The example eName literal "@50e8400-e29b-41d4-a716-446655440000" is
a malformed UUID (first segment has 7 hex chars); update the example in
glossary.md to a valid RFC 4122 UUID by making the first segment 8 hex
characters (for example add one hex digit to the first group to produce
8-4-4-4-12 total), and verify the final eName follows the @<UUID> pattern and
RFC 4122 format.
In `@infrastructure/evault-core/src/core/protocol/graphql-server.ts`:
- Around line 855-917: The createBindingDocumentSignature resolver (the
middleware-wrapped async function named createBindingDocumentSignature)
currently calls bindingDocumentService.addCounterpartySignature (which
internally calls updateMetaEnvelopeById) but does not emit an audit event or
trigger webhook delivery; update the resolver to emit the same audit logging and
webhook call flow used by the analogous createBindingDocument/updateMetaEnvelope
resolvers: after a successful addCounterpartySignature result, call the
audit/event emitter with appropriate action (e.g., "COUNTERSIGN" or
"ADD_SIGNATURE") including context.eName, metaEnvelopeId
(input.bindingDocumentId) and signature metadata, and ensure webhook dispatch is
invoked with the resulting bindingDocument payload before returning to the
client; keep existing error handling and return shape intact.
- Around line 784-853: The createBindingDocument resolver currently skips audit
logging and webhook delivery and uses an unsafe cast for input.type; update the
resolver (createBindingDocument in graphql-server.ts) to validate input.type
against the BindingDocumentType enum (reject or map invalid values instead of
using "as any"), then after a successful
this.bindingDocumentService.createBindingDocument call call the existing audit
helper (appendEnvelopeOperationLog) with the created metaEnvelopeId and
appropriate operation type and payload, and invoke the existing webhook
dispatcher (deliverEnvelopeWebhooks or the project's webhook delivery method)
before returning the response; ensure the final return happens only after
audit/webhook succeed (or handle/log failures but still return the created
binding document), and include error handling/logging for the audit/webhook
steps so failures don’t crash the resolver.
In `@infrastructure/evault-core/src/core/types/binding-document.ts`:
- Around line 31-35: BindingDocumentData's union contains
BindingDocumentSocialConnectionData and BindingDocumentSelfData which are
structurally identical, so add a discriminant to safely narrow variants: update
the interfaces BindingDocumentSocialConnectionData and BindingDocumentSelfData
to include a literal property (e.g., kind: 'social_connection' | 'self') or
ensure callers always switch on BindingDocument.type (the parent tag) when
narrowing; then update any type guards or switch statements that handle
BindingDocumentData to check the new literal discriminant (or
BindingDocument.type) so the union becomes a proper tagged union.
- Around line 37-42: Add creation timestamp and revocation status to the
BindingDocument interface by adding createdAt: string (ISO timestamp) and a
revocation field such as revoked: boolean plus optional revokedAt?: string;
update the MetaEnvelope layer to carry these fields through so
getBindingDocument returns them in the payload, and modify
BindingDocumentService methods (creation and revocation/update paths in
BindingDocumentService) to populate createdAt on create and set
revoked/revokedAt when documents are revoked or updated. Ensure types are
exported/updated where BindingDocument is used so consumers receive the new
fields.
In `@infrastructure/evault-core/src/services/BindingDocumentService.spec.ts`:
- Around line 294-359: Add assertions in the BindingDocumentService.spec tests
to verify audit log entries are emitted when creating and modifying binding
documents: after calling bindingDocumentService.createBindingDocument (and after
bindingDocumentService.addCounterpartySignature if present in tests) query the
DbService.getEnvelopeOperationLogs (or the store/DbService wrapper that exposes
logs) or use appendEnvelopeOperationLog hooks to confirm at least one log entry
exists for the envelope/enName; specifically, call getEnvelopeOperationLogs for
the created envelope id or TEST_ENAME and assert result.length > 0 and that an
entry references the operation (e.g., "create" or the counterparty action) so
tests cover appendEnvelopeOperationLog/getEnvelopeOperationLogs behavior tied to
createBindingDocument and addCounterpartySignature.
- Around line 351-358: The test currently uses a for-loop over result.edges
which will pass silently if edges is empty; add an explicit assertion that
result.edges is not empty (e.g., expect(result.edges.length).toBeGreaterThan(0)
or expect(result.totalCount).toBeGreaterThan(0)) immediately after calling
bindingDocumentService.findBindingDocuments(TEST_ENAME, { type: "id_document" })
and before the for-loop, then keep the existing loop that asserts each
edge.node.parsed?.type === "id_document".
In `@infrastructure/evault-core/src/services/BindingDocumentService.ts`:
- Around line 119-155: findBindingDocuments currently filters by parsed?.type
in-memory after calling this.db.findMetaEnvelopesPaginated, which breaks
Relay-style pagination (wrong page size, pageInfo and totalCount). Fix by
pushing the type filter into the DB query: update the call to
this.db.findMetaEnvelopesPaginated in findBindingDocuments to include a filter
that matches BindingDocument parsed.type (or the payload field containing type)
together with ontologyId (BINDING_DOCUMENT_ONTOLOGY) so the DB returns correctly
paginated results and accurate pageInfo/totalCount; remove the post-query
edges.filter and derived totalCount so the method returns the DB’s pagination
metadata unchanged.
- Around line 32-59: In createBindingDocument, validate that input.data matches
the declared input.type before calling db.storeMetaEnvelope: add a runtime type
check (e.g., a switch on input.type using type-guard/helper validators or a
schema library) that verifies the shape for each allowed type (e.g.,
"id_document" vs "self_data"), reuse normalizeSubject as needed but perform
validation after it and before constructing BindingDocument; if the data does
not match, throw a clear error (e.g., ValidationError) and do not call
storeMetaEnvelope. Ensure the validation logic is located/coordinated with the
BindingDocument type definitions and referenced symbols (createBindingDocument,
BindingDocument, BINDING_DOCUMENT_ONTOLOGY) so future types can be added
consistently.
- Around line 61-97: The addCounterpartySignature method must enforce that the
incoming signature is from the expected counterparty and prevent duplicate
signatures: after loading metaEnvelope and casting to BindingDocument, verify
the document is BINDING_DOCUMENT_ONTOLOGY and then (for FRIEND-type
bindings—e.g., check bindingDocument.relation or role field) assert
input.signature.signer === bindingDocument.subject (or the expected counterparty
field) before proceeding; also check bindingDocument.signatures for an existing
signature by the same signer and throw if found to avoid duplicates; only after
those checks build updatedBindingDocument (appending the signature),
updateMetaEnvelopeById and return it. Ensure you reference
AddCounterpartySignatureInput, BindingDocument, signatures, subject,
addCounterpartySignature and BINDING_DOCUMENT_ONTOLOGY when locating the code to
change.
In `@services/ontology/schemas/binding-document.json`:
- Around line 19-32: The current "data" schema uses oneOf with references to
SocialConnectionData and SelfData which are structurally identical, causing
oneOf to fail; fix by replacing the oneOf with conditional validation based on
the top-level "type": remove the oneOf in the "data" property and add a
top-level allOf with if/then blocks that check { "properties": { "type": {
"const": "social_connection" } } } then validate data against
"#/definitions/SocialConnectionData", and similarly an if for "self" that
validates against "#/definitions/SelfData"; alternatively, if these types are
truly identical, merge SelfData into SocialConnectionData or add a
discriminating field (e.g., "kind") to each definition and use
discriminator/oneOf instead.
---
Nitpick comments:
In `@infrastructure/evault-core/src/core/db/schema.ts`:
- Around line 91-101: Comment text is slightly inaccurate: the catch block and
fallthrough cover both non-JSON input and JSON that is not an array. Update the
inline comment inside the array deserialization branch (the block checking if
type === "array") to accurately state that non-array JSON and non-JSON inputs
should fall through to the schema deserializer; specifically replace "// Not
JSON, return as-is" with something like "// Not a JSON array; use schema
deserializer" so it matches the behavior of JSON.parse + schemaType.deserialize.
In `@infrastructure/evault-core/src/core/protocol/graphql-server.ts`:
- Around line 1201-1204: The code performs a redundant case-insensitive header
lookup for eName; replace the two calls with a single call to
request.headers.get("x-ename") (or "X-ENAME") and keep the null fallback if
needed so eName is set from request.headers.get(...) only; update the assignment
to use the single get call (referencing the eName variable and
request.headers.get) and remove the duplicate header.get() invocation.
In `@infrastructure/evault-core/src/core/protocol/typedefs.ts`:
- Around line 144-149: The GraphQL enum BindingDocumentType currently uses
snake_case values (id_document, photograph, social_connection, self); change
these to SCREAMING_SNAKE_CASE (ID_DOCUMENT, PHOTOGRAPH, SOCIAL_CONNECTION, SELF)
to follow GraphQL conventions, and update any corresponding TypeScript mappings
or string literal uses that reference BindingDocumentType to match the new enum
names (search for BindingDocumentType and any string compares/usages to adjust
them consistently).
- Around line 184-199: The bindingDocuments query currently returns
MetaEnvelopeConnection (wrapping MetaEnvelope nodes with parsed: JSON) which
mismatches the documented intent; introduce a proper GraphQL connection by
adding BindingDocumentEdge and BindingDocumentConnection types (edges.node
should be BindingDocument), change the bindingDocuments return type from
MetaEnvelopeConnection to BindingDocumentConnection, and update the resolver
that builds the connection (where MetaEnvelope nodes are produced) to map each
MetaEnvelope.parsed into a BindingDocument node (or wrap parsed into the new
edge.node) so clients receive typed BindingDocument objects; alternatively, if
you cannot change types now, update the bindingDocuments documentation to state
that the payload lives in node.parsed (MetaEnvelope.parsed) and adjust resolvers
to ensure parsed contains a valid BindingDocument shape.
In `@infrastructure/evault-core/src/services/BindingDocumentService.spec.ts`:
- Around line 30-34: The afterAll teardown currently calls driver.close()
redundantly; remove the explicit driver.close() call and rely on
DbService.close() to manage the driver lifecycle. Edit the afterAll block that
invokes dbService.close(), driver.close(), and container.stop() so it only
awaits dbService.close() and container.stop(), leaving dbService.close() as the
single place that closes the driver.
* fix: correct envDir path in Vite configuration * feat: add allowNegativeGroupOnly feature to currency management * feat: implement overdraft validation for group members and enhance negative balance checks
* feat: evault inspector * feat: sandbox config
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platforms/ecurrency/api/src/controllers/CurrencyController.ts (2)
209-220:⚠️ Potential issue | 🟡 Minor
updateMaxNegativeBalanceresponse omitsallowNegativeGroupOnly.All other currency response shapes include this field. The omission here creates an inconsistent API contract.
Proposed fix
res.status(200).json({ id: updated.id, name: updated.name, description: updated.description, ename: updated.ename, groupId: updated.groupId, allowNegative: updated.allowNegative, + allowNegativeGroupOnly: updated.allowNegativeGroupOnly, maxNegativeBalance: updated.maxNegativeBalance, createdBy: updated.createdBy, createdAt: updated.createdAt, updatedAt: updated.updatedAt, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/ecurrency/api/src/controllers/CurrencyController.ts` around lines 209 - 220, The response object returned by updateMaxNegativeBalance is missing the allowNegativeGroupOnly field, causing an inconsistent currency response shape; update the response in CurrencyController (inside the updateMaxNegativeBalance handler where res.status(200).json({...}) is built) to include allowNegativeGroupOnly: updated.allowNegativeGroupOnly so it matches other currency endpoints.
69-75:⚠️ Potential issue | 🟠 MajorThe new cross-field validation error from
CurrencyServiceis swallowed as a 500.When
allowNegativeGroupOnlyistruebutallowNegativeisfalse, the service throws"Cannot restrict overdraft to group members when negative balances are disabled". This message doesn't match"Only group admins", so the catch block falls through to the generic 500 response. The client receives an opaque "Internal server error" instead of the actionable validation message.🐛 Proposed fix
} catch (error: any) { console.error("Error creating currency:", error); if (error.message.includes("Only group admins")) { return res.status(403).json({ error: error.message }); } + if (error.message.includes("Cannot restrict overdraft") || + error.message.includes("Cannot set max negative") || + error.message.includes("Max negative balance")) { + return res.status(400).json({ error: error.message }); + } res.status(500).json({ error: "Internal server error" }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/ecurrency/api/src/controllers/CurrencyController.ts` around lines 69 - 75, The catch in CurrencyController currently only special-cases "Only group admins" and returns 500 for other service validation errors; update the catch block in the create currency flow to detect the cross-field validation from CurrencyService (the "Cannot restrict overdraft to group members when negative balances are disabled" message or generally any validation error thrown by CurrencyService relating to allowNegativeGroupOnly/allowNegative) and return a 400 with the actual error.message to the client instead of a 500; keep the existing 403 handling for "Only group admins" and add an additional branch (or a generic validation branch) that checks error.message for the overdraft-restriction text or uses a ValidationError sentinel from CurrencyService and responds res.status(400).json({ error: error.message }).
♻️ Duplicate comments (1)
infrastructure/evault-core/src/services/BindingDocumentService.spec.ts (1)
189-232: Audit log test manually emits the log rather than asserting the service emits it.The test calls
dbService.appendEnvelopeOperationLog(...)directly (Line 213) and then asserts it was persisted. This validates the DB infrastructure, not thatcreateBindingDocumentitself emits audit events. Per learnings, audit emission is out of scope for the initial implementation, so this is fine for now — but consider renaming or adding a comment to clarify this tests the log infrastructure, not automatic emission.Based on learnings: "Binding documents in the MetaState prototype project are initially implemented for 'dumb' read/write operations... Validation logic such as countersign enforcement, revocation checks, and audit event emission is out of scope for the initial binding document implementation and will be addressed separately."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/BindingDocumentService.spec.ts` around lines 189 - 232, Rename the test or add a clarifying comment so it no longer implies createBindingDocument emits audit logs; specifically update the test that calls createBindingDocument and then directly invokes dbService.appendEnvelopeOperationLog (the lines around dbService.appendEnvelopeOperationLog(...) and the assertions) to either rename the test to something like "should persist envelope operation logs via dbService" or insert a comment above the manual append explaining this verifies the DB logging infrastructure only and that audit emission by createBindingDocument is out of scope; keep the assertions that check logs but change the test name/comment to reference dbService and the log infrastructure rather than automatic emission from createBindingDocument.
🧹 Nitpick comments (6)
platforms/ecurrency/api/src/services/LedgerService.ts (1)
244-253:burnduplicates the negative-balance validation thatvalidateNegativeAllowancenow encapsulates.Consider reusing the helper here to keep a single source of truth for balance-floor enforcement.
♻️ Suggested refactor
- // Enforce bounds - if (!currency.allowNegative && currentBalance < amount) { - throw new Error("Insufficient balance. Negative balances are not allowed."); - } - - if (currency.allowNegative && currency.maxNegativeBalance !== null && currency.maxNegativeBalance !== undefined) { - const newBalance = currentBalance - amount; - if (newBalance < Number(currency.maxNegativeBalance)) { - throw new Error(`Insufficient balance. This currency allows negative balances down to ${currency.maxNegativeBalance}.`); - } - } + // Enforce bounds + this.validateNegativeAllowance(currency, currentBalance, amount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/ecurrency/api/src/services/LedgerService.ts` around lines 244 - 253, The burn method duplicates negative-balance enforcement already implemented in validateNegativeAllowance; remove the duplicated checks in LedgerService.burn and call validateNegativeAllowance(...) instead (pass the relevant currency, currentBalance, and amount or other expected parameters) so all balance-floor logic is centralized in validateNegativeAllowance; ensure any error messages remain consistent by relying on validateNegativeAllowance's errors.platforms/ecurrency/api/src/services/CurrencyService.ts (1)
20-28: Optional parameterdescriptionplaced before defaulted parameterallowNegativeGroupOnlyis fragile.Positional parameters with defaults/optional values in the middle of a signature are easy to misuse. Consider using an options object for the growing parameter list.
♻️ Suggested refactor: options object pattern
- async createCurrency( - name: string, - groupId: string, - createdBy: string, - allowNegative: boolean = false, - maxNegativeBalance: number | null = null, - description?: string, - allowNegativeGroupOnly: boolean = false - ): Promise<Currency> { + async createCurrency( + name: string, + groupId: string, + createdBy: string, + options: { + allowNegative?: boolean; + maxNegativeBalance?: number | null; + description?: string; + allowNegativeGroupOnly?: boolean; + } = {} + ): Promise<Currency> { + const { + allowNegative = false, + maxNegativeBalance = null, + description, + allowNegativeGroupOnly = false, + } = options;This avoids positional ambiguity and makes adding future parameters non-breaking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/ecurrency/api/src/services/CurrencyService.ts` around lines 20 - 28, The createCurrency signature is fragile due to optional/ defaulted parameters (description before allowNegativeGroupOnly); change it to accept a single options object (e.g., createCurrency(name: string, groupId: string, createdBy: string, options?: { allowNegative?: boolean; maxNegativeBalance?: number | null; description?: string; allowNegativeGroupOnly?: boolean }) and update internal references inside CurrencyService.createCurrency to read from options with defaults, then update all call sites and tests to pass an options object instead of positional args so adding future params is non-breaking; ensure exported types and JSDoc reflect the new options shape.platforms/ecurrency/client/client/src/components/currency/create-currency-modal.tsx (1)
181-213: Inconsistent indentation between the new checkbox block and the existing max-negative input.Lines 199–211 are indented differently from the enclosing
<div>at line 197. This appears to be a merge artifact. While it's purely cosmetic (JSX rendering is unaffected), it hurts readability.♻️ Re-indent the input block to match the new wrapper
<div> <label className="block text-sm font-medium mb-1">Max negative balance (absolute value)</label> - <input - type="number" - min={0} - max={MAX_NEGATIVE_LIMIT} - step={0.01} - value={maxNegativeInput} - onChange={(e) => setMaxNegativeInput(e.target.value)} - placeholder="Leave blank for no cap" - className="w-full px-3 py-2 border rounded-md" - /> - <p className="text-xs text-muted-foreground mt-1"> - Limit how far any account can go below zero (max {MAX_NEGATIVE_LIMIT.toLocaleString()}). - </p> - </div> + <input + type="number" + min={0} + max={MAX_NEGATIVE_LIMIT} + step={0.01} + value={maxNegativeInput} + onChange={(e) => setMaxNegativeInput(e.target.value)} + placeholder="Leave blank for no cap" + className="w-full px-3 py-2 border rounded-md" + /> + <p className="text-xs text-muted-foreground mt-1"> + Limit how far any account can go below zero (max {MAX_NEGATIVE_LIMIT.toLocaleString()}). + </p> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/ecurrency/client/client/src/components/currency/create-currency-modal.tsx` around lines 181 - 213, The JSX indentation for the checkbox block and the "Max negative balance" input is inconsistent (looks like a merge artifact); reformat the JSX so both child <div> blocks under the fragment are indented the same level and align with their surrounding wrapper, keeping the existing props and handlers (allowNegativeGroupOnly, setAllowNegativeGroupOnly, maxNegativeInput, setMaxNegativeInput, MAX_NEGATIVE_LIMIT) unchanged to improve readability.infrastructure/evault-core/src/services/BindingDocumentService.spec.ts (1)
293-347: Consider adding a test for duplicate-signer prevention.
addCounterpartySignaturenow guards against duplicate signatures from the same signer (as shown inBindingDocumentService.tsLines 154-162). There's no test asserting this throws when the same signer signs twice. Adding one would prevent regressions on this validation.Sketch
it("should throw when same signer signs twice", async () => { const created = await bindingDocumentService.createBindingDocument( { subject: "test-user-123", type: "self", data: { kind: "self", name: "Dup Test" }, ownerSignature: { signer: TEST_ENAME, signature: "owner-sig", timestamp: new Date().toISOString(), }, }, TEST_ENAME, ); await bindingDocumentService.addCounterpartySignature( { metaEnvelopeId: created.id, signature: { signer: "@counterparty", signature: "sig-1", timestamp: new Date().toISOString(), }, }, TEST_ENAME, ); await expect( bindingDocumentService.addCounterpartySignature( { metaEnvelopeId: created.id, signature: { signer: "@counterparty", signature: "sig-2", timestamp: new Date().toISOString(), }, }, TEST_ENAME, ), ).rejects.toThrow("has already signed"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/BindingDocumentService.spec.ts` around lines 293 - 347, Add a unit test to BindingDocumentService.spec.ts that verifies BindingDocumentService.addCounterpartySignature rejects when the same signer attempts to sign twice: create a binding via bindingDocumentService.createBindingDocument (include an ownerSignature), call bindingDocumentService.addCounterpartySignature once with signer "@counterparty", then call it a second time with the same signer and assert the promise rejects (e.g., expect(...).rejects.toThrow) with the duplicate-signature message produced by addCounterpartySignature (refer to BindingDocumentService.ts duplicate check around addCounterpartySignature).infrastructure/evault-core/src/core/protocol/graphql-server.ts (1)
194-220:args.type as anybypasses type safety — unlike the mutation, the query doesn't validate the type.
createBindingDocument(Line 824) validatesinput.typeagainstVALID_BINDING_DOCUMENT_TYPESbefore proceeding, but thebindingDocumentsquery passes it through unchecked at Line 212. While an invalid type simply yields no results (no crash), adding the same validation here would give the caller a clear error rather than an empty result set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/core/protocol/graphql-server.ts` around lines 194 - 220, The bindingDocuments query currently forwards args.type unchecked to bindingDocumentService.findBindingDocuments; mirror the mutation validation by checking args.type against VALID_BINDING_DOCUMENT_TYPES (the same check used in createBindingDocument) and throw a clear Error when the type is invalid, then pass the validated/typed value to findBindingDocuments so callers receive an explicit error instead of an empty result set.platforms/ecurrency/client/client/src/components/currency/transfer-modal.tsx (1)
245-251: Conditional messaging looks good, but client-side validation doesn't account for group-only restriction.The text branching is correct. However, when
allowNegativeGroupOnlyis true, the client-side balance check (Line 163) and the submit-button disabled logic (Line 374) still treat it as unrestrictedallowNegative, meaning a non-group-member could attempt a transfer that goes negative — the server would reject it, but the UX could be confusing (no insufficient-balance warning, then a server error).Consider gating the client-side balance skip on
allowNegativeGroupOnlyas well, or surfacing the server error more explicitly when a non-member is denied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/ecurrency/client/client/src/components/currency/transfer-modal.tsx` around lines 245 - 251, In TransferModal, the client-side balance validation and submit-button disable logic currently treat fromCurrencyData.allowNegative as global; change the checks so that when fromCurrencyData.allowNegativeGroupOnly is true you only bypass the insufficient-balance check if the current user is a group member (use the existing isGroupMember or membership flag used in this component), and update whatever computed flag controls the submit button (e.g., isSubmitDisabled/canSubmit) to account for allowNegativeGroupOnly instead of just allowNegative; also ensure handleSubmit surfaces the server rejection clearly (map server error to a user-visible message) when a non-member is blocked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/dev-sandbox/src/routes/`+page.svelte:
- Around line 928-936: The envelope-count header shows "1–0 of 0" when
selectedOntologyId is set but envelopes haven't loaded because pageOffset is 0
and envelopes.length is 0; update the Svelte template around the envelope-count
span (the block using selectedOntologyId, pageLoading, pageOffset, envelopes,
totalCount) to only render the "start–end of total" text when envelopes.length >
0 or pageLoading is true (e.g. wrap the current range display in {`#if`
envelopes.length > 0 || pageLoading} ... {:else} render a sensible alternative
like "0 of {totalCount}" or nothing) so you avoid showing "1–0 of 0" before
loadMetaEnvelopes() completes.
- Around line 254-274: The highlighted JSON is passed through {`@html`
highlightJson(...)} without escaping, enabling XSS if string values contain
HTML; modify highlightJson to first HTML-escape the input JSON string (escape &,
<, >, ", ', / or use a trusted escape utility) and then run the
regex/syntax-highlighting on the escaped text so any embedded tags are rendered
inert; update the function named highlightJson to perform that escape step at
the start (or call a new escapeHtml helper) before the replace logic so keys,
strings, booleans, nulls, and numbers are still wrapped but all original HTML is
neutralized.
In `@infrastructure/evault-core/src/core/protocol/graphql-server.ts`:
- Around line 853-873: The hardcoded ontology ID literal used in
computeEnvelopeHash and appendEnvelopeOperationLog (and other occurrences)
should be replaced with the shared constant BINDING_DOCUMENT_ONTOLOGY from
BindingDocumentService; update this file to import (or re-export)
BINDING_DOCUMENT_ONTOLOGY at the top and replace every inline
"b1d0a8c3-4e5f-6789-0abc-def012345678" occurrence (used in computeEnvelopeHash
calls and the ontology field passed to this.db.appendEnvelopeOperationLog) with
BINDING_DOCUMENT_ONTOLOGY so all code (e.g., where metaEnvelopeId, envelopeHash,
computeEnvelopeHash, and appendEnvelopeOperationLog are used) references the
single source of truth.
In `@infrastructure/evault-core/src/services/BindingDocumentService.ts`:
- Around line 1-15: Merge the two imports from "../core/db/types" into a single
import statement that brings in FindMetaEnvelopesPaginatedOptions and
MetaEnvelopeConnection alongside any other types, and change the local
BINDING_DOCUMENT_ONTOLOGY const to an exported symbol (export const
BINDING_DOCUMENT_ONTOLOGY = "...") so it becomes the single source of truth;
then update the other module that hardcodes the same UUID (graphql-server.ts) to
import this exported BINDING_DOCUMENT_ONTOLOGY instead of duplicating the
string.
In `@platforms/ecurrency/client/vite.config.ts`:
- Line 6: Remove the debug console.log that prints envDir—specifically delete
the console.log("🔍 Vite envDir:", envDir); statement in the vite config so it
no longer logs on every vite dev/build; if you need conditional debugging later,
wrap logging behind a boolean flag or check an env var (e.g.,
process.env.DEBUG_VITE) rather than leaving an unconditional console.log in the
config.
---
Outside diff comments:
In `@platforms/ecurrency/api/src/controllers/CurrencyController.ts`:
- Around line 209-220: The response object returned by updateMaxNegativeBalance
is missing the allowNegativeGroupOnly field, causing an inconsistent currency
response shape; update the response in CurrencyController (inside the
updateMaxNegativeBalance handler where res.status(200).json({...}) is built) to
include allowNegativeGroupOnly: updated.allowNegativeGroupOnly so it matches
other currency endpoints.
- Around line 69-75: The catch in CurrencyController currently only
special-cases "Only group admins" and returns 500 for other service validation
errors; update the catch block in the create currency flow to detect the
cross-field validation from CurrencyService (the "Cannot restrict overdraft to
group members when negative balances are disabled" message or generally any
validation error thrown by CurrencyService relating to
allowNegativeGroupOnly/allowNegative) and return a 400 with the actual
error.message to the client instead of a 500; keep the existing 403 handling for
"Only group admins" and add an additional branch (or a generic validation
branch) that checks error.message for the overdraft-restriction text or uses a
ValidationError sentinel from CurrencyService and responds
res.status(400).json({ error: error.message }).
---
Duplicate comments:
In `@infrastructure/evault-core/src/services/BindingDocumentService.spec.ts`:
- Around line 189-232: Rename the test or add a clarifying comment so it no
longer implies createBindingDocument emits audit logs; specifically update the
test that calls createBindingDocument and then directly invokes
dbService.appendEnvelopeOperationLog (the lines around
dbService.appendEnvelopeOperationLog(...) and the assertions) to either rename
the test to something like "should persist envelope operation logs via
dbService" or insert a comment above the manual append explaining this verifies
the DB logging infrastructure only and that audit emission by
createBindingDocument is out of scope; keep the assertions that check logs but
change the test name/comment to reference dbService and the log infrastructure
rather than automatic emission from createBindingDocument.
---
Nitpick comments:
In `@infrastructure/evault-core/src/core/protocol/graphql-server.ts`:
- Around line 194-220: The bindingDocuments query currently forwards args.type
unchecked to bindingDocumentService.findBindingDocuments; mirror the mutation
validation by checking args.type against VALID_BINDING_DOCUMENT_TYPES (the same
check used in createBindingDocument) and throw a clear Error when the type is
invalid, then pass the validated/typed value to findBindingDocuments so callers
receive an explicit error instead of an empty result set.
In `@infrastructure/evault-core/src/services/BindingDocumentService.spec.ts`:
- Around line 293-347: Add a unit test to BindingDocumentService.spec.ts that
verifies BindingDocumentService.addCounterpartySignature rejects when the same
signer attempts to sign twice: create a binding via
bindingDocumentService.createBindingDocument (include an ownerSignature), call
bindingDocumentService.addCounterpartySignature once with signer
"@counterparty", then call it a second time with the same signer and assert the
promise rejects (e.g., expect(...).rejects.toThrow) with the duplicate-signature
message produced by addCounterpartySignature (refer to BindingDocumentService.ts
duplicate check around addCounterpartySignature).
In `@platforms/ecurrency/api/src/services/CurrencyService.ts`:
- Around line 20-28: The createCurrency signature is fragile due to optional/
defaulted parameters (description before allowNegativeGroupOnly); change it to
accept a single options object (e.g., createCurrency(name: string, groupId:
string, createdBy: string, options?: { allowNegative?: boolean;
maxNegativeBalance?: number | null; description?: string;
allowNegativeGroupOnly?: boolean }) and update internal references inside
CurrencyService.createCurrency to read from options with defaults, then update
all call sites and tests to pass an options object instead of positional args so
adding future params is non-breaking; ensure exported types and JSDoc reflect
the new options shape.
In `@platforms/ecurrency/api/src/services/LedgerService.ts`:
- Around line 244-253: The burn method duplicates negative-balance enforcement
already implemented in validateNegativeAllowance; remove the duplicated checks
in LedgerService.burn and call validateNegativeAllowance(...) instead (pass the
relevant currency, currentBalance, and amount or other expected parameters) so
all balance-floor logic is centralized in validateNegativeAllowance; ensure any
error messages remain consistent by relying on validateNegativeAllowance's
errors.
In
`@platforms/ecurrency/client/client/src/components/currency/create-currency-modal.tsx`:
- Around line 181-213: The JSX indentation for the checkbox block and the "Max
negative balance" input is inconsistent (looks like a merge artifact); reformat
the JSX so both child <div> blocks under the fragment are indented the same
level and align with their surrounding wrapper, keeping the existing props and
handlers (allowNegativeGroupOnly, setAllowNegativeGroupOnly, maxNegativeInput,
setMaxNegativeInput, MAX_NEGATIVE_LIMIT) unchanged to improve readability.
In
`@platforms/ecurrency/client/client/src/components/currency/transfer-modal.tsx`:
- Around line 245-251: In TransferModal, the client-side balance validation and
submit-button disable logic currently treat fromCurrencyData.allowNegative as
global; change the checks so that when fromCurrencyData.allowNegativeGroupOnly
is true you only bypass the insufficient-balance check if the current user is a
group member (use the existing isGroupMember or membership flag used in this
component), and update whatever computed flag controls the submit button (e.g.,
isSubmitDisabled/canSubmit) to account for allowNegativeGroupOnly instead of
just allowNegative; also ensure handleSubmit surfaces the server rejection
clearly (map server error to a user-visible message) when a non-member is
blocked.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.cursor/.gitignoreinfrastructure/dev-sandbox/src/routes/+page.svelteinfrastructure/evault-core/src/core/protocol/graphql-server.tsinfrastructure/evault-core/src/core/types/binding-document.tsinfrastructure/evault-core/src/services/BindingDocumentService.spec.tsinfrastructure/evault-core/src/services/BindingDocumentService.tsplatforms/ecurrency/api/src/controllers/CurrencyController.tsplatforms/ecurrency/api/src/controllers/LedgerController.tsplatforms/ecurrency/api/src/database/entities/Currency.tsplatforms/ecurrency/api/src/database/migrations/1771523000000-add-allow-negative-group-only.tsplatforms/ecurrency/api/src/services/CurrencyService.tsplatforms/ecurrency/api/src/services/GroupService.tsplatforms/ecurrency/api/src/services/LedgerService.tsplatforms/ecurrency/client/client/src/components/currency/create-currency-modal.tsxplatforms/ecurrency/client/client/src/components/currency/transfer-modal.tsxplatforms/ecurrency/client/client/src/pages/dashboard.tsxplatforms/ecurrency/client/vite.config.tsservices/ontology/schemas/binding-document.json
✅ Files skipped from review due to trivial changes (1)
- .cursor/.gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- services/ontology/schemas/binding-document.json
- infrastructure/evault-core/src/core/types/binding-document.ts
infrastructure/evault-core/src/services/BindingDocumentService.ts
Outdated
Show resolved
Hide resolved
sosweetham
approved these changes
Feb 23, 2026
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
3 participants
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.
Description of change
Issue Number
closes #792
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Documentation
UI