Add lossless typed transfer readers#432
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTransfer conversion now uses dedicated generated-codec validation, typed DAML/OCF mappings, defensive plain-data checks, template validation, and expanded runtime and compile-time tests across stock, warrant, equity-compensation, and convertible transfers. ChangesTransfer conversion pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant OCFClient
participant TransferReader
participant GeneratedCodec
participant OCFEvent
OCFClient->>TransferReader: read transfer contract
TransferReader->>GeneratedCodec: validate and decode createArgument
GeneratedCodec-->>TransferReader: decoded transfer_data
TransferReader->>OCFEvent: construct native transfer event
OCFEvent-->>OCFClient: 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 |
…-transfer-readers
…to codex/decoder-backed-transfer-readers
|
@coderabbitai pause |
✅ Action performedReviews paused. |
…coder-backed-transfer-readers
…coder-backed-transfer-readers
|
Transfer hardening update pushed at Implemented:
Local validation on the required
The PR remains draft and unassigned. The attempted merge of the now-stale lower-stack head was aborted cleanly; this branch is waiting for the corrected #431 exact head before its final parent merge. |
|
Transfer hardening update pushed at .\n\nImplemented:\n- Transplanted the bounded diagnostics and trap-free plain-data foundation used by the later stack.\n- Routed all four transfer direct decoders, dispatcher paths, generated wrapper decoders, and ledger readers through preflight plus generated-codec losslessness.\n- Made all four writers symmetric: exact DAML Numeric(10) limits, uppercase currency validation, unique required result arrays, schema-valid empty uid=501(fairnick) gid=20(staff) groups=20(staff),12(everyone),61(localaccounts),79(_appserverusr),80(admin),81(_appserveradm),701(com.apple.sharepoint.group.1),33(_appstore),98(_lpadmin),100(_lpoperator),204(_developer),250(_analyticsusers),395(com.apple.access_ftp),398(com.apple.access_screensharing),399(com.apple.access_ssh),400(com.apple.access_remote_ae) and , present empty optional strings, and byte-preserved comments.\n- Added 46 focused round-trip and adversarial boundary tests, including accessor/Proxy zero-invocation assertions.\n\nLocal validation on the required :\n- Copied OCF schemas to /Users/fairnick/Documents/code/fairmint/.worktrees/ocp-canton-sdk-transfer-readers/dist/ocf-schema — passed\n- -----------------------------------------------------------------|---------|----------|---------|---------|---------------------------------------------------------------------------------------------------------
|
…coder-backed-transfer-readers # Conflicts: # src/errors/OcpContractError.ts # src/errors/OcpError.ts # src/errors/OcpNetworkError.ts # src/errors/OcpParseError.ts # src/errors/OcpValidationError.ts # src/functions/OpenCapTable/capTable/damlEntityData.ts # src/functions/OpenCapTable/capTable/ocfToDaml.ts # src/functions/OpenCapTable/convertibleTransfer/convertibleTransferDataToDaml.ts # src/functions/OpenCapTable/convertibleTransfer/damlToOcf.ts # src/functions/OpenCapTable/convertibleTransfer/getConvertibleTransferAsOcf.ts # src/functions/OpenCapTable/equityCompensationTransfer/equityCompensationTransferDataToDaml.ts # src/functions/OpenCapTable/equityCompensationTransfer/getEquityCompensationTransferAsOcf.ts # src/functions/OpenCapTable/shared/singleContractRead.ts # src/functions/OpenCapTable/stockTransfer/createStockTransfer.ts # src/functions/OpenCapTable/stockTransfer/getStockTransferAsOcf.ts # src/functions/OpenCapTable/warrantTransfer/getWarrantTransferAsOcf.ts # src/functions/OpenCapTable/warrantTransfer/warrantTransferDataToDaml.ts # src/utils/typeConversions.ts # test/batch/damlToOcfDispatcher.test.ts # test/converters/convertibleIssuanceConverters.test.ts # test/converters/valuationVestingConverters.test.ts # test/functions/damlEntityDataLosslessness.test.ts
|
Final corrected-parent merge pushed at Ancestry:
Semantic conflict resolution:
Final local validation:
The PR remains draft and unassigned with no review requests. GitHub CI and QuickStart are queued on this exact head. |
|
@copilot review Please perform the required formal review of exact current head |
Formal review completed on exact head No high-confidence bugs, regressions, or security issues were found in the requested areas:
Verification performed:
|
|
@cursor review Please perform an explicit exact-head review of |
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 2991df8. Configure here.
|
@coderabbitai review Please run and publish the complete substantive review pipeline for exact current head |
|
✅ Action performedReview finished.
|
|
@coderabbitai review The adaptive included-review window shown by the prior rate-limited sticky has fully elapsed. Please now run and publish the complete substantive review pipeline for exact current head |
|
If you believe the earlier pass at this head was truncated or incomplete (rather than simply "already reviewed, nothing to add"), the most reliable way to force a full fresh pass is either:
I'll trigger the full review now given the explicit request for complete pipeline coverage across all 34 files. ✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 4 seconds. |
|
@coderabbitai full_review The final 2-second adaptive window shown on Run ID |
|
If you're looking for a genuinely fresh pass with updated eyes on the code, the only way to guarantee that is pushing a new commit (even a no-op rebase) to get a new head SHA. ✅ Action performedFull review finished. |
|
Material exact-boundary fix pushed at The shared canonical writer validator no longer synthesizes a missing Validation on this exact revision:
All reviews of |
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/capTable/transferContractData.ts`:
- Around line 159-170: Update requireOwnFields so transferDecodeError receives a
decoderPath extended with the missing field, matching
validateTransferDamlDataInput’s input.${field} path construction. Keep the
existing required-own-property validation and message unchanged while ensuring
context.decoderPath identifies the specific absent field.
In `@src/functions/OpenCapTable/convertibleTransfer/damlToOcf.ts`:
- Around line 30-33: The amount conversion in convertibleTransferDataToOcf must
enforce the same nonnegative monetary invariant as
convertibleTransferDataToDaml. Replace the plain requireDecimalString validation
for decoded.amount.amount with the existing nonnegative monetary validation used
by requireMonetary, while preserving the existing currency validation and field
mapping.
In `@src/functions/OpenCapTable/shared/singleContractRead.ts`:
- Around line 217-226: Update the pendingResponse handling in the
single-contract read flow to accept only an exact native Promise before
awaiting; reject or route Promise subclasses through the unsafe-response path so
await cannot access a hostile then getter before assertSafeLedgerResponse(). Add
a regression test covering a branded Promise subclass with a hostile then
accessor.
In `@test/declarations/transferReaders.types.ts`:
- Around line 58-59: Update the negative type assertion for
wrongPublicConvertible to assign a declared PublicStockData value instead of
null, ensuring `@ts-expect-error` specifically verifies that root OcpClient
stock-transfer data is incompatible with OcfConvertibleTransfer.
🪄 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: 7ff6791e-6bb6-4f90-9af3-b1a22c71700d
📒 Files selected for processing (34)
src/errors/OcpNetworkError.tssrc/functions/OpenCapTable/capTable/damlEntityData.tssrc/functions/OpenCapTable/capTable/damlToOcf.tssrc/functions/OpenCapTable/capTable/ocfToDaml.tssrc/functions/OpenCapTable/capTable/transferContractData.tssrc/functions/OpenCapTable/convertibleTransfer/convertibleTransferDataToDaml.tssrc/functions/OpenCapTable/convertibleTransfer/damlToOcf.tssrc/functions/OpenCapTable/convertibleTransfer/getConvertibleTransferAsOcf.tssrc/functions/OpenCapTable/equityCompensationTransfer/damlToOcf.tssrc/functions/OpenCapTable/equityCompensationTransfer/equityCompensationTransferDataToDaml.tssrc/functions/OpenCapTable/equityCompensationTransfer/getEquityCompensationTransferAsOcf.tssrc/functions/OpenCapTable/shared/damlText.tssrc/functions/OpenCapTable/shared/ocfWriterValidation.tssrc/functions/OpenCapTable/shared/plainDataValidation.tssrc/functions/OpenCapTable/shared/singleContractRead.tssrc/functions/OpenCapTable/shared/transferWriterValidation.tssrc/functions/OpenCapTable/stockTransfer/createStockTransfer.tssrc/functions/OpenCapTable/stockTransfer/damlToOcf.tssrc/functions/OpenCapTable/stockTransfer/getStockTransferAsOcf.tssrc/functions/OpenCapTable/warrantTransfer/damlToOcf.tssrc/functions/OpenCapTable/warrantTransfer/getWarrantTransferAsOcf.tssrc/functions/OpenCapTable/warrantTransfer/warrantTransferDataToDaml.tssrc/utils/typeConversions.tstest/batch/damlToOcfDispatcher.test.tstest/converters/schemaCardinalityBoundaries.test.tstest/converters/transferWriterRoundtrip.test.tstest/declarations/transferConverterInputs.types.tstest/declarations/transferReaders.types.tstest/errors/errors.test.tstest/functions/damlEntityDataLosslessness.test.tstest/functions/transferReaders.test.tstest/types/transferConverterInputs.types.tstest/types/transferReaders.types.tstest/utils/typeConversions.test.ts
💤 Files with no reviewable changes (2)
- src/utils/typeConversions.ts
- test/utils/typeConversions.test.ts
|
CodeRabbit findings addressed on exact head Implemented:
Validation on this exact revision:
The PR remains draft and unassigned. Fresh GitHub build and QuickStart are running; all earlier automated reviews are stale. |
|
@copilot review Please perform the required formal review of exact current head 6f87de5 after the CodeRabbit follow-up fixes. Both build-and-test and the full 8m20s QuickStart run are green, GitHub is CLEAN/MERGEABLE, the PR is unassigned, and all review threads are resolved. Focus on exact native Promise handling, hostile Promise-subclass rejection without then access, field-level decoder paths, nonnegative convertible-transfer monetary symmetry, and the declaration incompatibility assertion. Report every current-head actionable issue before Cursor or another CodeRabbit review. |
Formal review completed on exact head No actionable issues found in the requested areas:
Verification:
|
|
@cursor review Please review exact current head 6f87de5 after the four CodeRabbit fixes. Copilot completed the required formal exact-head review with no actionable findings: #432 (comment). Both required checks remain green, GitHub is CLEAN/MERGEABLE, the PR is unassigned, and all threads are resolved. Focus on exact native Promise handling, hostile Promise-subclass trap safety, field-level decoder diagnostics, nonnegative transfer amount symmetry, and exact declarations. |
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 6f87de5. Configure here.
|
@coderabbitai review Please run and publish the substantive incremental review for exact current head 6f87de5, covering every file changed since your prior reviewed head 2991df8. Verify the four fixes you requested: precise wrapper decoderPath, nonnegative convertible-transfer read amounts, exact-native-Promise await safety with hostile subclass regression, and the PublicStockData declaration assertion. Copilot and Cursor are exact-head clean, both required checks are green, GitHub is CLEAN/MERGEABLE, and all threads are resolved. Report every actionable finding. |
|
✅ Action performedReview finished.
|
|
@coderabbitai review The full 20-minute adaptive window on Run ID |
|
If a genuinely fresh, from-scratch pass over this exact head is what you need (to double check the four fixes — wrapper I'll trigger the review command once more now for completeness. ✅ Action performedReview finished.
|
|
Ready for Nick final review at exact head 6f87de5 on exact stacked base 40428ed. Final live gate:
Assigned to @HardlyDifficult for final human review. No merge was performed because the PR has no standing thumbs-up authorization. |
…coder-backed-transfer-readers
|
Parent propagation is complete at exact head de5d089 via a normal conflict-free merge of #431 855d5c1. The transfer slice retained an identical full-index patch fingerprint, with zero overlap between parent and child files. Exact validation passes: 7 focused suites / 361 tests, full 99 suites / 5,829 tests, typecheck, lint, formatting, declarations, exact-public/cardinality, package-consumer, and package dry-run gates. Independent semantic inspection found no issue. Keeping this PR draft/unassigned until the upstream #417-derived chain reaches it and final exact-head CI/reviewer gates can restart. |
…to codex/decoder-backed-transfer-readers # Conflicts: # src/functions/OpenCapTable/capTable/batchTypes.ts # src/functions/OpenCapTable/capTable/damlEntityData.ts # src/functions/OpenCapTable/capTable/damlToOcf.ts # src/functions/OpenCapTable/capTable/entityTypes.ts # src/functions/OpenCapTable/capTable/ocfToDaml.ts # src/utils/replicationHelpers.ts # src/utils/typeConversions.ts # test/functions/damlEntityDataLosslessness.test.ts # test/schemaAlignment/canonicalOcfObjectInventory.json # test/types/capTableBatch.types.ts
|
Restacked onto the current lossless-decoder parent and hardened exact head The merge exposed and fixed a schema-diagnostic regression for explicit Local exact-head validation is green: 101 suites / 5,973 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 remaining stack is restacked in order. |
Summary
Validation
npm run test:ci— 101 suites, 5,973 testsnpm run test:declarationsnpm run lintnpm run formatnpm pack --dry-run— 1,230 files, 561.4 kB packed / 3.1 MB unpackedgit diff --checkStack