Enforce vesting condition graph integrity#254
Conversation
📝 WalkthroughWalkthroughThis PR adds graph-level validation to Daml vesting terms, covering condition references, roots, reachability, relative-trigger dominance, and cycles. It also bumps the package to 0.0.13, updates the Test DAR dependency, refreshes DAR metadata, and adds create/edit validation tests. ChangesVesting condition graph validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestVestingTerms
participant CT.UpdateCapTable
participant validateOcfVestingTermsData
participant validateOcfVestingConditionGraph
TestVestingTerms->>CT.UpdateCapTable: submit vesting terms create or edit
CT.UpdateCapTable->>validateOcfVestingTermsData: validate vesting_conditions
validateOcfVestingTermsData->>validateOcfVestingConditionGraph: validate condition graph
validateOcfVestingConditionGraph->>validateOcfVestingConditionGraph: check references, reachability, dominance, and cycles
validateOcfVestingConditionGraph-->>validateOcfVestingTermsData: return graph validity
validateOcfVestingTermsData-->>CT.UpdateCapTable: return overall validation result
CT.UpdateCapTable-->>TestVestingTerms: succeed or fail
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@copilot review |
Addressed in |
![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: 7e6d657d24
ℹ️ 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".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@OpenCapTable-v34/daml/Fairmint/OpenCapTable/OCF/VestingTerms.daml`:
- Around line 260-268: `vestingConditionHasCycle` currently only uses the
current path, so shared subgraphs are re-walked across branches and can explode
exponentially. Update the DFS in `VestingTerms.daml` to thread an additional
accumulated visited set alongside `path`, marking nodes that have already been
fully explored with no cycle. Use that visited set in the recursive
`vestingConditionHasCycle`/`conditionById` traversal so each
`OcfVestingCondition` is expanded at most once while still detecting back-edges
via the current path.
- Around line 254-268: The edge construction in vestingConditionEdgeIds is
reversed for OcfVestingScheduleRelativeTrigger.relative_to_condition_id, which
makes valid relative chains look cyclic. Update vestingConditionEdgeIds so the
referenced condition points to the current condition instead of appending
relative_to_condition_id to the current node’s outgoing edges, and keep
vestingConditionHasCycle using the corrected graph shape so
validateOcfVestingConditionGraph no longer flags normal schedules as cycles.
🪄 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: 2de591c1-302a-4c30-b9a2-1920dae126e1
⛔ Files ignored due to path filters (1)
dars/dars.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
OpenCapTable-v34/daml.yamlOpenCapTable-v34/daml/Fairmint/OpenCapTable/OCF/VestingTerms.damlTest/daml.yamlTest/daml/OpenCapTable/TestVestingTerms.damldars/OpenCapTable-v34/0.0.7/OpenCapTable-v34.dar
|
@copilot review |
Addressed in |
…y' into codex/ocf-vesting-graph-integrity
|
@copilot review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
…' into codex/ocf-vesting-graph-integrity
|
@copilot review Please review exact head |
Reviewed exact head |
|
@codex review exact head @coderabbitai resume Copilot completed this exact-head review with no findings; CI is green and all review threads are clear. |
|
@cursor review Please review 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 0ae9f4b. Configure here.
|
Codex Review: Didn't find any major issues. Swish! 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". |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@OpenCapTable-v34/daml/Fairmint/OpenCapTable/OCF/VestingTerms.daml`:
- Around line 232-235: The vestingGraphRootConditionId function must identify
the graph’s start condition from its structure rather than selecting
conditions[0]. Derive the root using the existing ID-linked relationships, such
as the node with no incoming edges or the start-trigger condition, while
preserving None for an empty or rootless graph and ensuring
allConditionsReachableFromStart validates unordered condition lists correctly.
🪄 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: 42ba6793-0bcd-4343-b81d-737b16d05f16
⛔ Files ignored due to path filters (1)
dars/dars.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
OpenCapTable-v34/daml.yamlOpenCapTable-v34/daml/Fairmint/OpenCapTable/OCF/VestingTerms.damlTest/daml.yamlTest/daml/OpenCapTable/TestVestingTerms.damldars/OpenCapTable-v34/0.0.13/OpenCapTable-v34.dar
|
✅ Action performedReviews resumed. Review finished.
|
…' into codex/ocf-vesting-graph-integrity
|
Restacked onto the final reviewed head of #253 ( Exact head: Fresh validation on the combined branch:
Keeping this PR in draft until exact-head CI is green. |
|
CI is green and all 16 review threads are resolved on exact head |
Reviewed exact head |
|
@codex review exact head |
|
@cursor review Please review exact head |
|
@coderabbitai review exact head |
|
✅ Action performedReview finished.
|
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 1591822. Configure here.
|
Codex Review: Didn't find any major issues. Breezy! 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". |
Preliminary OCF validation-policy auditPolicy: OCF Validation Policy Appears supported
Needs line-by-line evidence or narrowing
Proposed next step |
Summary
Stacked on #253.
This enforces OCF vesting-condition graph integrity inside
VestingTermsvalidation. A vesting terms object now rejects final data where:next_condition_idscontains duplicate IDsnext_condition_idsreferences a missing vesting conditionvesting_conditionsentry, which OCF treats as the graph rootVESTING_STARTcondition is presentVESTING_SCHEDULE_RELATIVE.relative_to_condition_idreferences a missing condition or itselfnext_condition_idspathThe check runs through
validateOcfVestingTermsData, so it applies to both creates and edits.OCF requirement references
Source checked against Open-Cap-Format-OCF at
dd84bdb6bafb63681c77166f315cea5a2078a4c8.VestingTerms.v1.vesting_conditionsdescribes conditions and triggers as the graph of vesting schedules and events.VestingCondition.idis the reference identifier for a condition.VestingCondition.next_condition_idslists all condition IDs that can trigger after the current condition and requires unique IDs.VestingScheduleRelativeTrigger.relative_to_condition_idis a reference to another vesting condition ID.Review feedback carried forward
Set-based visited state to avoid repeated work on larger DAGs.Stack/version
0.0.13dars.lockentries for the stacked category branch.Validation
Exact head:
1591822700805a917da2173f3e87c3d17c2a2c2bnpm run buildnpm test(complete suite; 610 test scripts: 608 behavioral + 2 setup)npm run lint(0 errors; 2 pre-existing warnings inscripts/remove-participant-dar.ts)npm run lint:damlnpm run formatnpm run check:schema-gaps -- --ocf-schema-dir /tmp/open-cap-format-ocf-schema-check/schemanpm run verify-darsnpm run verify-packagenpm run check-upgrade-compat(0.0.12 -> 0.0.13)git diff --checkOpenCapTable-v34-0.0.13.dar: SHA-2564caa07fd6391427d8c83303f29f4f63b92a4edd7044c8489212442e9bf647c50, 3,192,596 bytesNote
Medium Risk
Tightens on-ledger validation for cap-table vesting data; legitimate schedules that violate the new graph rules will fail create/edit, but logic is localized to VestingTerms with extensive tests.
Overview
Vesting terms now reject invalid OCF vesting-condition graphs at template
ensuretime via a newvalidateOcfVestingConditionGraphstep wired intovalidateOcfVestingTermsData(creates and edits).The checks cover unique condition IDs, unique
next_condition_ids, known references, reachability from the first condition as graph root, at most oneVESTING_START, relative-trigger predecessor and anchor-dominance rules, and acyclicity (including edges implied byrelative_to_condition_id).OpenCapTable-v34 is bumped to 0.0.13 with an updated test DAR dependency, regenerated packaged DAR/
dars.lock, and broad Daml Script coverage for invalid graphs plus one edit-path regression.Reviewed by Cursor Bugbot for commit 1591822. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests