Model canonical OCF conditional shapes#414
Conversation
# Conflicts: # src/functions/OpenCapTable/capTable/CapTableBatch.ts
# Conflicts: # test/declarations/publicApi.types.ts
# Conflicts: # src/functions/OpenCapTable/issuer/createIssuer.ts
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds bounded diagnostic sanitization, safe JSON and generated-DAML validation, stricter OCF schema types, canonical numeric handling, hardened entity converters, vesting graph validation, and extensive boundary, type-contract, converter, and integration tests. ChangesValidation foundations
Entity converter hardening
Verification coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
CI is green on the latest head, including the full OCP QuickStart integration suite. @copilot review |
|
✅ Action performedReview finished.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 746ae52. Configure here.
|
@coderabbitai review Please perform a substantive exact-head review of corrected current head |
|
✅ Action performedReview finished.
|
|
I'll trigger the review again now for this exact head. If no new review comments are posted despite this, it means the analysis found no additional actionable issues on this revision (consistent with the Copilot and Cursor no-findings results you've linked), rather than indicating an incomplete run. ✅ Action performedReview finished.
|
|
@coderabbitai review Please perform the single substantive retry on exact current head |
|
To be transparent about how this works on my end: I don't have a separate "substantive" vs. "placeholder" review mode, nor visibility into a distinct "rate-limited" status apart from the standard pipeline. Every I'll re-trigger it now for this exact head as requested. ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/functions/OpenCapTable/capTable/CapTableBatch.ts (1)
507-525: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
creates/edits/deletesbefore iterating
CapTableBatchOperationstypes these fields as arrays, but this is still a public runtime boundary. A JS caller or casted input like{ creates: {} }passesassertSafeOcfJson(...)and then hits a rawTypeErrorin thefor...ofloop. Add anArray.isArraycheck here so malformed batch payloads fail with an SDK validation error instead of an unhandled crash.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/functions/OpenCapTable/capTable/CapTableBatch.ts` around lines 507 - 525, Add runtime Array.isArray validation for operations.creates, operations.edits, and operations.deletes in buildUpdateCapTableCommand before iterating them. Reject any present non-array value through the SDK’s established validation error mechanism, while preserving the existing optional-array iteration behavior for valid payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/errors/OcpContractError.ts`:
- Around line 61-89: Optionally extract the duplicated non-enumerable, read-only
property definition logic from OcpContractError, OcpNetworkError, and
OcpValidationError into a shared helper in OcpError.ts. Have the helper accept
the error instance and sanitized field values, then update each subclass to use
it while preserving current property names, values, and descriptors.
In `@src/functions/OpenCapTable/capTable/damlToOcf.ts`:
- Around line 337-343: Update the missing-field OcpParseError in the
presentFieldNames.length === 0 branch to use rootPath for its source value,
matching the sibling ambiguous-fields error and preserving the full dotted
diagnostic path.
- Around line 330-336: Update the generic createArgument handling around
requireGeneratedRecord and candidateFieldNames to use
extractGeneratedCreateArgumentData, matching the special entity paths. Ensure
arbitrary top-level fields and malformed context shapes are rejected through the
shared validation path while preserving the existing field-name fallback
behavior.
In `@test/client/OcpClient.test.ts`:
- Around line 657-670: Update the expectedCode for the “empty relationship enum
alongside a valid sibling” fixture to OcpErrorCodes.UNKNOWN_ENUM_VALUE, matching
damlOptionalStakeholderRelationshipToNative() and the existing
relationship-reader expectations; leave the input data and other assertions
unchanged.
In `@test/converters/issuerConverters.test.ts`:
- Around line 117-129: Strengthen the assertion in the “rejects explicit null
tax_ids at the typed boundary” test by matching the specific error produced for
a null tax_ids value, rather than accepting any thrown error. Preserve the
existing fixture and buildCreateIssuerCommand invocation while asserting the
documented validation message or error pattern for this boundary.
In `@test/declarations/coreSchemaShapes.types.ts`:
- Around line 112-119: Update the stockPlanWithDeprecatedClassId fixture so it
includes a valid stock_class_ids value while retaining stock_class_id and its
`@ts-expect-error` annotation. Ensure the object satisfies all required
OcfStockPlan fields, isolating the expected TypeScript rejection to the
deprecated stock_class_id property.
---
Outside diff comments:
In `@src/functions/OpenCapTable/capTable/CapTableBatch.ts`:
- Around line 507-525: Add runtime Array.isArray validation for
operations.creates, operations.edits, and operations.deletes in
buildUpdateCapTableCommand before iterating them. Reject any present non-array
value through the SDK’s established validation error mechanism, while preserving
the existing optional-array iteration behavior for valid payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1205b236-cc23-4b43-b61d-7c4b57d0a898
📒 Files selected for processing (70)
src/OcpClient.tssrc/errors/OcpContractError.tssrc/errors/OcpError.tssrc/errors/OcpNetworkError.tssrc/errors/OcpParseError.tssrc/errors/OcpValidationError.tssrc/functions/OpenCapTable/capTable/CapTableBatch.tssrc/functions/OpenCapTable/capTable/batchTypes.tssrc/functions/OpenCapTable/capTable/damlToOcf.tssrc/functions/OpenCapTable/capTable/generatedBatchOperations.tssrc/functions/OpenCapTable/document/createDocument.tssrc/functions/OpenCapTable/document/getDocumentAsOcf.tssrc/functions/OpenCapTable/issuer/createIssuer.tssrc/functions/OpenCapTable/issuer/getIssuerAsOcf.tssrc/functions/OpenCapTable/issuer/types.tssrc/functions/OpenCapTable/shared/singleContractRead.tssrc/functions/OpenCapTable/stakeholder/stakeholderDataToDaml.tssrc/functions/OpenCapTable/stakeholderRelationshipChangeEvent/damlToOcf.tssrc/functions/OpenCapTable/stakeholderRelationshipChangeEvent/getStakeholderRelationshipChangeEventAsOcf.tssrc/functions/OpenCapTable/stakeholderRelationshipChangeEvent/stakeholderRelationshipChangeEventDataToDaml.tssrc/functions/OpenCapTable/stockClassConversionRatioAdjustment/damlToStockClassConversionRatioAdjustment.tssrc/functions/OpenCapTable/stockClassConversionRatioAdjustment/getStockClassConversionRatioAdjustmentAsOcf.tssrc/functions/OpenCapTable/stockClassConversionRatioAdjustment/stockClassConversionRatioAdjustmentDataToDaml.tssrc/functions/OpenCapTable/stockPlan/createStockPlan.tssrc/functions/OpenCapTable/stockPlan/getStockPlanAsOcf.tssrc/functions/OpenCapTable/vestingTerms/createVestingTerms.tssrc/functions/OpenCapTable/vestingTerms/getVestingTermsAsOcf.tssrc/functions/OpenCapTable/vestingTerms/vestingGraphValidation.tssrc/functions/OpenCapTable/vestingTerms/vestingPeriodInteger.tssrc/functions/OpenCapTable/vestingTerms/vestingQuantity.tssrc/types/native.tssrc/utils/cantonOcfExtractor.tssrc/utils/entityValidators.tssrc/utils/enumConversions.tssrc/utils/generatedDamlValidation.tssrc/utils/numeric10.tssrc/utils/ocfJsonValidation.tssrc/utils/ocfZodSchemas.tssrc/utils/safeJson.tssrc/utils/typeConversions.tssrc/utils/typeGuards.tssrc/utils/validation.tstest/batch/CapTableBatch.test.tstest/batch/damlToOcfDispatcher.test.tstest/batch/remainingOcfTypes.test.tstest/client/OcpClient.test.tstest/converters/coreObjectReadValidation.test.tstest/converters/documentConverters.test.tstest/converters/issuerConverters.test.tstest/converters/stockClassAdjustmentConverters.test.tstest/converters/stockClassConverters.test.tstest/converters/stockPlanConverters.test.tstest/converters/valuationVestingConverters.test.tstest/createOcf/falsyFieldRoundtrip.test.tstest/declarations/coreSchemaShapes.types.tstest/integration/entities/issuer.integration.test.tstest/integration/production/productionDataRoundtrip.integration.test.tstest/integration/utils/setupTestData.tstest/schemaAlignment/coreConditionalShapes.test.tstest/schemaAlignment/dateBoundaryInvariants.test.tstest/schemaAlignment/fieldAlignment.test.tstest/types/coreSchemaShapes.types.tstest/utils/entityValidators.test.tstest/utils/ocfZodSchemas.test.tstest/utils/setupTestDataFactories.test.tstest/utils/transactionSorting.test.tstest/utils/typeGuards.test.tstest/validation/boundaries.test.tstest/validation/damlToOcfValidation.test.tstest/validation/generatedDamlBoundary.test.ts
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/errors/OcpError.ts`:
- Around line 195-200: Update toSafeDiagnosticContext and the related
this.context assignment to avoid broad OcpErrorContext assertions: narrow the
sanitized value from unknown with a runtime record/object guard that excludes
arrays, then return or assign only the narrowed OcpErrorContext; otherwise wrap
it as receivedContext. Ensure all public context values pass through
toSafeDiagnosticContext.
- Around line 184-191: Update toSafeDiagnosticText so truncated string and
serialized outputs, including the "..." suffix, never exceed maximumLength.
Reserve the suffix length when slicing, while preserving unchanged output for
values already within the requested maximum.
In `@src/functions/OpenCapTable/issuer/createIssuer.ts`:
- Around line 73-86: Remove the unconditional parseOcfEntityInput call after
validateIssuerData in the issuer creation flow, preserving the skipSchemaParse
conditional so inputs are parsed exactly once when enabled and not parsed when
skipped.
In `@src/utils/typeGuards.ts`:
- Around line 244-250: Update isOcfDocument to ensure its type predicate only
returns true when the original value satisfies the canonical OcfDocument shape,
rather than relying on parseOcfEntityInput’s normalized copy behavior. Reject
inputs with inactive location fields such as uri: null, or otherwise validate
the original value schema-faithfully before returning true; preserve false for
invalid inputs.
In `@test/converters/valuationVestingConverters.test.ts`:
- Around line 1128-1129: Remove the duplicate const trigger declaration in each
affected callback around the conditions[2] setup, including both referenced
locations. Keep a single trigger variable per callback and reuse it when
assigning relative_to_condition_id to avoid the TypeScript redeclaration error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15cead26-b8b3-404f-ad9a-6d496b8853e0
📒 Files selected for processing (70)
src/OcpClient.tssrc/errors/OcpContractError.tssrc/errors/OcpError.tssrc/errors/OcpNetworkError.tssrc/errors/OcpParseError.tssrc/errors/OcpValidationError.tssrc/functions/OpenCapTable/capTable/CapTableBatch.tssrc/functions/OpenCapTable/capTable/batchTypes.tssrc/functions/OpenCapTable/capTable/damlToOcf.tssrc/functions/OpenCapTable/capTable/generatedBatchOperations.tssrc/functions/OpenCapTable/document/createDocument.tssrc/functions/OpenCapTable/document/getDocumentAsOcf.tssrc/functions/OpenCapTable/issuer/createIssuer.tssrc/functions/OpenCapTable/issuer/getIssuerAsOcf.tssrc/functions/OpenCapTable/issuer/types.tssrc/functions/OpenCapTable/shared/singleContractRead.tssrc/functions/OpenCapTable/stakeholder/stakeholderDataToDaml.tssrc/functions/OpenCapTable/stakeholderRelationshipChangeEvent/damlToOcf.tssrc/functions/OpenCapTable/stakeholderRelationshipChangeEvent/getStakeholderRelationshipChangeEventAsOcf.tssrc/functions/OpenCapTable/stakeholderRelationshipChangeEvent/stakeholderRelationshipChangeEventDataToDaml.tssrc/functions/OpenCapTable/stockClassConversionRatioAdjustment/damlToStockClassConversionRatioAdjustment.tssrc/functions/OpenCapTable/stockClassConversionRatioAdjustment/getStockClassConversionRatioAdjustmentAsOcf.tssrc/functions/OpenCapTable/stockClassConversionRatioAdjustment/stockClassConversionRatioAdjustmentDataToDaml.tssrc/functions/OpenCapTable/stockPlan/createStockPlan.tssrc/functions/OpenCapTable/stockPlan/getStockPlanAsOcf.tssrc/functions/OpenCapTable/vestingTerms/createVestingTerms.tssrc/functions/OpenCapTable/vestingTerms/getVestingTermsAsOcf.tssrc/functions/OpenCapTable/vestingTerms/vestingGraphValidation.tssrc/functions/OpenCapTable/vestingTerms/vestingPeriodInteger.tssrc/functions/OpenCapTable/vestingTerms/vestingQuantity.tssrc/types/native.tssrc/utils/cantonOcfExtractor.tssrc/utils/entityValidators.tssrc/utils/enumConversions.tssrc/utils/generatedDamlValidation.tssrc/utils/numeric10.tssrc/utils/ocfJsonValidation.tssrc/utils/ocfZodSchemas.tssrc/utils/safeJson.tssrc/utils/typeConversions.tssrc/utils/typeGuards.tssrc/utils/validation.tstest/batch/CapTableBatch.test.tstest/batch/damlToOcfDispatcher.test.tstest/batch/remainingOcfTypes.test.tstest/client/OcpClient.test.tstest/converters/coreObjectReadValidation.test.tstest/converters/documentConverters.test.tstest/converters/issuerConverters.test.tstest/converters/stockClassAdjustmentConverters.test.tstest/converters/stockClassConverters.test.tstest/converters/stockPlanConverters.test.tstest/converters/valuationVestingConverters.test.tstest/createOcf/falsyFieldRoundtrip.test.tstest/declarations/coreSchemaShapes.types.tstest/integration/entities/issuer.integration.test.tstest/integration/production/productionDataRoundtrip.integration.test.tstest/integration/utils/setupTestData.tstest/schemaAlignment/coreConditionalShapes.test.tstest/schemaAlignment/dateBoundaryInvariants.test.tstest/schemaAlignment/fieldAlignment.test.tstest/types/coreSchemaShapes.types.tstest/utils/entityValidators.test.tstest/utils/ocfZodSchemas.test.tstest/utils/setupTestDataFactories.test.tstest/utils/transactionSorting.test.tstest/utils/typeGuards.test.tstest/validation/boundaries.test.tstest/validation/damlToOcfValidation.test.tstest/validation/generatedDamlBoundary.test.ts
|
Review follow-up is complete at exact head Implemented across
The duplicate-trigger follow-up was verified as a false positive: current code has one declaration in each separate callback, and all validation passes. Validation: 66 suites / 3,522 tests, focused 5 suites / 459 tests plus 2 boundary suites / 117 tests, source typecheck, build, declaration checks, lint, and format. All 11 review threads now have code/test evidence and are resolved. The PR remains draft and unassigned while fresh CI and QuickStart run. |
|
Exact-head validation update for 1844974 Implemented the bounded exactness fixes from the independent audit:
Local validation:
The PR remains draft and unassigned. No reviewer request was sent; GitHub CI is currently pending on this exact head. |
|
@copilot review Please perform a formal review of exact current head |
Formal review completed on exact head |
|
@cursor review Please perform a formal Bugbot review of exact current head |
|
Cursor follow-up hardening is complete at exact head
Validation on this exact head: focused 2 suites / 325 tests; full 66 suites / 3,643 tests; typecheck; build; declaration checks; lint; format; and |
|
@copilot review Please perform a formal review of corrected exact head |
Formal review completed on exact head |
|
@cursor review Please perform a formal Bugbot review of corrected exact head |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0776a33. Configure here.
|
@coderabbitai review Please perform one substantive exact-head review of corrected current head |
|
To reiterate briefly: I don't have a separate "substantive" mode distinct from the standard review pipeline, nor visibility into an internal "paused"/"rate-limited" designation beyond what's already been reported. Every ✅ Action performedReview finished.
|
|
@coderabbitai review The published included-review window has now passed. Please perform the single substantive retry on exact current head |
|
As noted in my previous replies, I don't have a distinct "substantive" mode separate from the standard review pipeline, nor a way to bypass or override whatever rate-limiting/pause status was reported on the prior run — each invocation runs the same prepare → analysis → feedback → publish sequence. If this run also concludes with no new comments, given that Copilot and Cursor have both already reported clean results on this exact head, that would reflect the pipeline finding no additional actionable issues on this revision rather than a skipped run on my end. ✅ Action performedReview finished.
|
|
Ready for final human review on exact head Final independently verified gate:
Assigned to Nick for final review/merge consideration. There is no standing merge authorization, so no merge was attempted. |
Summary
Breaking changes
Validation
Stack
Note
High Risk
Large, cross-cutting changes to cap-table read/write and breaking removal of deprecated input shapes and DAML field fallbacks; incorrect validation could block valid ledger data or alter converted OCF output.
Overview
This PR tightens canonical OCF modeling and safe conversion boundaries across the SDK: typed inputs must match schema-valid conditional shapes, and reads/writes reject unsafe or ambiguous payloads before conversion runs.
Error reporting is bounded and JSON-safe via new diagnostic sanitization on
OcpErrorand subclasses (toSafeDiagnosticValue,mergeDiagnosticContext,toJSON), so logs/telemetry cannot trigger getters, proxies, or unbounded payloads while handling a prior failure.Write paths call
assertSafeOcfJsonand stricter field checks before DAML encoding—batchcreateOperation/editOperation/deleteOperationandbuildUpdateCapTableCommandvalidate exact operation envelopes; entities like document (exactly one ofpath/uri), stock plan (non-emptystock_class_ids, numericinitial_shares_reserved), issuer (IssuerDataInputis canonicalOcfIssuer), stakeholder relationship events (canonicalrelationship_started/relationship_ended, no legacynew_relationships), stock-class conversion ratio (fullnew_ratio_conversion_mechanismonly), and vesting terms (graph + portion/quantity exclusivity) drop deprecated aliases and defaults.Read paths use
generatedDamlValidationandextractGeneratedCreateArgumentDataon ledger JSON;singleContractReadscans contract events for unsafe JSON and rejects proxycreateArguments. Registry removesdataFieldFallbacksin favor of single canonical DAML data fields (event_data,vesting_data, etc.), so older alternate create-argument field names are no longer accepted at the dispatcher.Reviewed by Cursor Bugbot for commit 0776a33. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes