Enforce stock plan stock class references#240
Conversation
|
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:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ce-integrity' into codex/ocf-stock-plan-class-reference-integrity
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 98ee063. Configure here.
| # StockTransfer can only reference a stock security, not a warrant security. | ||
| validations: | ||
| # Stock plans - validate referenced stock classes exist | ||
| StockPlan: ['stock_class_ids[]:stock_classes'] |
There was a problem hiding this comment.
Same-batch plan before class fails
Medium Severity
New StockPlan reference checks require each stock_class_ids entry to exist in stock_classes before the plan is created. UpdateCapTable only sorts creates by tier, and both types are tier 1, so a batch that lists OcfCreateStockPlan before OcfCreateStockClass fails even when the class is included in the same batch.
Reviewed by Cursor Bugbot for commit 98ee063. Configure here.
There was a problem hiding this comment.
Pull request overview
Adds contract-level reference validation so StockPlan.stock_class_ids must point to existing StockClass contracts, aligning DAML behavior with the OCF schema’s intent and tightening CapTable integrity.
Changes:
- Configures codegen reference validation for
StockPlan.stock_class_ids[]againststock_classes. - Adds new DAML Script tests covering missing-reference failures, same-batch creation, and edit-time validation.
- Updates an existing stock plan return-to-pool test to create the referenced stock class before creating stock plans; updates packaged DAR + lock metadata.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Test/daml/OpenCapTable/TestStockPlanReturnToPool.daml | Updates test setup to ensure a referenced StockClass exists before creating StockPlans. |
| Test/daml/OpenCapTable/TestStockPlanReferences.daml | Adds new tests for StockPlan→StockClass reference validation on create/edit and same-batch scenarios. |
| scripts/codegen/captable-config.yaml | Enables codegen-driven validation for StockPlan.stock_class_ids[] referencing stock_classes. |
| dars/OpenCapTable-v34/0.0.2/OpenCapTable-v34.dar | Updates the packaged DAR artifact pointer (Git LFS). |
| dars/dars.lock | Updates DAR checksum/size metadata to match the new artifact. |
| # Stock plans - validate referenced stock classes exist | ||
| StockPlan: ['stock_class_ids[]:stock_classes'] | ||
|
|
| edits = [] | ||
| deletes = [] | ||
|
|
||
| testStockPlan_StockClassCreatedInSameBatchSucceeds = script do |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98ee06302f
ℹ️ 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".
| # StockTransfer can only reference a stock security, not a warrant security. | ||
| validations: | ||
| # Stock plans - validate referenced stock classes exist | ||
| StockPlan: ['stock_class_ids[]:stock_classes'] |
There was a problem hiding this comment.
Move StockPlan into a later create tier
With this validation, StockPlan now depends on StockClass, but both remain tier 1 (scripts/codegen/captable-config.yaml lines 13-19) while UpdateCapTable sorts creates only by getCreateTier (scripts/codegen/templates/CapTable.daml.template line 330). If a client submits a valid same-batch create with OcfCreateStockPlan before the referenced OcfCreateStockClass, the stock-class lookup runs before the class is inserted and the batch fails even though the reference is present in the batch. Put StockPlan in a later tier, and move its dependent plan transactions after it, or otherwise sort same-tier creates by dependency.
Useful? React with 👍 / 👎.
|
Superseded by #251, which consolidates the reference-integrity work into one PR. Before closing this stack PR, I rechecked the live review comments and folded the actionable improvements into #251: StockPlan create-tier ordering, document related-object error messages, stock-class conversion source IDs, warrant conversion validation across all OcfAnyConversionRight variants, WarrantIssuance same-batch vesting-term edits, and immutable DAR backup versioning. |


Summary
Stacked on #239.
This adds contract-level reference validation for
StockPlan.stock_class_ids:stock_class_ids[]entry must reference an existingStockClasscontract;OcfCreateStockPlanandOcfEditStockPlan;StockClassandStockPlanare both tier 1 and generated processing ordersStockClassbeforeStockPlan.OCF documentation
StockPlan.stock_class_idsdescribes identifiers ofStockClassobjects: https://github.com/Open-Cap-Table-Coalition/Open-Cap-Format-OCF/blob/main/schema/objects/StockPlan.schema.json#L41-L48stock_class_idor currentstock_class_ids; this DAML model uses the currentstock_class_idsarray: https://github.com/Open-Cap-Table-Coalition/Open-Cap-Format-OCF/blob/main/schema/objects/StockPlan.schema.json#L52-L66Tests
TestStockPlanReferencesfor missing stock class, same-batch stock class + plan creation, and edit-time missing-reference rejection.TestStockPlanReturnToPoolto create the referenced stock class before creating its plans.Validation
NODE_PATH=/Users/fairnick/Documents/code/fairmint/open-captable-protocol-daml/node_modules PATH="/Users/fairnick/Documents/code/fairmint/open-captable-protocol-daml/node_modules/.bin:$HOME/.dpm/bin:$PATH" npm run buildTest/:JAVA_HOME=/opt/homebrew/opt/openjdk@17 PATH="/opt/homebrew/opt/openjdk@17/bin:$HOME/.dpm/bin:$PATH" dpm test --show-coverage --color --coverage-ignore-choice 'splice-amulet:.*'Note
Medium Risk
Tightens cap-table mutation rules so invalid stock plan references fail at the contract; existing flows that create plans without prior stock classes will now reject unless fixed or batched correctly.
Overview
Stock plans must reference real stock classes on create and edit: every
stock_class_ids[]entry is checked against existingStockClasscontracts (aligned with OCFStockPlan.stock_class_ids).Codegen now declares
StockPlan: ['stock_class_ids[]:stock_classes']incaptable-config.yaml, so CapTable batch processing applies the same reference rules as other OCF types. Same-batch updates still work whenStockClassandStockPlanare created together (tier-1 ordering processes classes before plans).Tests: new
TestStockPlanReferencescovers missing class on create, valid same-batch create, and missing class on edit;TestStockPlanReturnToPoolseedsSC_COMMONviaaddDefaultStockClassbefore plan creation.Artifacts:
OpenCapTable-v340.0.2 DAR anddars.lockare refreshed for the rebuilt package.Reviewed by Cursor Bugbot for commit 98ee063. Bugbot is set up for automated code reviews on this repo. Configure here.