fix(circuits): add range constraints and isMint burn path#99
Merged
Conversation
UTXOSettlement circuit: - Add Num2Bits(64) constraint on amount (prevents >64-bit values) - Add Num2Bits(160) constraint on recipient and settlementModule - Add Num2Bits(64) constraint on chainId - 1050 non-linear constraints (up from 602) MARKPool: - withdraw() now accepts isMint param (true=mint to recipient, false=burn from recipient) - Document settlementModule binding is circuit-enforced Trusted setup rerun for updated circuit. Verification key updated. 9 witness tests passing. 84 Solidity unit tests passing.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![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: 0036d832c1
ℹ️ 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".
WalkthroughThe PR adds formal bit-size constraints to the UTXOSettlement circuit, regenerates Groth16 verification artifacts and verifier constants, extends MARKPool.withdraw with an isMint flag to select mint vs burn behavior, and refactors circuit tests to use a single instantiated witness calculator and updated call signatures. ChangesUTXO Settlement Constraints & Mint/Burn Mode
Sequence DiagramsequenceDiagram
participant Caller
participant MARKPool
participant UTXOVerifier
participant Groth16
Caller->>MARKPool: withdraw(recipient, amount, isMint, nullifierHash, commitmentHash, proof)
MARKPool->>MARKPool: Build signals [nullifierHash, commitmentHash, amount, isMint]
MARKPool->>UTXOVerifier: verifyProof(signals, proof)
UTXOVerifier->>Groth16: perform pairing checks with IC/delta constants
Groth16-->>UTXOVerifier: verification result
UTXOVerifier-->>MARKPool: boolean valid
MARKPool->>MARKPool: mark nullifierHash as spent
MARKPool->>MARKPool: delete commitment
MARKPool->>MARKPool: if isMint == 1 then mint amount to recipient
MARKPool->>MARKPool: if isMint == 0 then transfer amount from recipient and burn
MARKPool-->>Caller: Done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 `@circuits/test/UTXOSettlement.test.mjs`:
- Around line 74-88: Add a failing test case that mirrors the other overflow
checks: call expectFail with a descriptive message like "settlementModule
exceeds 160 bits" and pass a test vector spreading valid but setting
settlementModule to 2n ** 160n, and compute commitmentHash using
poseidonHash(secret, amount, isMint, recipient, chainId, 2n ** 160n); place this
new expectFail in UTXOSettlement.test.mjs next to the other overflow tests so
the Num2Bits(160) constraint for settlementModule is validated.
In `@contracts/src/pool/MARKPool.sol`:
- Around line 149-154: Add a unit test exercising the burn branch when isMint ==
false: use the MARKPool flow that calls TOKEN.safeTransferFrom(recipient,
address(this), amount) and TOKEN.burn(amount) to assert the recipient has
approved the pool beforehand (approve(poolAddress, amount)), that
safeTransferFrom transfers tokens into the pool, and that burn reduces total
supply/balance accordingly; also update documentation/comments around the
function that processes commitments (reference commitmentHash and proof) to
state clearly that access control is enforced by the zero-knowledge proof (proof
must bind to the recipient’s shielded secret) and not by msg.sender, and note
the explicit requirement that the recipient must pre-approve the pool contract
for the burn path.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c526499e-458a-4c08-a33f-e5238575c53f
📒 Files selected for processing (5)
circuits/artifacts/utxo_verification_key.jsoncircuits/test/UTXOSettlement.test.mjscircuits/utxo/UTXOSettlement.circomcontracts/src/pool/MARKPool.solcontracts/src/pool/verifier/UTXOVerifier.sol
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/test/unit/pool/MARKPool.t.sol (1)
79-125: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit
isMint = falsetests for the new burn branch.From Line 83 through Line 124, every updated
withdrawcall passestrue, so this suite validates only mint mode. Please add burn-path coverage (isMint = false) with:
- a success case (recipient has balance + approves pool), and
- a failure case (missing approval or insufficient balance).
Without this, the newly introduced branch can regress silently.
Proposed test additions
+ function testWithdrawBurnsFromRecipientWhenIsMintFalse() public { + vm.prank(operator); + pool.commit(COMMITMENT, AMOUNT); + + // Prepare recipient balance and approval for burn path + vm.prank(admin); + token.mint(recipient, AMOUNT); + vm.prank(recipient); + token.approve(address(pool), AMOUNT); + + pool.withdraw(recipient, AMOUNT, false, NULLIFIER, COMMITMENT, A, B, C); + + assertEq(token.balanceOf(recipient), 0); + assertTrue(pool.usedNullifiers(NULLIFIER)); + assertEq(pool.commitments(COMMITMENT), 0); + } + + function testWithdrawBurnRevertsWithoutApproval() public { + vm.prank(operator); + pool.commit(COMMITMENT, AMOUNT); + + vm.prank(admin); + token.mint(recipient, AMOUNT); + + vm.expectRevert(); // replace with concrete selector if exposed by token/pool + pool.withdraw(recipient, AMOUNT, false, NULLIFIER, COMMITMENT, A, B, C); + }🤖 Prompt for 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. In `@contracts/test/unit/pool/MARKPool.t.sol` around lines 79 - 125, Add two tests exercising the new burn branch of Pool.withdraw (isMint = false): one success and one failure. Create a success test (e.g., testWithdrawBurnsFromRecipientSuccess) that mints or assigns AMOUNT to recipient, has recipient approve the pool for AMOUNT, commits the commitment as operator, then calls pool.withdraw(recipient, AMOUNT, false, NULLIFIER, COMMITMENT, A, B, C) and asserts recipient balance decreased by AMOUNT, pool.usedNullifiers(NULLIFIER) is true and pool.commitments(COMMITMENT) == 0. Create a failure test (e.g., testWithdrawBurnsFromRecipientRevertsOnInsufficientApprovalOrBalance) that sets up recipient without sufficient balance or without approving pool and then vm.expectRevert with the appropriate PoolErrors (Allowance/transfer failure or CommitmentInvalid if applicable) before calling pool.withdraw(...) with isMint = false to ensure the burn path reverts correctly.
🤖 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.
Outside diff comments:
In `@contracts/test/unit/pool/MARKPool.t.sol`:
- Around line 79-125: Add two tests exercising the new burn branch of
Pool.withdraw (isMint = false): one success and one failure. Create a success
test (e.g., testWithdrawBurnsFromRecipientSuccess) that mints or assigns AMOUNT
to recipient, has recipient approve the pool for AMOUNT, commits the commitment
as operator, then calls pool.withdraw(recipient, AMOUNT, false, NULLIFIER,
COMMITMENT, A, B, C) and asserts recipient balance decreased by AMOUNT,
pool.usedNullifiers(NULLIFIER) is true and pool.commitments(COMMITMENT) == 0.
Create a failure test (e.g.,
testWithdrawBurnsFromRecipientRevertsOnInsufficientApprovalOrBalance) that sets
up recipient without sufficient balance or without approving pool and then
vm.expectRevert with the appropriate PoolErrors (Allowance/transfer failure or
CommitmentInvalid if applicable) before calling pool.withdraw(...) with isMint =
false to ensure the burn path reverts correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3e9f43a4-429f-44bb-acc5-f7c9611bf955
📒 Files selected for processing (1)
contracts/test/unit/pool/MARKPool.t.sol
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds range constraints to UTXOSettlement circuit to prevent malformed private inputs.
Changes:
MARKPool.withdraw now accepts isMint param: true mints RYLA to recipient, false burns RYLA from recipient (pre-approved).
Trusted setup rerun for updated circuit (1050 constraints, pot11).
Scope: circuits, contracts
Verification: 9 witness tests passing. 84 Solidity unit tests passing.
Risk: Low. Circuit change requires trusted setup rerun (done). No deployed contracts affected.
Summary by CodeRabbit
New Features
withdraw()now accepts anisMintflag to support both mint and burn settlement flows.Bug Fixes / Chores
Tests
Refactor