Enforce OCF reference integrity#251
Closed
HardlyDifficult wants to merge 10 commits into
Closed
Conversation
📝 WalkthroughWalkthroughAdds codegen support for issuer and object reference validation in generated CapTable DAML, bumps the package to 0.0.4, and updates DAML test suites to cover reference integrity for stock classes, plans, issuances, documents, valuations, convertibles, warrants, and equity compensation. ChangesReference validation feature and tests
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant UpdateCapTable
participant processCreate
participant validateObjectReference
participant CapTableMaps
Client->>UpdateCapTable: submit creates/edits/deletes
UpdateCapTable->>processCreate: newIssuerCid, maps, createData
processCreate->>validateObjectReference: check related_objects/refs
validateObjectReference->>CapTableMaps: lookup referenced entity
CapTableMaps-->>validateObjectReference: found/not found
validateObjectReference-->>processCreate: assert or fail
processCreate-->>UpdateCapTable: updated maps
UpdateCapTable-->>Client: success or failure
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 |
This was referenced Jul 9, 2026
Collaborator
Author
|
@copilot review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e07ce3cfe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| validateWarrantConversionRightReferences finalMaps | ||
| validateValuationStockClassReferences finalMaps | ||
| validateEquityCompensationIssuanceReferences finalMaps | ||
| validateWarrantIssuanceReferences finalMaps |
There was a problem hiding this comment.
This final integrity pass skips the new StockPlan and StockIssuance references, so delete-only batches bypass the checks added in the config because processCreate/processEdit never run. For example, deleting a stock class used by an existing StockPlan, or deleting a legend/plan/vesting terms used by an existing StockIssuance, reaches this point and recreates the CapTable with dangling IDs; add final-state validators here or run the config validations over the final maps.
Useful? React with 👍 / 👎.
Comment on lines
+87
to
+88
| 'stock_plan_id?:stock_plans', | ||
| 'vesting_terms_id?:vesting_terms', |
There was a problem hiding this comment.
Putting these newly added references in the per-edit validation path makes processEdit check them before creates are processed, so an existing StockIssuance cannot be edited to point at a StockPlan or VestingTerms created in the same UpdateCapTable batch. That contradicts the final-state validation pattern used elsewhere in this patch (for example WarrantIssuance can edit to a newly created VestingTerms); these references need to be validated against the final maps rather than only the pre-create maps.
Useful? React with 👍 / 👎.
| {{object_type_constructor}} -> | ||
| assertMsg ("{{name}} not found: " <> ref.object_id) (isSome (Map.lookup ref.object_id maps.{{map_field}})) | ||
| {{/each}} | ||
| _ -> assertMsg ("Unsupported related object type " <> show ref.object_type <> " for object id " <> ref.object_id) False |
There was a problem hiding this comment.
When a document relates to an equity-compensation/plan-security transaction using the OcfObjTxPlanSecurity* constructors that are still part of OcfObjectType, this wildcard rejects the reference as unsupported even if the matching EquityCompensation* contract exists in the CapTable. Since this package models plan securities through the EquityCompensation* templates, those object-type aliases need to be checked against the corresponding equity-compensation maps rather than falling through here.
Useful? React with 👍 / 👎.
| OcfRightWarrant warrantRight -> | ||
| validateOptionalStockClassReference (label <> " warrant conversion target") maps warrantRight.converts_to_stock_class_id | ||
| OcfRightStockClass stockClassRight -> | ||
| validateStockClassReference (label <> " stock class conversion target") maps stockClassRight.converts_to_stock_class_id |
There was a problem hiding this comment.
When a warrant exercise trigger uses OcfRightStockClass, this branch only checks the outer converts_to_stock_class_id, but OcfStockClassConversionRight also contains its own conversion_trigger with another OcfAnyConversionRight. If that nested right points at a missing stock class, the update still succeeds with a dangling reference; recurse into stockClassRight.conversion_trigger.conversion_right as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR consolidates and extends OCF cross-object reference integrity enforcement in UpdateCapTable, pairing stricter reference validation (create/edit and post-batch invariants) with updated test fixtures and a v34 package bump to preserve immutable DAR backups.
Changes:
- Adds/extends codegen support for optional (
field_id?:map) reference validations and object-type aware documentrelated_objectsvalidation. - Introduces post-batch invariant checks for multiple reference families (documents, conversion-right targets, valuations, equity-comp issuances, warrant vesting terms) and adjusts batch tiers/order for StockPlan and plan adjustments.
- Updates/expands DAML script tests to create required referenced objects (or clear optional refs) and bumps
OpenCapTable-v34to0.0.3with a new DAR backup + lock entry.
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Test/daml/OpenCapTable/TestWarrantIssuanceReferences.daml | Adds focused script coverage for warrant vesting-terms reference integrity (create/edit/delete). |
| Test/daml/OpenCapTable/TestWarrantIssuance.daml | Updates warrant optional-fields test to create referenced vesting terms in-batch. |
| Test/daml/OpenCapTable/TestWarrantConversionRightReferences.daml | Adds script coverage for warrant exercise-trigger conversion-right stock-class targets across OcfAnyConversionRight variants. |
| Test/daml/OpenCapTable/TestVestingStart.daml | Clears equity-comp optional refs to avoid failing stricter reference checks. |
| Test/daml/OpenCapTable/TestVestingEvent.daml | Clears equity-comp optional refs to avoid failing stricter reference checks. |
| Test/daml/OpenCapTable/TestVestingAcceleration.daml | Clears equity-comp optional refs to avoid failing stricter reference checks. |
| Test/daml/OpenCapTable/TestValuationReferences.daml | Adds script coverage for valuation stock-class reference integrity (create/edit/delete). |
| Test/daml/OpenCapTable/TestStockRetraction.daml | Removes phantom stock legend IDs from stock-issuance helper to match new validation expectations. |
| Test/daml/OpenCapTable/TestStockPlanReturnToPool.daml | Ensures required stock class exists before creating stock plans in these flows. |
| Test/daml/OpenCapTable/TestStockPlanReferences.daml | Adds script coverage for StockPlan.stock_class_ids reference validation on create/edit and same-batch create. |
| Test/daml/OpenCapTable/TestStockIssuanceReferences.daml | Adds script coverage for StockIssuance legend/plan/vesting reference validation on create/edit and same-batch creates. |
| Test/daml/OpenCapTable/TestStockIssuance.daml | Creates referenced legend/plan/vesting terms before the “optionals populated” issuance case. |
| Test/daml/OpenCapTable/TestStockClassConversionRightReferences.daml | Adds script coverage for StockClass conversion target references (create/edit/delete target). |
| Test/daml/OpenCapTable/TestStockClass.daml | Refactors common stock class fixture and ensures target classes exist where referenced. |
| Test/daml/OpenCapTable/TestStockCancellation.daml | Removes phantom stock legend IDs from stock-issuance helper to match new validation expectations. |
| Test/daml/OpenCapTable/TestStockAcceptance.daml | Removes phantom stock legend IDs from stock-issuance helper to match new validation expectations. |
| Test/daml/OpenCapTable/TestIssuerAuthorizedSharesAdjustment.daml | Adds issuer-id mismatch negative tests and refactors adjustment construction helper. |
| Test/daml/OpenCapTable/TestHelpers.daml | Adds new default fixtures (legend template, stock plan, vesting terms) and removes placeholder legend IDs. |
| Test/daml/OpenCapTable/TestEquityCompensationTransfer.daml | Clears equity-comp optional refs to avoid failing stricter reference checks. |
| Test/daml/OpenCapTable/TestEquityCompensationRetraction.daml | Clears equity-comp optional refs to avoid failing stricter reference checks. |
| Test/daml/OpenCapTable/TestEquityCompensationIssuanceReferences.daml | Adds script coverage for equity-comp optional reference integrity incl. delete-blocking scenarios. |
| Test/daml/OpenCapTable/TestEquityCompensationIssuance.daml | Updates “optionals populated” path to create referenced objects in-batch. |
| Test/daml/OpenCapTable/TestEquityCompensationCancellation.daml | Clears equity-comp optional refs to avoid failing stricter reference checks. |
| Test/daml/OpenCapTable/TestEquityCompensationAcceptance.daml | Clears equity-comp optional refs to avoid failing stricter reference checks. |
| Test/daml/OpenCapTable/TestDocument.daml | Adds document related-object reference tests and updates issuer reference to actual cap table issuer ID. |
| Test/daml/OpenCapTable/TestConvertibleConversionRightReferences.daml | Adds script coverage for convertible conversion target stock-class references (create/edit/delete target). |
| Test/daml/OpenCapTable/TestConversionMechanisms.daml | Ensures referenced stock classes exist for conversion-mechanism tests under stricter checks. |
| Test/daml.yaml | Points tests at OpenCapTable-v34-0.0.3.dar. |
| scripts/codegen/templates/loops/edit-case.daml | Adds optional-reference validation generation and issuer-id validation hook for edits. |
| scripts/codegen/templates/loops/create-case.daml | Adds optional-reference validation generation and issuer-id validation hook for creates. |
| scripts/codegen/templates/CapTable.daml.template | Adds issuer/document/object reference validation and multiple post-batch invariant checks; threads issuer cid into create/edit. |
| scripts/codegen/generate-captable.ts | Adds optional-reference validation parsing, object-type constructor mapping, and document handling metadata. |
| scripts/codegen/captable-config.yaml | Adjusts tiers (StockPlan ordering) and declares new optional/array validations. |
| OpenCapTable-v34/daml.yaml | Bumps package version to 0.0.3. |
| dars/OpenCapTable-v34/0.0.3/OpenCapTable-v34.dar | Adds new immutable DAR backup pointer for 0.0.3. |
| dars/dars.lock | Records new 0.0.3 DAR hash/size metadata. |
Comment on lines
+454
to
+460
| validateDocumentRelatedObjects finalMaps newIssuerCid | ||
| validateStockClassConversionRightReferences finalMaps | ||
| validateConvertibleConversionRightReferences finalMaps | ||
| validateWarrantConversionRightReferences finalMaps | ||
| validateValuationStockClassReferences finalMaps | ||
| validateEquityCompensationIssuanceReferences finalMaps | ||
| validateWarrantIssuanceReferences finalMaps |
Comment on lines
+251
to
+258
| validateOptionalStockClassReference : Text -> CapTableMaps -> Optional Text -> Update () | ||
| validateOptionalStockClassReference label maps stockClassId = case stockClassId of | ||
| Some id -> validateStockClassReference label maps id | ||
| None -> pure () | ||
|
|
||
| validateStockClassConversionRightReferences : CapTableMaps -> Update () | ||
| validateStockClassConversionRightReferences maps = | ||
| mapA_ |
Comment on lines
+303
to
+319
| validateEquityCompensationIssuanceReferences : CapTableMaps -> Update () | ||
| validateEquityCompensationIssuanceReferences maps = | ||
| mapA_ | ||
| (\issuanceCid -> do | ||
| issuance <- fetch issuanceCid | ||
| validateOptionalStockClassReference "Equity compensation stock class" maps issuance.issuance_data.stock_class_id | ||
| validateOptionalReference "Equity compensation stock plan" maps.stock_plans issuance.issuance_data.stock_plan_id | ||
| validateOptionalReference "Equity compensation vesting terms" maps.vesting_terms issuance.issuance_data.vesting_terms_id) | ||
| (Map.values maps.equity_compensation_issuances) | ||
|
|
||
| validateWarrantIssuanceReferences : CapTableMaps -> Update () | ||
| validateWarrantIssuanceReferences maps = | ||
| mapA_ | ||
| (\issuanceCid -> do | ||
| issuance <- fetch issuanceCid | ||
| validateOptionalReference "Warrant vesting terms" maps.vesting_terms issuance.issuance_data.vesting_terms_id) | ||
| (Map.values maps.warrant_issuances) |
Collaborator
Author
|
@copilot review |
…ing' into codex/ocf-reference-integrity # Conflicts: # scripts/codegen/templates/CapTable.daml.template # scripts/codegen/templates/loops/edit-case.daml
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@scripts/codegen/templates/CapTable.daml.template`:
- Around line 668-674: The new global validators invoked from UpdateCapTable add
multiple full-map scans on every batch update, compounding the existing
authorized-shares scan in CapTable.daml.template. Review the validation flow
around validateDocumentRelatedObjects,
validateStockClassConversionRightReferences,
validateConvertibleConversionRightReferences,
validateWarrantConversionRightReferences, validateValuationStockClassReferences,
validateEquityCompensationIssuanceReferences, and
validateWarrantIssuanceReferences, and reduce repeated whole-map iteration where
possible or gate these checks so they only run when the relevant maps change.
- Around line 495-519: The revalidation logic in
validateValuationStockClassReferences and
validateEquityCompensationIssuanceReferences is incomplete because deletes can
leave StockPlan.stock_class_ids and StockIssuance.stock_plan_id/vesting_terms_id
dangling. Update the batch revalidation path in CapTable.daml.template to also
iterate through the relevant StockPlan and StockIssuance maps and re-run the
appropriate reference checks after delete-case.daml archives/removes entries,
using the existing validateOptionalReference/validateStockClassReference helpers
so these relationships are validated globally.
In `@Test/daml/OpenCapTable/TestConvertibleConversionRightReferences.daml`:
- Around line 76-89: Add a happy-path test alongside
testConvertibleConversionRightEditMissingTargetFails to verify processEdit
accepts a valid conversion target. In
TestConvertibleConversionRightReferences.daml, create a stock class and a
convertible issuance with convertibleIssuanceWithConversionTarget, then submit
an OcfEditConvertibleIssuance that sets converts_to_stock_class_id to the
existing stock class and assert the update succeeds. Use the existing helper
names (testConvertibleConversionRightEditMissingTargetFails, processEdit,
convertibleIssuanceWithConversionTarget) to keep the new test aligned with the
current validation coverage.
In `@Test/daml/OpenCapTable/TestHelpers.daml`:
- Around line 152-189: Extract the hardcoded numeric defaults in
defaultStockPlan and defaultVestingTerms into named constants, following the
same pattern used by defaultInitialSharesAuthorized. Introduce clear constants
for the reserved shares in defaultStockPlan and the vesting quantity in
defaultVestingTerms, then reference those symbols inside the StockPlanOcfData
and VestingTermsOcfData builders to keep the test helpers consistent and easy to
tune.
In `@Test/daml/OpenCapTable/TestIssuerAuthorizedSharesAdjustment.daml`:
- Around line 76-93: The new issuer-id failure tests in
TestIssuerAuthorizedSharesAdjustment should separate “mismatch” from “does not
exist” by using a real, created Issuer contract for the negative case. Update
testIssuerAuthorizedSharesAdjustment_WrongIssuerIdCreateFails and
testIssuerAuthorizedSharesAdjustment_WrongIssuerIdEditFails so one variant uses
an actual alternate issuer contract id rather than only "OTHER_ISSUER", while
keeping the existing non-existent-id case if desired. Use setupTestOcp,
createIssuerAuthorizedSharesAdjustment, and the CT.UpdateCapTable /
issuerAuthorizedSharesAdjustment helpers to make the mismatch-specific behavior
explicit.
🪄 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: 821e91d0-a119-4d98-9d51-f49d2e8d74ff
⛔ Files ignored due to path filters (1)
dars/dars.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
OpenCapTable-v34/daml.yamlTest/daml.yamlTest/daml/OpenCapTable/TestConversionMechanisms.damlTest/daml/OpenCapTable/TestConvertibleConversionRightReferences.damlTest/daml/OpenCapTable/TestDocument.damlTest/daml/OpenCapTable/TestEquityCompensationAcceptance.damlTest/daml/OpenCapTable/TestEquityCompensationCancellation.damlTest/daml/OpenCapTable/TestEquityCompensationIssuance.damlTest/daml/OpenCapTable/TestEquityCompensationIssuanceReferences.damlTest/daml/OpenCapTable/TestEquityCompensationRetraction.damlTest/daml/OpenCapTable/TestEquityCompensationTransfer.damlTest/daml/OpenCapTable/TestHelpers.damlTest/daml/OpenCapTable/TestIssuerAuthorizedSharesAdjustment.damlTest/daml/OpenCapTable/TestStockAcceptance.damlTest/daml/OpenCapTable/TestStockCancellation.damlTest/daml/OpenCapTable/TestStockClass.damlTest/daml/OpenCapTable/TestStockClassConversionRightReferences.damlTest/daml/OpenCapTable/TestStockIssuance.damlTest/daml/OpenCapTable/TestStockIssuanceReferences.damlTest/daml/OpenCapTable/TestStockPlanReferences.damlTest/daml/OpenCapTable/TestStockPlanReturnToPool.damlTest/daml/OpenCapTable/TestStockRetraction.damlTest/daml/OpenCapTable/TestValuationReferences.damlTest/daml/OpenCapTable/TestVestingAcceleration.damlTest/daml/OpenCapTable/TestVestingEvent.damlTest/daml/OpenCapTable/TestVestingStart.damlTest/daml/OpenCapTable/TestWarrantConversionRightReferences.damlTest/daml/OpenCapTable/TestWarrantIssuance.damlTest/daml/OpenCapTable/TestWarrantIssuanceReferences.damldars/OpenCapTable-v34/0.0.4/OpenCapTable-v34.darscripts/codegen/captable-config.yamlscripts/codegen/generate-captable.tsscripts/codegen/templates/CapTable.daml.templatescripts/codegen/templates/loops/create-case.damlscripts/codegen/templates/loops/edit-case.daml
Comment on lines
+76
to
+89
| testConvertibleConversionRightEditMissingTargetFails = script do | ||
| TestOcp{issuer, cap_table} <- setupTestOcp | ||
| capTableWithStakeholder <- addDefaultStakeholder issuer cap_table | ||
| result <- submit issuer do | ||
| exerciseCmd capTableWithStakeholder CT.UpdateCapTable with | ||
| creates = [CT.OcfCreateConvertibleIssuance (convertibleIssuanceWithConversionTarget "TX_CONVERTIBLE_EDIT_TARGET" "CONV-EDIT-TARGET" None)] | ||
| edits = [] | ||
| deletes = [] | ||
|
|
||
| submitMustFail issuer do | ||
| exerciseCmd result.updatedCapTableCid CT.UpdateCapTable with | ||
| creates = [] | ||
| edits = [CT.OcfEditConvertibleIssuance (convertibleIssuanceWithConversionTarget "TX_CONVERTIBLE_EDIT_TARGET" "CONV-EDIT-TARGET" (Some "SC_MISSING_CONVERTIBLE_EDIT_TARGET"))] | ||
| deletes = [] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider adding a positive edit-success test.
Only the failure path for editing a conversion target is covered (testConvertibleConversionRightEditMissingTargetFails). There's no test confirming that editing a convertible issuance's converts_to_stock_class_id to a valid existing stock class succeeds, which is the complementary happy-path for the edit validation wired into processEdit.
🧪 Example addition
testConvertibleConversionRightEditValidTargetSucceeds = script do
TestOcp{issuer, cap_table} <- setupTestOcp
capTableWithStakeholder <- addDefaultStakeholder issuer cap_table
result <- submit issuer do
exerciseCmd capTableWithStakeholder CT.UpdateCapTable with
creates =
[ CT.OcfCreateStockClass (defaultStockClass "SC_CONVERTIBLE_EDIT_VALID_TARGET")
, CT.OcfCreateConvertibleIssuance (convertibleIssuanceWithConversionTarget "TX_CONVERTIBLE_EDIT_VALID" "CONV-EDIT-VALID" None)
]
edits = []
deletes = []
_ <- submit issuer do
exerciseCmd result.updatedCapTableCid CT.UpdateCapTable with
creates = []
edits = [CT.OcfEditConvertibleIssuance (convertibleIssuanceWithConversionTarget "TX_CONVERTIBLE_EDIT_VALID" "CONV-EDIT-VALID" (Some "SC_CONVERTIBLE_EDIT_VALID_TARGET"))]
deletes = []
pure ()🤖 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 `@Test/daml/OpenCapTable/TestConvertibleConversionRightReferences.daml` around
lines 76 - 89, Add a happy-path test alongside
testConvertibleConversionRightEditMissingTargetFails to verify processEdit
accepts a valid conversion target. In
TestConvertibleConversionRightReferences.daml, create a stock class and a
convertible issuance with convertibleIssuanceWithConversionTarget, then submit
an OcfEditConvertibleIssuance that sets converts_to_stock_class_id to the
existing stock class and assert the update succeeds. Use the existing helper
names (testConvertibleConversionRightEditMissingTargetFails, processEdit,
convertibleIssuanceWithConversionTarget) to keep the new test aligned with the
current validation coverage.
Comment on lines
+152
to
+189
| -- | Returns valid StockLegendTemplateOcfData with sensible defaults. | ||
| defaultStockLegendTemplate : Text -> StockLegendTemplateOcfData | ||
| defaultStockLegendTemplate legendId = StockLegendTemplateOcfData with | ||
| id = legendId | ||
| name = "Restricted Stock Legend" | ||
| text = "Shares represented by this certificate are subject to transfer restrictions." | ||
| comments = [] | ||
|
|
||
| -- | Returns valid StockPlanOcfData with sensible defaults for a stock class. | ||
| defaultStockPlan : Text -> Text -> StockPlanOcfData | ||
| defaultStockPlan planId stockClassId = StockPlanOcfData with | ||
| id = planId | ||
| initial_shares_reserved = 1000.0 | ||
| plan_name = "Equity Incentive Plan" | ||
| comments = [] | ||
| stock_class_ids = [stockClassId] | ||
| board_approval_date = None | ||
| default_cancellation_behavior = None | ||
| stockholder_approval_date = None | ||
|
|
||
| -- | Returns valid VestingTermsOcfData with a single start condition. | ||
| defaultVestingTerms : Text -> VestingTermsOcfData | ||
| defaultVestingTerms vestingTermsId = | ||
| let startCondition = OcfVestingCondition with | ||
| id = "start" | ||
| trigger = OcfVestingStartTrigger | ||
| next_condition_ids = [] | ||
| description = Some "All at start" | ||
| portion = None | ||
| quantity = Some 100.0 | ||
| in VestingTermsOcfData with | ||
| id = vestingTermsId | ||
| allocation_type = OcfAllocationCumulativeRounding | ||
| description = "All shares vest at start" | ||
| name = "Immediate vesting" | ||
| comments = [] | ||
| vesting_conditions = [startCondition] | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Extract magic numbers into named constants.
defaultStockPlan's 1000.0 and defaultVestingTerms's 100.0 are hardcoded, unlike defaultInitialSharesAuthorized (Line 51) which was already extracted as a named constant for the same purpose. Consider the same pattern here for consistency and easier tuning.
♻️ Proposed refactor
+-- | Default reserved shares for test stock plans.
+defaultStockPlanReservedShares : Decimal
+defaultStockPlanReservedShares = 1000.0
+
defaultStockPlan : Text -> Text -> StockPlanOcfData
defaultStockPlan planId stockClassId = StockPlanOcfData with
id = planId
- initial_shares_reserved = 1000.0
+ initial_shares_reserved = defaultStockPlanReservedShares📝 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
| -- | Returns valid StockLegendTemplateOcfData with sensible defaults. | |
| defaultStockLegendTemplate : Text -> StockLegendTemplateOcfData | |
| defaultStockLegendTemplate legendId = StockLegendTemplateOcfData with | |
| id = legendId | |
| name = "Restricted Stock Legend" | |
| text = "Shares represented by this certificate are subject to transfer restrictions." | |
| comments = [] | |
| -- | Returns valid StockPlanOcfData with sensible defaults for a stock class. | |
| defaultStockPlan : Text -> Text -> StockPlanOcfData | |
| defaultStockPlan planId stockClassId = StockPlanOcfData with | |
| id = planId | |
| initial_shares_reserved = 1000.0 | |
| plan_name = "Equity Incentive Plan" | |
| comments = [] | |
| stock_class_ids = [stockClassId] | |
| board_approval_date = None | |
| default_cancellation_behavior = None | |
| stockholder_approval_date = None | |
| -- | Returns valid VestingTermsOcfData with a single start condition. | |
| defaultVestingTerms : Text -> VestingTermsOcfData | |
| defaultVestingTerms vestingTermsId = | |
| let startCondition = OcfVestingCondition with | |
| id = "start" | |
| trigger = OcfVestingStartTrigger | |
| next_condition_ids = [] | |
| description = Some "All at start" | |
| portion = None | |
| quantity = Some 100.0 | |
| in VestingTermsOcfData with | |
| id = vestingTermsId | |
| allocation_type = OcfAllocationCumulativeRounding | |
| description = "All shares vest at start" | |
| name = "Immediate vesting" | |
| comments = [] | |
| vesting_conditions = [startCondition] | |
| -- | Returns valid StockLegendTemplateOcfData with sensible defaults. | |
| defaultStockLegendTemplate : Text -> StockLegendTemplateOcfData | |
| defaultStockLegendTemplate legendId = StockLegendTemplateOcfData with | |
| id = legendId | |
| name = "Restricted Stock Legend" | |
| text = "Shares represented by this certificate are subject to transfer restrictions." | |
| comments = [] | |
| -- | Returns valid StockPlanOcfData with sensible defaults for a stock class. | |
| -- | Default reserved shares for test stock plans. | |
| defaultStockPlanReservedShares : Decimal | |
| defaultStockPlanReservedShares = 1000.0 | |
| defaultStockPlan : Text -> Text -> StockPlanOcfData | |
| defaultStockPlan planId stockClassId = StockPlanOcfData with | |
| id = planId | |
| initial_shares_reserved = defaultStockPlanReservedShares | |
| plan_name = "Equity Incentive Plan" | |
| comments = [] | |
| stock_class_ids = [stockClassId] | |
| board_approval_date = None | |
| default_cancellation_behavior = None | |
| stockholder_approval_date = None | |
| -- | Returns valid VestingTermsOcfData with a single start condition. | |
| defaultVestingTerms : Text -> VestingTermsOcfData | |
| defaultVestingTerms vestingTermsId = | |
| let startCondition = OcfVestingCondition with | |
| id = "start" | |
| trigger = OcfVestingStartTrigger | |
| next_condition_ids = [] | |
| description = Some "All at start" | |
| portion = None | |
| quantity = Some 100.0 | |
| in VestingTermsOcfData with | |
| id = vestingTermsId | |
| allocation_type = OcfAllocationCumulativeRounding | |
| description = "All shares vest at start" | |
| name = "Immediate vesting" | |
| comments = [] | |
| vesting_conditions = [startCondition] |
🤖 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 `@Test/daml/OpenCapTable/TestHelpers.daml` around lines 152 - 189, Extract the
hardcoded numeric defaults in defaultStockPlan and defaultVestingTerms into
named constants, following the same pattern used by
defaultInitialSharesAuthorized. Introduce clear constants for the reserved
shares in defaultStockPlan and the vesting quantity in defaultVestingTerms, then
reference those symbols inside the StockPlanOcfData and VestingTermsOcfData
builders to keep the test helpers consistent and easy to tune.
Comment on lines
+76
to
+93
| testIssuerAuthorizedSharesAdjustment_WrongIssuerIdCreateFails = script do | ||
| TestOcp{issuer, cap_table} <- setupTestOcp | ||
|
|
||
| submitMustFail issuer do | ||
| exerciseCmd cap_table CT.UpdateCapTable with | ||
| creates = [CT.OcfCreateIssuerAuthorizedSharesAdjustment (issuerAuthorizedSharesAdjustment "TX_BAD_ISSUER_ID" "OTHER_ISSUER" 2000.0)] | ||
| edits = [] | ||
| deletes = [] | ||
|
|
||
| testIssuerAuthorizedSharesAdjustment_WrongIssuerIdEditFails = script do | ||
| TestOcp{issuer, cap_table} <- setupTestOcp | ||
| result <- createIssuerAuthorizedSharesAdjustment issuer cap_table | ||
|
|
||
| submitMustFail issuer do | ||
| exerciseCmd result.updatedCapTableCid CT.UpdateCapTable with | ||
| creates = [] | ||
| edits = [CT.OcfEditIssuerAuthorizedSharesAdjustment (issuerAuthorizedSharesAdjustment "TX_ISSUER_AUTH_SHARES_ADJ_1" "OTHER_ISSUER" 2000.0)] | ||
| deletes = [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Consider testing mismatch vs. non-existence separately.
Both new tests use "OTHER_ISSUER", an issuer id that never exists as an Issuer contract anywhere in the test setup. If the underlying validation checks "does the referenced issuer_id match the cap table's configured issuer" (a mismatch check) rather than "does an Issuer contract with this id exist" (a pure existence check), these tests only exercise the latter path and may not confirm mismatch-specific logic works. Consider adding a variant where a second, real Issuer is created and used as the mismatched id, to unambiguously test the mismatch case.
🤖 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 `@Test/daml/OpenCapTable/TestIssuerAuthorizedSharesAdjustment.daml` around
lines 76 - 93, The new issuer-id failure tests in
TestIssuerAuthorizedSharesAdjustment should separate “mismatch” from “does not
exist” by using a real, created Issuer contract for the negative case. Update
testIssuerAuthorizedSharesAdjustment_WrongIssuerIdCreateFails and
testIssuerAuthorizedSharesAdjustment_WrongIssuerIdEditFails so one variant uses
an actual alternate issuer contract id rather than only "OTHER_ISSUER", while
keeping the existing non-existent-id case if desired. Use setupTestOcp,
createIssuerAuthorizedSharesAdjustment, and the CT.UpdateCapTable /
issuerAuthorizedSharesAdjustment helpers to make the mismatch-specific behavior
explicit.
…ing' into codex/ocf-reference-integrity # Conflicts: # scripts/codegen/generate-captable.ts
…ing' into codex/ocf-reference-integrity
Collaborator
Author
|
@copilot review |
Collaborator
Author
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 37674c1. Configure here.
Collaborator
Author
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Test/daml/OpenCapTable/TestWarrantConversionRightReferences.daml (1)
148-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvertible and stock-class conversion-right variants lack success/edit/delete coverage.
Only "missing target fails" tests exist for
convertibleConversionTriggerToandstockClassConversionTriggerTo. The fixed-amount variant gets full parity (same-batch success, edit-missing-fails, delete-dangling-fails at Lines 124-136, 197-228), but no equivalent tests exercise those paths for the other two trigger kinds.Suggested additional test skeletons
testWarrantConvertibleConversionRightTargetCreatedInSameBatchSucceeds = script do ... creates = [ CT.OcfCreateStockClass (defaultStockClass "SC_CONVERTIBLE_TARGET") , CT.OcfCreateWarrantIssuance (warrantIssuanceWithExerciseTriggers ... [convertibleConversionTriggerTo (Some "SC_CONVERTIBLE_TARGET")]) ] testWarrantConvertibleConversionRightTargetDeleteCannotLeaveDanglingReference = script do ... testWarrantStockClassConversionRightTargetCreatedInSameBatchSucceeds = script do ... testWarrantStockClassConversionRightTargetDeleteCannotLeaveDanglingReference = script do ...🤖 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 `@Test/daml/OpenCapTable/TestWarrantConversionRightReferences.daml` around lines 148 - 179, The warrant conversion-right coverage is incomplete for the convertible and stock-class trigger variants; add parity tests around `convertibleConversionTriggerTo` and `stockClassConversionTriggerTo` similar to the fixed-amount cases. Extend the existing `testWarrantConvertibleConversionRightMissingTargetFails` and `testWarrantStockClassConversionRightMissingTargetFails` area with success-in-same-batch tests using `CT.OcfCreateStockClass` plus `CT.OcfCreateWarrantIssuance`, and add edit/delete negative cases that prove dangling references fail when the target is removed or changed. Use the same helpers (`warrantIssuanceWithExerciseTriggers`, `exerciseCmd`, `submitMustFail`) so the new tests cover create, edit, and delete behavior consistently.
🤖 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.
Outside diff comments:
In `@Test/daml/OpenCapTable/TestWarrantConversionRightReferences.daml`:
- Around line 148-179: The warrant conversion-right coverage is incomplete for
the convertible and stock-class trigger variants; add parity tests around
`convertibleConversionTriggerTo` and `stockClassConversionTriggerTo` similar to
the fixed-amount cases. Extend the existing
`testWarrantConvertibleConversionRightMissingTargetFails` and
`testWarrantStockClassConversionRightMissingTargetFails` area with
success-in-same-batch tests using `CT.OcfCreateStockClass` plus
`CT.OcfCreateWarrantIssuance`, and add edit/delete negative cases that prove
dangling references fail when the target is removed or changed. Use the same
helpers (`warrantIssuanceWithExerciseTriggers`, `exerciseCmd`, `submitMustFail`)
so the new tests cover create, edit, and delete behavior consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: acea7fcd-43b8-47f7-aa1e-267f8707d1f2
⛔ Files ignored due to path filters (1)
dars/dars.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Test/daml/OpenCapTable/TestDocument.damlTest/daml/OpenCapTable/TestEquityCompensationIssuanceReferences.damlTest/daml/OpenCapTable/TestStockIssuanceReferences.damlTest/daml/OpenCapTable/TestStockPlanReferences.damlTest/daml/OpenCapTable/TestValuationReferences.damlTest/daml/OpenCapTable/TestWarrantConversionRightReferences.damldars/OpenCapTable-v34/0.0.4/OpenCapTable-v34.darscripts/codegen/captable-config.yamlscripts/codegen/generate-captable.tsscripts/codegen/templates/CapTable.daml.template
💤 Files with no reviewable changes (1)
- scripts/codegen/generate-captable.ts
Collaborator
Author
|
Closed as superseded by grouped reference-integrity #268. Before closing, I carried the actionable feedback into #268: reference checks run against final post-delete/post-edit/post-create cap-table state; plan-security aliases are treated as equity compensation references; same-batch reference edits are allowed when the referenced object is created in the same batch; and nested stock-class/warrant conversion targets are validated. #268 is restacked on #238 and locally passes build, DAR backup/verify, full tests, lint, upgrade compatibility, and diff check. |
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.
Summary
OpenCapTable-v340.0.3for authorized-share ceiling enforcement.UpdateCapTablefor stock issuance optional refs, stock-plan stock classes, issuer adjustments, document related objects, conversion-right targets, valuation stock classes, equity-compensation issuance refs, and warrant vesting terms.OpenCapTable-v34to0.0.4and adds a new immutable DAR backup for the reference-integrity feature.Review comments folded in
StockPlanafterStockClassin create tiers, moved plan adjustments afterStockPlan, and updated the same-batch test to submitStockPlanbeforeStockClass.validateIssuerReferencefor document issuer references and improved unsupported related-object errors to includeobject_typeandobject_id.OcfAnyConversionRightvariants in warrant exercise triggers, with regression tests for convertible and stock-class variants.WarrantIssuance.vesting_terms_idcheck and added a same-batch edit/create regression test that relies on the final invariant.0.0.3DAR as the base and addingOpenCapTable-v340.0.4for this PR.OCF documentation
Validation
npm run buildnpm run verify-darsJAVA_HOME=/opt/homebrew/opt/openjdk@17 npm run check-upgrade-compatcd Test && JAVA_HOME=/opt/homebrew/opt/openjdk@17 dpm test --show-coverage --color --coverage-ignore-choice 'splice-amulet:.*'JAVA_HOME=/opt/homebrew/opt/openjdk@17 npm run lint:damlNote
High Risk
Changes core
UpdateCapTablevalidation and batch ordering for cap-table mutations; incorrect rules could reject valid OCF batches or miss dangling references across many object types.Overview
UpdateCapTablenow runs post-batch reference checks on the final cap table maps so creates, edits, and deletes cannot leave dangling OCF links. Covered areas include documents (related_objects/ issuer ID), stock plans (stock class IDs), stock and equity-compensation issuances (stakeholder, class, plan, legends, vesting), valuations, issuer authorized-share adjustments (issuer ID), and conversion targets on stock classes, convertibles, and warrants (including nested warrant trigger rights).The codegen path was updated:
StockPlanand plan pool adjustments were re-tiered for dependencies; several issuance types no longer validate rich refs at create/edit time and instead rely on the final invariant; optional and array reference specs were added;processCreate/processEdittake the issuer contract for issuer-scoped checks.OpenCapTable-v34is bumped to0.0.4with a new DAR backup; Test depends on that DAR and adds broad Daml Script coverage (same-batch success, missing refs, bad edits, delete-blocking) plus test helper defaults so fixtures create referenced objects when needed.Reviewed by Cursor Bugbot for commit 942fb17. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit