Add lossless typed vesting readers#433
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughVesting entities now use dedicated generated-wrapper decoding, shared validation and numeric converters, typed reader results, stricter template identity checks, iterative graph validation, and expanded tests for boundary safety, deep graphs, round trips, and type contracts. ChangesVesting decoding and conversion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant VestingReader
participant LedgerJsonApiClient
participant VestingCodec
participant NativeConverter
Client->>VestingReader: request vesting entity
VestingReader->>LedgerJsonApiClient: read contract with expected template
LedgerJsonApiClient-->>VestingReader: contractId and createArgument
VestingReader->>VestingCodec: decode generated wrapper
VestingCodec-->>VestingReader: canonical vesting data
VestingReader->>NativeConverter: convert decoded data
NativeConverter-->>Client: event and contractId
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…er-backed-vesting-readers
|
@coderabbitai pause |
✅ Action performedReviews paused. |
…coder-backed-vesting-readers # Conflicts: # test/converters/valuationVestingConverters.test.ts
…coder-backed-vesting-readers
|
Implementation validation update for head 9bac6bb:
GitHub currently reports the branch mergeable. CodeRabbit is successful; build-and-test and quickstart integration are in progress. The PR remains draft and unassigned, with no review requests added. |
…coder-backed-vesting-readers
…coder-backed-vesting-readers
|
Vesting hardening and final transfer-review propagation are pushed at exact head 5fdf56b on exact base 6f87de5. This revision fixes the independent audit findings:
Exact combined validation:
The PR remains draft and unassigned while fresh build and QuickStart checks run. No reviewers have been requested. |
|
@copilot review Please perform the required formal review of exact current head 5fdf56b on exact stacked base 6f87de5. Both build-and-test and full QuickStart are green, GitHub is CLEAN/MERGEABLE, the PR is unassigned, and there are zero unresolved threads. Focus on exact vesting object_type enforcement, direct and dispatcher trap safety, allocation-versus-graph diagnostic precedence, iterative graph correctness and performance, lossless generated-codec boundaries, and exact public result types. Report every current-head actionable issue before Cursor or CodeRabbit review. |
|
@copilot review Please run the required formal review now for unchanged exact head 5fdf56b. The earlier request overlapped another active Copilot review and produced no response. All checks, including automatic Cursor Bugbot, are now successful; GitHub remains CLEAN/MERGEABLE with zero threads. Review the vesting object_type, trap-safety, diagnostic-precedence, graph correctness/performance, generated-codec, and exact-type changes and report every actionable issue. |
Completed the formal review on head |
|
@cursor review Please perform an explicit review of exact current head 5fdf56b on base 6f87de5. Copilot completed the required formal exact-head review with no actionable findings: #433 (comment). All checks are green, GitHub is CLEAN/MERGEABLE, and there are zero threads. Focus on vesting discriminator enforcement, trap safety, error precedence, iterative graph correctness/performance, and exact types. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 5fdf56b. Configure here.
Completed the formal review on exact head |
|
@coderabbitai review The observed global included-review interval after the completed #432 substantive run has fully elapsed, and no later repository-wide CodeRabbit run has consumed the slot. Please now run and publish the complete substantive review pipeline for exact PR head |
|
✅ Action performedReview finished.
|
|
@coderabbitai review The full 11-minute availability window on Run ID |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/functions/OpenCapTable/vestingTerms/getVestingTermsAsOcf.ts`:
- Around line 423-428: Update the description spread in the common object within
getVestingTermsAsOcf to check c.description with a nullish guard, so both null
and undefined omit the optional description field while defined values remain
included.
In `@test/declarations/vestingReaders.types.ts`:
- Around line 18-20: Update the IsExactly helper in
test/declarations/vestingReaders.types.ts and test/types/vestingReaders.types.ts
to return false whenever either type operand is any, using IsAny before the
bidirectional exactness check. Preserve true results for genuinely identical
non-any types and ensure all existing exactness assertions use the strengthened
helper.
- Around line 88-89: Update the negative type assertion for
wrongPublicVestingTerms to assign a PublicVestingStartData value instead of
null, preserving the `@ts-expect-error` so the test verifies that public
vesting-start data is incompatible with OcfVestingTerms.
In `@test/functions/vestingGraphDepth.test.ts`:
- Around line 104-108: Remove the wall-clock assertion from the distinct-target
traversal test, while retaining the findVestingGraphIssue(...).toBeUndefined()
correctness check. Use the test’s existing Jest timeout configuration to detect
regressions without relying on performance.now() or a fixed 1,500 ms threshold.
🪄 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: dad458b8-2314-4473-99bd-430a3cf3a960
📒 Files selected for processing (32)
src/functions/OpenCapTable/capTable/damlEntityData.tssrc/functions/OpenCapTable/capTable/damlToOcf.tssrc/functions/OpenCapTable/capTable/ocfToDaml.tssrc/functions/OpenCapTable/capTable/vestingContractData.tssrc/functions/OpenCapTable/shared/damlText.tssrc/functions/OpenCapTable/shared/transferWriterValidation.tssrc/functions/OpenCapTable/vestingAcceleration/damlToOcf.tssrc/functions/OpenCapTable/vestingAcceleration/getVestingAccelerationAsOcf.tssrc/functions/OpenCapTable/vestingAcceleration/vestingAccelerationDataToDaml.tssrc/functions/OpenCapTable/vestingEvent/damlToOcf.tssrc/functions/OpenCapTable/vestingEvent/getVestingEventAsOcf.tssrc/functions/OpenCapTable/vestingEvent/vestingEventDataToDaml.tssrc/functions/OpenCapTable/vestingStart/damlToOcf.tssrc/functions/OpenCapTable/vestingStart/getVestingStartAsOcf.tssrc/functions/OpenCapTable/vestingStart/vestingStartDataToDaml.tssrc/functions/OpenCapTable/vestingTerms/createVestingTerms.tssrc/functions/OpenCapTable/vestingTerms/getVestingTermsAsOcf.tssrc/functions/OpenCapTable/vestingTerms/vestingGraphValidation.tssrc/functions/OpenCapTable/vestingTerms/vestingPeriodInteger.tssrc/functions/OpenCapTable/vestingTerms/vestingQuantity.tssrc/utils/templateIdentity.tstest/converters/valuationVestingConverters.test.tstest/declarations/vestingReaders.types.tstest/functions/generatedDamlReaderValidation.test.tstest/functions/vestingBoundarySafety.test.tstest/functions/vestingGraphDepth.test.tstest/functions/vestingReaders.test.tstest/functions/vestingTextRoundtrip.test.tstest/types/vestingReaders.types.tstest/utils/templateIdentity.test.tstest/validation/damlToOcfValidation.test.tstest/validation/generatedDamlBoundary.test.ts
|
CodeRabbit completed its exact-head substantive pass and identified four actionable improvements. Returning this PR to draft while I fix and validate all four; reviewer gates will be rerun on the resulting exact head. |
|
@copilot review Please perform the required formal review of exact current head f57e17a on exact stacked base 6f87de5. This revision addresses all four CodeRabbit findings with resolved threads: nullish omission of optional vesting-condition descriptions plus regression coverage, any-safe exact-type assertions, a real PublicVestingStartData incompatibility assertion, and removal of a flaky wall-clock bound while preserving the 20k correctness test and 30s Jest timeout. Both exact-head checks are green, GitHub is CLEAN/MERGEABLE, the PR is unassigned, and there are zero unresolved threads. Report every current-head actionable issue before Cursor or a final CodeRabbit pass. |
|
@cursor review Please perform an explicit review of exact current head f57e17a on exact stacked base 6f87de5. Copilot completed the required exact-head review with no actionable findings: #433 (comment). Both required checks are green, GitHub is CLEAN/MERGEABLE, the PR is unassigned, and there are zero unresolved threads. Focus on the four review-fix areas: nullish optional-field omission and its regression, any-safe exact-type assertions, the public vesting-start incompatibility assertion, and stable deep-graph correctness testing without a wall-clock threshold. Report every current-head actionable issue. |
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 f57e17a. Configure here.
…aders' into codex/decoder-backed-vesting-readers # Conflicts: # src/functions/OpenCapTable/capTable/damlCodecLosslessness.ts # src/functions/OpenCapTable/capTable/damlEntityData.ts # src/functions/OpenCapTable/capTable/damlToOcf.ts # src/functions/OpenCapTable/shared/plainDataValidation.ts # src/functions/OpenCapTable/shared/transferWriterValidation.ts
HardlyDifficult
changed the title
|
Restacked onto exact #432 head The combined boundary now retains transfer and vesting invariants, detaches generated decoder ownership, preserves structured decoder paths, exposes readonly vesting decoder inputs, and bounds data plus prototype traversal. The restack also caught and fixed transfer non-empty-text and depth-limit regressions before push. Local exact-head validation is green: 106 suites / 6,024 tests, declarations/build/package-consumer checks, lint, formatting, package dry-run, and diff checks. This PR remains draft and unassigned while hosted CI runs and the stack is propagated. |
Summary
Validation
npm run test:ci— 106 suites, 6,024 testsnpm run test:declarationsnpm run lintnpm run formatnpm pack --dry-run— 1,234 files, 567.8 kB packed / 3.2 MB unpackedgit diff --checkStack