Model canonical conversion mechanisms#420
Conversation
# Conflicts: # src/functions/OpenCapTable/convertibleConversion/getConvertibleConversionAsOcf.ts # src/functions/OpenCapTable/convertibleIssuance/createConvertibleIssuance.ts # src/functions/OpenCapTable/convertibleIssuance/getConvertibleIssuanceAsOcf.ts # src/functions/OpenCapTable/warrantIssuance/getWarrantIssuanceAsOcf.ts # src/utils/planSecurityAliases.ts # test/converters/convertibleIssuanceConverters.test.ts # test/converters/warrantIssuanceConverters.test.ts
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…conversion-mechanisms
|
CI is green on the latest head, including the full OCP QuickStart integration suite. @copilot review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR refactors OCF↔DAML conversion handling to use a canonical, discriminator-driven conversion-mechanism/right model (with stricter validation), centralizes mapping logic in a shared module, and updates tests to assert the refined semantics and round-trips.
Changes:
- Introduces
shared/conversionMechanisms.tsas the single mapping layer for convertible, warrant, and stock-class ratio mechanisms (read/write). - Tightens runtime parsing/validation to enforce mechanism/right correlation, required discriminators, and PPS discount invariants.
- Removes prior normalization that silently defaulted/mutated conversion-related fields and replaces legacy round-trip tests with canonical mechanism matrices + type contracts.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils/planSecurityAliases.test.ts | Updates alias/normalization expectations to reflect preservation (no invented booleans, preserve 1:1 ratio rights). |
| test/utils/conversionSemanticRefinements.test.ts | Adds runtime parser regression tests for refined conversion-right/mechanism semantics. |
| test/types/conversionMechanisms.types.ts | Adds compile-time contracts for canonical conversion mechanism/right unions (source types). |
| test/schemaAlignment/schemaConformanceRegistry.ts | Repoints schema-conformance runtime coverage references to the new matrix test. |
| test/schemaAlignment/schemaConformance.test.ts | Updates intentional-refinement expectations for StockClassConversionRight naming/coverage. |
| test/declarations/conversionMechanisms.types.ts | Adds compile-time contracts for canonical conversion mechanism/right unions (built declarations). |
| test/createOcf/falsyFieldRoundtrip.test.ts | Aligns DAML fixtures with required conversion-right discriminator and note-required fields. |
| test/converters/warrantIssuanceConverters.test.ts | Removes legacy warrant issuance round-trip regression suite (replaced by matrix coverage). |
| test/converters/convertibleIssuanceConverters.test.ts | Removes legacy convertible issuance regression suite (replaced by matrix coverage). |
| test/converters/conversionMechanismMatrix.test.ts | Adds canonical round-trip matrices for convertible, warrant, and stock-class ratio mechanisms. |
| src/utils/planSecurityAliases.ts | Removes conversion-specific normalization logic from OCF normalization pipeline. |
| src/utils/ocfZodSchemas.ts | Adds post-AJV semantic validation for canonical conversion rights/mechanisms (including PPS discount invariants). |
| src/types/native.ts | Reworks conversion mechanism/right types into canonical discriminated unions; tightens capitalization rules typing; adds CapitalizationDefinition. |
| src/functions/OpenCapTable/warrantIssuance/getWarrantIssuanceAsOcf.ts | Refactors DAML→OCF warrant issuance reader to shared mechanism mappers and stricter validators. |
| src/functions/OpenCapTable/warrantIssuance/createWarrantIssuance.ts | Refactors OCF→DAML warrant issuance writer to canonical types and shared mechanism mappers. |
| src/functions/OpenCapTable/stockClass/stockClassDataToDaml.ts | Refactors OCF→DAML stock class writer to go through ratio mechanism mapping only. |
| src/functions/OpenCapTable/stockClass/getStockClassAsOcf.ts | Refactors DAML→OCF stock class reader to rebuild canonical ratio mechanisms via shared logic. |
| src/functions/OpenCapTable/shared/conversionMechanisms.ts | New shared conversion mechanism and capitalization-rule read/write module with strict validation. |
| src/functions/OpenCapTable/convertibleIssuance/getConvertibleIssuanceAsOcf.ts | Refactors DAML→OCF convertible issuance reader around canonical types + shared mechanism mapping. |
| src/functions/OpenCapTable/convertibleIssuance/createConvertibleIssuance.ts | Refactors OCF→DAML convertible issuance writer around canonical types + shared mechanism mapping. |
| src/functions/OpenCapTable/convertibleConversion/getConvertibleConversionAsOcf.ts | Narrows capitalization_definition typing and adds a runtime guard for the structured definition shape. |
| src/functions/OpenCapTable/convertibleConversion/damlToOcf.ts | Updates capitalization_definition typing to the new structured CapitalizationDefinition. |
| src/functions/OpenCapTable/capTable/ocfToDaml.ts | Removes unknown double-casts by using OcfDataTypeFor-typed inputs for issuance converters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Current head 80c7b76 is green, merge-clean, and fixes explicit-null optional issuance fields with JavaScript-boundary regressions. All prior review threads are resolved; the proposed long-form timing aliases are intentionally excluded because they are not present in the DAML source, generated bindings, runtime decoder, or contract history. @copilot-pull-request-reviewer please review the latest head. |
|
cursor review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
|
The current-head Cursor finding was verified against the pinned OCF schema, v34/v35 DAML sources, and generated decoder: SAFE conversion_mfn is a required Bool, so null normalization would be a schema workaround and is intentionally not added. The thread is resolved with that evidence. @copilot review |
…nonical-conversion-mechanisms # Conflicts: # src/functions/OpenCapTable/convertibleIssuance/getConvertibleIssuanceAsOcf.ts
|
Resolved the consolidated convertible-reader merge at exact head
Validation passed: 496 focused converter tests, exact-source typecheck, lint, and format. This PR remains draft while final propagation and endpoint validation continue. |
…nonical-conversion-mechanisms # Conflicts: # src/functions/OpenCapTable/convertibleIssuance/createConvertibleIssuance.ts # src/functions/OpenCapTable/shared/vesting.ts
|
Final parent-fix propagation complete at exact head a42473a, incorporating #419 at f8ef7b1. The semantic conflict was resolved in favor of this PR's canonical shared conversion-mechanism architecture: nested interest-rate records continue through the strict shared decoder, including canonical percentage-string validation and structured indexed diagnostics; the new vesting record guard uses this stack's canonical dot paths. Source typecheck plus 4 focused suites / 428 tests passed on canton-node-sdk 0.0.227. The PR remains draft and unassigned while CI runs. |
|
@copilot review |
Reviewed exact head I did not find any actionable issues in this diff. The implementation correctly enforces canonical conversion mechanisms with exact eight-way union types, strict field validation, proper error classification (REQUIRED_FIELD_MISSING/INVALID_TYPE/INVALID_FORMAT), bounded diagnostics, and lossless round-trip preservation. The writer/reader alignment for CI status check: |
|
@cursor review Copilot completed a fresh no-findings review of exact head a42473a: #420 (comment). Exact CI and QuickStart are green, GitHub reports CLEAN/MERGEABLE, and all prior threads are resolved. Please perform a formal review of this exact head against f8ef7b1, focusing on canonical conversion mechanisms and writer/reader validation symmetry. |
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 a42473a. Configure here.
…nonical-conversion-mechanisms # Conflicts: # src/utils/entityValidators.ts
|
Material parent propagation complete at exact head 0f61496, incorporating #419 at 8df0102. The semantic conflict retained this PR's stricter nonnegative DAML Numeric validation while adding the exhaustive shared stakeholder-relationship set. Symmetric vesting-graph validation was incorporated through the existing generated-DAML/lossless read and strict write architecture. Source typecheck plus 5 focused graph/relationship suites / 389 tests passed on canton-node-sdk 0.0.227. The PR remains draft and unassigned while CI runs. |
|
Fresh conversion-boundary audit hardening is pushed at exact head Three declaration/runtime gaps are closed:
The matrix covers SAFE/note discounts, note interest rates, fixed-percent and PPS discounts, direct/generic writer lexemes, generated-DAML canonical forms, empty result IDs, source+built compile-negative rounding, and read/write round trips. Validation: affected 9 suites / 1,133 tests; full 80 suites / 4,446 tests; build; source exactness; built declarations; lint; format. The PR remains draft/unassigned. GitHub currently reports a stacked-parent conflict because #419 advanced; conflict attribution is limited to the generated schema inventory/test/registry files and will be resolved during the final parent restack. No review was requested. |
…nonical-conversion-mechanisms # Conflicts: # src/errors/OcpContractError.ts # src/errors/OcpError.ts # src/errors/OcpNetworkError.ts # src/errors/OcpParseError.ts # src/errors/OcpValidationError.ts # src/functions/OpenCapTable/capTable/damlToOcf.ts # src/utils/ocfZodSchemas.ts # test/batch/damlToOcfDispatcher.test.ts # test/schemaAlignment/canonicalOcfObjectInventory.json # test/schemaAlignment/schemaConformance.test.ts # test/schemaAlignment/schemaConformanceRegistry.ts
|
Propagated corrected PR #419 head Preserved the exact schema-conformance guardrails (103 literal conditional witnesses, static-bypass rejection, draft-07 traversal, and source/dist inventory parity) together with this PR’s strict conversion contracts: OCF-vs-DAML percentage handling, non-empty conversion result IDs, and NORMAL-only persisted stock-class ratios. The canonical inventory was regenerated twice with identical SHA-256 Validation is green locally: 81 suites / 5,035 tests, build, typecheck, declaration tests, exact source/public-config checks, lint, Prettier, and diff checks. This PR remains draft and unassigned while the corrected stack is propagated and fresh exact-head reviews are pending. |
…nonical-conversion-mechanisms
|
Re-propagated corrected PR #419 head Preserved strict OCF-versus-DAML percentage handling, non-empty conversion result IDs, NORMAL-only persisted stock-class ratios, the full schema-conformance harness, and corrected full-payload PlanSecurity/Monetary behavior. Validation is green locally: 7 focused suites / 773 tests; full coverage run 81 suites / 5,049 tests; clean build; typecheck; declaration, exact source, and exact public-config tests; lint; Prettier; and diff checks. This PR remains draft and unassigned pending downstream propagation and fresh exact-head reviews. |
|
@copilot review Please review exact current head |
|
@copilot review Retrying the required exact-head gate for |
Reviewed exact head I did not find any actionable issues in this diff. The implementation demonstrates: Canonical conversion-mechanism discriminated unions: All conversion mechanisms use the exact eight-way OCF union with type-specific field sets (SAFE_FIELDS, NOTE_FIELDS, CUSTOM_FIELDS, etc.). Each mechanism enforces its complete required fields via Discriminator precedence: The validation flow correctly prioritizes discriminators:
Runtime validation: Comprehensive boundaries include:
Declaration parity: Test files ( CI status: Both |
Reviewed exact head I did not find any actionable issues in this diff. The implementation correctly enforces:
CI status: Both |
|
@cursor review Please review exact current 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 f3c5773. Configure here.
|
Returned to draft because upstream #417 received a material exact-head corrective revision after full CodeRabbit review. This stacked PR will remain held until the audited #417 head and each intervening parent are propagated normally; fresh CI and Copilot-first reviewer gates are required afterward. |
|
Restacked through current #419 head The canonical/persistence type split is now explicit: canonical OCF Local exact-head validation passes: lint, Prettier, build, typecheck/exact-source, declarations/exact-public-config, full |
|
Exact draft head |
Summary
Validation
Stack
Depends on #419 at exact head ab1e25a. This PR remains draft until its fresh CI, QuickStart, and exact-head reviewer gates complete.