Skip to content

#127 SDK hardening#128

Merged
b3y0urs3lf merged 18 commits into
mainfrom
issue-127
Jul 8, 2026
Merged

#127 SDK hardening#128
b3y0urs3lf merged 18 commits into
mainfrom
issue-127

Conversation

@martti007

Copy link
Copy Markdown
Collaborator

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the token split payment protocol by replacing the plain sparse Merkle tree with a radix sparse Merkle sum tree (RSMST) to support value-conserving split allocations, introducing classes like SplitAllocationProof, SplitManifest, and VerificationContext. It also hardens JsonRpcHttpTransport with response size limits and improves CBOR/JSON deserialization. The code review identifies several critical issues: a potential compilation or runtime crash in InclusionProofVerificationRule due to accessing lockScript on ITransaction where it is undefined; a high-severity Denial of Service (infinite loop/OOM) vulnerability in Token.verify when processing circular provenance chains; unhandled exceptions in SplitMintJustificationVerifier.verify that should be caught defensively; a resource leak in JsonRpcHttpTransport when response limits are exceeded without canceling the reader; and missing SHA-256 validation checks in SplitAllocationProof and SplitManifest.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/transaction/verification/rule/InclusionProofVerificationRule.ts
Comment thread src/transaction/Token.ts
Comment thread src/payment/SplitMintJustificationVerifier.ts
Comment thread src/api/json-rpc/JsonRpcHttpTransport.ts
Comment thread src/payment/SplitAllocationProof.ts
Comment thread src/payment/SplitManifest.ts

@MastaP MastaP left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review — correctness & hardening pass

Reviewed the full diff (104 files) with a multi-angle finder + adversarial-verification pass. This is mostly solid defensive work; the notes below are the items worth acting on, left as inline comments and ordered by severity:

  1. Verification gap (highest priority) — the split burn-transfer guard became dead code, letting a genesis mint stand in for the required certified burn.
  2. Three JSON-RPC transport response-validation regressions (spec-compliant id:null/error:null shapes and null-body statuses now throw the wrong error).
  3. TokenSalt wire validation weakened from exact-32 to min-16 with no upper bound.
  4. AggregatorClient constructor now throws on inputs that previously worked.
  5. Two maintainability/perf items in the split path (duplicated consensus-critical commitment formulas; redundant TokenId re-derivation; O(n²) proofs copy).

The CBOR-reader rewrite, BFT certificate parsing, and radixsum tree math verified clean for correctness. Details inline.

Comment thread src/payment/SplitMintJustificationVerifier.ts Outdated
Comment thread src/api/json-rpc/JsonRpcHttpTransport.ts Outdated
Comment thread src/api/json-rpc/JsonRpcHttpTransport.ts
Comment thread src/transaction/TokenSalt.ts Outdated
Comment thread src/api/AggregatorClient.ts Outdated
Comment thread src/payment/SplitAllocationProof.ts
Comment thread src/payment/TokenSplit.ts Outdated
Comment thread src/payment/SplitMintJustificationVerifier.ts Outdated
@martti007
martti007 requested a review from MastaP July 3, 2026 12:11
@MastaP MastaP added this to Unicity Jul 6, 2026
@MastaP MastaP moved this to Test in Unicity Jul 6, 2026
@b3y0urs3lf b3y0urs3lf mentioned this pull request Jul 8, 2026
@b3y0urs3lf
b3y0urs3lf merged commit 470c2cc into main Jul 8, 2026
1 check passed
@github-project-automation github-project-automation Bot moved this from Test to Done in Unicity Jul 8, 2026
@b3y0urs3lf
b3y0urs3lf deleted the issue-127 branch July 8, 2026 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants