Enforce authorized and reserved share ceilings#238
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates the CapTable DAML template to validate issuer, stock-class, and stock-plan authorized-share ceilings from historical data and batch edits. Also bumps package/DAR references, adds new test helpers, and expands test coverage for issuer, stock-class, stock-plan, conversion, warrant, and equity-compensation flows. ChangesAuthorized Shares Ceiling Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UpdateCapTable
participant processEdit
participant validateAuthorizedShareCeilings
participant CapTable
UpdateCapTable->>processEdit: apply edits and collect finalized maps
UpdateCapTable->>validateAuthorizedShareCeilings: validate issuer, stock-class, and stock-plan ceilings
validateAuthorizedShareCeilings-->>UpdateCapTable: pass or fail
UpdateCapTable->>CapTable: create updated contract
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds contract-level enforcement of stock-class authorized share ceilings during CapTable.UpdateCapTable, ensuring issuances and authorized-share changes cannot result in issued shares exceeding the effective authorized limit (while leaving non-numeric/UNLIMITED values uncapped).
Changes:
- Adds authorized-share limit calculation + validation helpers to the generated
CapTablemodule template. - Updates codegen loop templates to invoke the new validations on create/edit/delete paths for StockIssuance and StockClassAuthorizedSharesAdjustment, and on StockClass edits.
- Adds a new DAML script test module covering several authorized-share ceiling scenarios; updates the versioned DAR pointer + lock metadata.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Test/daml/OpenCapTable/TestStockClassAuthorizedShares.daml | Adds new script tests for authorized-share ceiling enforcement scenarios. |
| scripts/codegen/templates/loops/edit-case.daml | Adds validation hooks for edits (stock class, stock issuance, authorized-share adjustments). |
| scripts/codegen/templates/loops/delete-case.daml | Adds validation hook for deleting authorized-share adjustments. |
| scripts/codegen/templates/loops/create-case.daml | Adds validation hooks for creating stock issuances and authorized-share adjustments. |
| scripts/codegen/templates/CapTable.daml.template | Introduces helper functions to compute effective authorized limits and assert issued shares remain within them. |
| scripts/codegen/generate-captable.ts | Plumbs per-type boolean flags to drive the new validation hooks in templates. |
| dars/OpenCapTable-v34/0.0.2/OpenCapTable-v34.dar | Updates the Git LFS pointer for the generated DAR artifact. |
| dars/dars.lock | Updates checksum/size metadata for the updated DAR artifact. |
![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: 76fb636f4e
ℹ️ 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".
HardlyDifficult
changed the title
|
@copilot review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/Setup.daml (1)
44-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM on the authorized-shares bump; consider reusing the new helper.
The bump to
100_000_000.0is a reasonable fix to keep this shared setup from tripping the new issuer ceiling checks. SinceTestHelpers.defaultIssuerWithAuthorizedSharesnow exists and duplicates this exactIssuerOcfDataliteral, this construction could be replaced with a call to that helper to avoid the two definitions drifting apart.♻️ Possible simplification
capTableCid <- submit issuer do exerciseCmd authCid CreateCapTable with - issuer_data = IssuerOcfData with - id = "ISSUER_1" - legal_name = "TestIssuer Inc." - formation_date = time (DA.Date.date 2020 Jan 15) 12 0 0 - country_of_formation = "US" - dba = None - country_subdivision_of_formation = None - country_subdivision_name_of_formation = None - tax_ids = [] - email = None - phone = None - address = None - initial_shares_authorized = Some (OcfInitialSharesNumeric 100_000_000.0) - comments = [] + issuer_data = defaultIssuerWithAuthorizedShares "ISSUER_1" (Some (OcfInitialSharesNumeric 100_000_000.0))🤖 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/Setup.daml` around lines 44 - 58, The `exerciseCmd authCid CreateCapTable` setup duplicates the same `IssuerOcfData` literal now available in `TestHelpers.defaultIssuerWithAuthorizedShares`, so replace the inline issuer construction with that helper to keep the shared test setup consistent and avoid drift. Update the `Setup.daml` fixture to use the helper for the issuer payload while preserving the authorized-shares value and surrounding cap table setup.
🤖 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 398-409: `stockClassAuthorizedAdjustmentIndex` is being recomputed
in both `validateAuthorizedShareCeilings` and
`validateAuthorizedShareCeilingsForEdits` on the same `CapTableMaps` input.
Compute the adjustment index once in `UpdateCapTable`, then thread that value
into both validator calls by updating `validateAuthorizedShareCeilings` and
`validateAuthorizedShareCeilingsForEdits` to accept the precomputed index
instead of recalculating it.
- Around line 227-304: `stockClassIssuedQuantity` is repeatedly rebuilding share
events for every issuance, which makes `UpdateCapTable` far more expensive than
necessary. Refactor `stockSecurityShareEvents` and
`stockIssuanceCurrentQuantity` so they read from precomputed or indexed event
collections instead of scanning all `Map.values` for cancellations, repurchases,
retractions, and splits on every call. Use `CapTableMaps` as the home for the
new indexes, similar to `stock_issuances_by_security_id`, and update the paths
that call `validateAllStockClassAuthorizedShares` to consume the indexed
lookups.
In `@Test/daml/OpenCapTable/TestIssuerAuthorizedShares.daml`:
- Around line 12-15: The helper definitions duplicated here and in
TestStockClassAuthorizedShares.daml should be consolidated so both test suites
use one shared implementation. Move the shared helpers, especially
limitedStockClass and stockClassAuthorizedSharesAdjustment, into
TestHelpers.daml and update both test files to import and use those shared
functions instead of maintaining separate copies.
---
Outside diff comments:
In `@Test/daml/OpenCapTable/Setup.daml`:
- Around line 44-58: The `exerciseCmd authCid CreateCapTable` setup duplicates
the same `IssuerOcfData` literal now available in
`TestHelpers.defaultIssuerWithAuthorizedShares`, so replace the inline issuer
construction with that helper to keep the shared test setup consistent and avoid
drift. Update the `Setup.daml` fixture to use the helper for the issuer payload
while preserving the authorized-shares value and surrounding cap table setup.
🪄 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: 19e0da66-0fa6-41e4-b04e-f98f06b62101
⛔ Files ignored due to path filters (1)
dars/dars.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
OpenCapTable-v34/daml.yamlTest/daml.yamlTest/daml/OpenCapTable/Setup.damlTest/daml/OpenCapTable/TestHelpers.damlTest/daml/OpenCapTable/TestIssuerAuthorizedShares.damlTest/daml/OpenCapTable/TestStockClassAuthorizedShares.damldars/OpenCapTable-v34/0.0.3/OpenCapTable-v34.darscripts/codegen/generate-captable.tsscripts/codegen/templates/CapTable.daml.template
|
@copilot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c68c7ce06
ℹ️ 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".
|
@codex review Please review exact head |
|
@cursor review Please review exact head |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 15aab77. Configure here.
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@cursor review Please reconcile the prior unresolved-issue signal against exact head |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 15aab77. Configure here.
|
Readiness audit on exact head That finding is disproved by current exact-head behavior: |
|
@coderabbitai review Please perform a substantive review of exact head |
|
✅ Action performedReview finished.
|
|
@coderabbitai Please explicitly confirm whether the fresh review you just completed on exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Find PR review comments authored by coderabbitai on PR 238
gh api repos/Fairmint/open-captable-protocol-daml/pulls/238/comments --paginate \
--jq '.[] | select(.user.login=="coderabbitai" or .user.login=="coderabbitai[bot]") | {id, commit_id, path, line, created_at, body: (.body[0:200])}'
echo "---REVIEWS---"
gh api repos/Fairmint/open-captable-protocol-daml/pulls/238/reviews --paginate \
--jq '.[] | select(.user.login=="coderabbitai" or .user.login=="coderabbitai[bot]") | {id, commit_id, state, submitted_at, body: (.body[0:300])}'Length of output: 6852 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Check resolution status of review threads, focusing on the one tied to commit 15aab77 (id 3551731544)
gh api graphql -f query='
query {
repository(owner: "Fairmint", name: "open-captable-protocol-daml") {
pullRequest(number: 238) {
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
comments(first: 1) {
nodes {
databaseId
path
body
originalCommit { oid }
createdAt
author { login }
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].author.login=="coderabbitai" or .comments.nodes[0].author.login=="coderabbitai[bot]") | {isResolved, isOutdated, path: .comments.nodes[0].path, commit: .comments.nodes[0].originalCommit.oid, created: .comments.nodes[0].createdAt, id: .comments.nodes[0].databaseId}'Length of output: 2514
I checked the full history of review threads I've posted on this PR directly via the API:
So, to directly answer your question: no, I did not find any new actionable issues on exact head ✅ Action performedReview finished.
|
|
@cursor review exact head |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 15aab77. Configure here.
|
Exact-head readiness recheck for |
|
Feature/test follow-up is complete at exact commit This adds cap-table-wide OCF object-ID uniqueness across the issuer and all 47 supported OCF object/transaction types. Validation reads the underlying OCF data IDs from final state, so mismatched map keys cannot hide a duplicate. Coverage includes same-batch and prior-batch cross-type collisions, issuer-ID reuse, corrupted/upgraded state with duplicate underlying IDs, and valid delete-then-reuse behavior. Local validation on this feature commit passed: No package version, DAR backup, dependency, workflow-policy, or release-policy files changed in this follow-up. The PR intentionally remains draft and unassigned while the existing |
…ormalization # Conflicts: # dars/OpenCapTable-v34/0.0.3/OpenCapTable-v34.dar # scripts/codegen/templates/CapTable.daml.template
|
Updated to exact head 48a4be2 after merging current main. OpenCapTable-v34 is normalized to the single 0.0.2 candidate anchored to deployment tag 0.0.1; superseded branch-only 0.0.4 and 0.0.5 backups were removed while retained history remains intact. The OCF object-ID validation was also corrected to match the spec: uniqueness is enforced within each object type, while cross-type reuse is allowed. Local validation passed: full build and test suite, DAML lint, DAR policy tests, integrity verification, and the 0.0.1 to 0.0.2 upgrade check. Kept draft while fresh CI runs. |
|
@copilot review |
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@copilot review |
Preliminary OCF validation-policy auditPolicy: OCF Validation Policy The current standard has two layers: formal schema validity and documented OCF data-model validity. A rule can be required even when JSON Schema cannot express it. Appears supported
Needs line-by-line evidence or narrowing
Proposed next step |
Summary
Groups the OCF authorized/reserved share ceiling and security-identity enforcement into one contract-level invariant PR. This supersedes #252, the standalone stock-plan reserved pool ceiling PR, and #270, the standalone security-ID uniqueness PR.
UpdateCapTablenow validates the final cap-table state after deletes, edits, issuer edits, and creates, rejecting states where:UNLIMITEDorNOT_APPLICABLE)Valid returns with a recorded source class may target a different stock plan containing that class, as required for OCF plan rollovers; the returned quantity is credited to the target plan while the source plan's original allocation remains consumed. Validated cross-plan credit may be carried across updates until later allocation, with a precise lower bound that does not weaken same-plan over-return checks. Successor attribution is deterministic from the submitted final state: a recorded successor plan must match any plan inherited from its source lineage, while a successor from a non-plan source may explicitly enter a valid plan.
Also keeps fixture data internally valid by removing unrelated placeholder
stock_plan_idvalues from tests that do not create stock plans.OCF requirement references
Schema references are pinned to Open-Cap-Format-OCF commit
dd84bdb6bafb63681c77166f315cea5a2078a4c8.Transaction.dateis required and defines when a transaction occurred.SecurityTransaction.security_ididentifies an issued security for all future transactions.Transfer.resulting_security_idsandbalance_security_ididentify new successor securities and the distinct remainder security.Reissuance.resulting_security_idsidentifies new replacement securities.Issuer.initial_shares_authorizeddefines the issuer's starting authorized shares.StockClass.initial_shares_authorizeddefines each stock class's starting authorized shares.IssuerAuthorizedSharesAdjustment.new_shares_authorizeddefines issuer authorization changes.StockClassAuthorizedSharesAdjustment.new_shares_authorizeddefines stock-class authorization changes.StockIssuance.quantity,StockConversion.quantity_converted,StockCancellation.quantity, andStockClassSplit.split_ratioprovide the quantities that affect stock-class issued totals and split-adjusted plan-backed stock allocations.StockPlan.initial_shares_reserveddefines each stock plan's starting reserved pool.StockPlan.default_cancellation_behaviordescribes cancellation behavior for plan-backed securities.StockPlanPoolAdjustment.shares_reserveddefines reserved pool changes.EquityCompensationIssuance.stock_plan_idandquantitydefine plan-backed equity-comp allocations.StockPlanReturnToPoolrecords shares returned upon cancellation. Itsstock_plan_idandquantityidentify the quantity and destination plan, which may differ from the issuing plan for rollovers.ConvertibleConversion.quantity_converted,trigger_id, and resulting security IDs describe the converted units, selected conversion right, and output securities.FixedAmountConversionMechanism.converts_to_quantityandRatioConversionMechanism.ratiodefine computable resulting share quantities.Validation
npm run buildnpm run verify-darsdpm test --files daml/OpenCapTable/TestStockPlanReservedShares.daml --colordpm test --files daml/OpenCapTable/TestStockClassAuthorizedShares.daml --colordpm test --files daml/OpenCapTable/TestConvertibleConversion.daml --colordpm test --files daml/OpenCapTable/TestSecurityIdValidation.daml --colordpm test --files daml/OpenCapTable/TestLifecycleInvariants.daml --colordpm test --files daml/OpenCapTable/TestIssuerAuthorizedShares.daml --colordpm test --files daml/OpenCapTable/TestStockPlanReturnToPool.daml --colornpm test(388 scenarios)npm run lintnpm run lint:damlnpm run formatnpm run check:schema-gaps -- --ocf-schema-dir /tmp/open-cap-format-ocf-schema-check/schemanpm run verify-packagenpm run check-upgrade-compatgit diff --checkFinal
OpenCapTable-v340.0.5DAR: SHA-256b99c41fe3a2eb3c3d4ce21febfc7568c90c7e9a509d06cb66db924570435c50f(2,962,199bytes).Note
Low Risk
Production change is a narrow OCF field constraint on consolidations; remaining edits are tests and fixtures with no runtime behavior change beyond that validator.
Overview
Adds
validateUniqueStringsArrayin shared validation and applies it to stock consolidationsecurity_ids, so duplicate source IDs fail at the templateensurelayer.The bulk of the diff is Daml Script test work: new
TestIssuerAuthorizedShares,TestLifecycleInvariants, andTestObjectIdUniquenessmodules, plus broaderTestSecurityIdValidationcases.TestHelpersandSetupgain reusable issuer/stock-class/plan factories and setup combinators.Existing scenario tests are updated so batches stay valid under stricter cap-table rules—e.g. materialized resulting/balance stock issuances for convertible conversions, equity releases, and partial cancellations;
stock_plan_id = Nonewhere no plan exists; andaddPrerequisites/setupForStockIssuanceinstead of stakeholder-only setup.Reviewed by Cursor Bugbot for commit 48a4be2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes