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 |
|
@copilot review |
Addressed with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40ee04830b
ℹ️ 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".
|
Reviewed head |
…egory' into codex/ocf-stock-security-balance
…' 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 |
Read-only data/requirement audit — standalone head
|
Exact-head LocalNet replay resultsBoth databases have now been replayed against exact head
The earlier repeatable-read audit explains the new batch failures:
Conclusion: the replay confirms this PR is not ready to merge as written. Narrow it to OCF-supported invariants (unique IDs, existing references, no self-reference, no cycles), then the remaining data issue should be only the two development dangling references, which require an explicit vesting-condition decision rather than an automatic data rewrite. |
Exact-head production replay after narrowingTested contract SHA:
Conclusion: this narrowed PR introduces no new production data issue. Development still has the two documented dangling |
…ph-integrity # Conflicts: # dars/OpenCapTable-v34/0.0.3/OpenCapTable-v34.dar # dars/dars.lock
|
@copilot review Please review exact standalone head |
|
@cursor review Please review exact standalone head |
|
@codex review Please review exact standalone 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 73dadbb. Configure here.
Reviewed exact standalone head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73dadbb794
ℹ️ 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".
Summary
Standalone on
main.This enforces the OCF-supported integrity rules for references inside a
VESTING_TERMScondition graph:next_condition_idsarray is unique and every ID resolves in the same terms objectrelative_to_condition_idresolves in the same terms objectIt deliberately does not treat the first array item as a universal root or require every condition to be reachable from it. It also does not require a relative anchor to be a direct predecessor or dominate every path, and it does not add a global
VESTING_START_DATEcardinality rule. Those assumptions are not universal requirements in the official OCF sources and rejected valid independent acceleration and milestone branches in existing data.OCF basis
VestingTerms.vesting_conditionsdescribes the array as a graph and requires at least one condition.VestingCondition.next_condition_idscontains references to conditions that may trigger next and requires unique array items.VestingScheduleRelativeTrigger.relative_to_condition_idis a reference to another condition.Data audit
The original broader implementation rejected 16/76 development and 2/51 production vesting-terms objects. Fourteen development and both production cases were independent acceleration/milestone branches or array-order patterns that this revision now accepts.
Two development-only objects still contain genuine dangling
relative_to_condition_id = "vesting-start"references and should fail until the source data is corrected:0c5d1a65-9826-47fe-9218-b785dca1a874, object5f79f615-ecb1-4dac-a403-04dc98343e52490ebb25-e314-45d4-bb17-a441f562e73f, object3583e335-c1f9-4412-9ce0-58fd196b29fdAdding a missing condition is not an automatic lossless repair because OCF requires that new condition to specify a vesting
quantityorportionand its graph placement. No database changes were made.The exact LocalNet replays linked in the comments ran against the former broader head and must be rerun for this revision before final review.
Package
mainOpenCapTable-v340.0.330bdc521c9f79340147ef3f68336393f0682bae5Local validation
npm run buildpassed, including compilation of the focused Daml scriptsnpm run check:dar-version-policy -- --base origin/mainpassednpm run verify-darspassed (3/3)npm run lint:damlpassed with no hintsgit diff --checkpassedCurrent standalone readiness evidence
Exact head:
73dadbb794d5d226e1c69ad80c652f59d8d67301Current base:
mainat6ac5d1099104446abfe48b5aac556a90376e3ab7mainnormally; the net PR diff remains four files: vesting graph validation, focused tests, the combined undeployed0.0.3DAR, anddars.lock.eb42ff84fa27f8d7006f40fef367a5597a90c135f2f27b58a6ccc5a57e70d032.0.0.2 -> 0.0.3upgrade compatibility pass.Read-only data audit
vesting-startcondition:0c5d1a65-9826-47fe-9218-b785dca1a874(test), object5f79f615-ecb1-4dac-a403-04dc98343e52490ebb25-e314-45d4-bb17-a441f562e73f(Thresh Power), object3583e335-c1f9-4412-9ce0-58fd196b29fdThe guarded dev-only repair has now appended version 4 for both objects. Each new version only prepends a zero-quantity
VESTING_START_DATEcondition namedvesting-start, pointing tocliff-vesting; independent read-only verification confirms the prior conditions and all other payload fields are unchanged. The latest data audit now finds 0 violations in development and 0 in production.Exact-head LocalNet replay evidence
batch,conversion, andschemafailed, identical to the immediately preceding baseline. The read-only graph audit remains 0/51 violations, so this PR adds no production data issue. Replay duration: 4m43s. Traffic: 3.667 MB, equivalent to $61.14.termination_exercise_windowsproperty. Guarded version 2 rows added the semantically empty value[]and changed no other payload data; a fresh strict-schema audit then found 0 issues across all 7,853 latest dev objects before the passing replay.Note
Medium Risk
Stricter on-ledger validation can block creates/edits that previously passed (e.g. dangling relative refs in dev data); impact is limited to vesting terms integrity, not auth or payments.
Overview
Vesting terms create and edit paths now reject invalid OCF condition graphs via
validateOcfVestingConditionGraphwired intovalidateOcfVestingTermsData(and thus theVestingTermstemplateensure).The new checks require unique condition IDs, unique and resolvable
next_condition_ids, resolvablerelative_to_condition_id(not self), and an acyclic combined graph built fromnext_condition_idsplus relative-trigger dependencies. The rules intentionally do not require a single array root, full reachability, relative anchors as direct predecessors, or a global cap on start-date conditions—matching the narrower OCF-aligned scope described in the PR.Tests add helpers and many Daml scripts for failure cases (duplicates, dangling refs, cycles, bad edits) and for accepted patterns (independent branches, non-root array order, etc.). The OpenCapTable-v34 0.0.3 DAR and
dars.lockare updated to ship the change.Reviewed by Cursor Bugbot for commit 73dadbb. Bugbot is set up for automated code reviews on this repo. Configure here.