Skip to content

fix(contracts): rewrite MARKPool for MARK's 4-signal circuit#98

Merged
iap merged 1 commit into
devfrom
fix/mark-pool-rewrite
May 11, 2026
Merged

fix(contracts): rewrite MARKPool for MARK's 4-signal circuit#98
iap merged 1 commit into
devfrom
fix/mark-pool-rewrite

Conversation

@iap
Copy link
Copy Markdown
Contributor

@iap iap commented May 11, 2026

MARKPool was incorrectly designed around a 13-signal circuit from an external project. Rewritten from scratch for MARK's own UTXOSettlement circuit.

New model: operators commit note hashes on-chain (commit), note owners prove ownership via ZK proof to withdraw RYLA (withdraw). No Merkle tree required for this circuit.

UTXOVerifier.sol regenerated from MARK's own trusted setup (fresh Powers of Tau, fresh phase 2 contribution). Verification key committed to circuits/artifacts/.

Scope: contracts, circuits

Verification: 84 unit tests passing.

Risk: Low. Replaces incorrect implementation with correct one. MARKPool is additive — no existing contracts changed.

Summary by CodeRabbit

  • New Features

    • Added a commit operation for registering notes with the pool.
  • Bug Fixes & Improvements

    • Simplified the withdrawal mechanism from a Merkle-tree-based approach to a commitment/nullifier-based scheme.
    • Updated proof verification to use a streamlined signature format.
  • Removals

    • Removed deposit, transact, and protocol epoch features.
    • Eliminated Merkle root tracking and withdrawal binding functionality.

Review Change Stack

MARKPool was incorrectly designed around a 13-signal circuit from an
external project. Rewritten from scratch for MARK's own UTXOSettlement
circuit (4 public signals: nullifierHash, commitmentHash, amount, isMint).

New model: operators commit note hashes on-chain, note owners prove
ownership via ZK proof to withdraw RYLA. No Merkle tree required.

- MARKPool: commit + withdraw (ZK-proven)
- IUTXOVerifier: 4-signal interface matching generated verifier
- UTXOVerifier.sol: regenerated from MARK's own trusted setup
- Removed: MerkleTree, PoseidonT3, PoolPublicInputs (not needed)
- circuits/artifacts/utxo_verification_key.json: MARK's verification key

10 unit tests passing.
@iap iap requested a review from a team as a code owner May 11, 2026 12:27
@github-actions
Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 11, 2026

Walkthrough

This PR redesigns the UTXO settlement architecture from a complex Merkle-tree-based multi-input system to a minimal single-note commitment/nullifier scheme. The Circom circuit is replaced with a 4-signal UTXOSettlement template, verifier keys are regenerated and IC points reduced to 4, MARKPool contract simplified to operator-only commit and proof-based withdraw, and supporting Merkle/Poseidon libraries removed.

Changes

UTXO Settlement Redesign

Layer / File(s) Summary
Circuit Design
circuits/utxo/UTXOSettlement.circom
New UTXOSettlement() template with 4 public signals (nullifierHash, commitmentHash, amount, isMint) and 5 private inputs (secret, nonce, recipient, chainId, settlementModule). Enforces isMint ∈ {0,1}, amount ≠ 0, nullifierHash = Poseidon(secret, nonce), commitmentHash = Poseidon(secret, amount, isMint, recipient, chainId, settlementModule). Old UTXO(depth, nIn, nOut) circuit and Merkle logic removed.
Verification Key Artifact
circuits/artifacts/utxo_verification_key.json
New Groth16 verification key for bn128 curve with nPublic=4 and IC point constants (IC0–IC4) for the settlement circuit.
Verifier Interface
contracts/src/pool/interfaces/IUTXOVerifier.sol
verifyProof() signature updated: 4 public signals (nullifierHash, commitmentHash, amount, isMint) replace 13-element input array.
Verifier Implementation
contracts/src/pool/verifier/UTXOVerifier.sol
Hardcoded VK constants updated for bn128 curve; IC points reduced from IC0–IC13 to IC0–IC4. Proof verification logic updated: vk_x accumulation and public-signal validation loop reduced to 4 signals.
Pool Contract
contracts/src/pool/MARKPool.sol
Operator-only commit(commitmentHash, amount) records notes. Public withdraw(recipient, amount, nullifierHash, commitmentHash, uint256[2] a, uint256[2][2] b, uint256[2] c) verifies Groth16 proof against verifier, validates commitment amount, marks nullifier used, deletes commitment, mints TOKEN. Storage simplified to commitments and usedNullifiers mappings. Removed: deposit, transact, withdraw-binding, Merkle root tracking, fee/epoch management.
Cleanup
contracts/src/pool/PoolPublicInputs.sol, contracts/src/pool/crypto/MerkleTree.sol, contracts/src/pool/crypto/PoseidonT3.sol
Removed PoolPublicInputs (13-signal array builder), MerkleTree (incremental Poseidon impl), and PoseidonT3 (inline asm hash) libraries.
Test Coverage
contracts/test/unit/pool/MARKPool.t.sol
New commit tests: testCommitRegistersNote(), testCommitRevertsForZeroCommitment(), testCommitRevertsForZeroAmount(), testCommitRevertsForDuplicate(). New withdraw tests: testWithdrawMintsToRecipient(), testWithdrawRevertsOnInvalidProof(), testWithdrawRevertsOnNullifierReplay(), testWithdrawRevertsOnAmountMismatch(), testWithdrawRevertsOnUnregisteredCommitment(). Old deposit/transact/withdraw-binding and Merkle-seeding tests removed.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • trade/mark#96: Both PRs modify circuits/utxo/UTXOSettlement.circom, introducing the UTXOSettlement template and main component public signals.
  • trade/mark#97: Both PRs refactor the same UTXO domain: this PR replaces the 13-signal Merkle-based design with a 4-signal settlement model, while the related PR modifies the complementary multi-input Merkle UTXO contract and verifier.

Poem

🐰 The circuits hop from trees to notes so light,
Four signals bloom where thirteen used to gleam,
No more the Merkle roots and fees in sight—
Just simple proofs to settle every dream! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the key changes and rationale, but does not follow the provided repository template structure with required sections like Scope, Verification, Risk Review, and Governance checkboxes. Reformat the description to match the template with explicit Scope checkboxes, Verification checklist items, Risk Review section, and Governance checks.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: rewriting MARKPool to use MARK's 4-signal circuit instead of an external 13-signal circuit.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/mark-pool-rewrite

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dc2d17ab16

ℹ️ 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".

Comment thread contracts/src/pool/MARKPool.sol
Comment thread contracts/src/pool/MARKPool.sol
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 `@contracts/src/pool/MARKPool.sol`:
- Around line 88-104: The NatSpec for commit() is misleading — it claims "RYLA
is transferred in and burned" though the commit function only records a
commitment (commitmentHash -> amount) and emits NoteCommitted; update the
NatSpec to accurately describe behavior by removing the transfer/burn statement
and clearly stating that the operator registers the commitment on-chain (the
actual mint/transfer happens on withdrawal), and ensure the params and
description reference the stored mapping commitments and emitted event
NoteCommitted for clarity.

In `@contracts/test/unit/pool/MARKPool.t.sol`:
- Around line 59-77: Add a unit test in MARKPool.t.sol that triggers the
commit() branch validating the SNARK scalar field: create a commitment value >=
SNARK_SCALAR_FIELD (e.g., SNARK_SCALAR_FIELD or SNARK_SCALAR_FIELD + 1) and call
pool.commit with that value while vm.prank(operator) is set, then
vm.expectRevert(PoolErrors.CommitmentInvalid.selector). Reference the existing
test style (testCommitRevertsForZeroCommitment/testCommitRevertsForZeroAmount)
and use the same operator/AMOUNT setup so the new test (e.g.,
testCommitRevertsForScalarFieldOverflow) asserts the revert when
uint256(commitmentHash) >= SNARK_SCALAR_FIELD.
🪄 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: e17d449f-b0ac-4b8c-85e8-749ae9eb3b32

📥 Commits

Reviewing files that changed from the base of the PR and between 8e080bd and dc2d17a.

📒 Files selected for processing (9)
  • circuits/artifacts/utxo_verification_key.json
  • circuits/utxo/UTXOSettlement.circom
  • contracts/src/pool/MARKPool.sol
  • contracts/src/pool/PoolPublicInputs.sol
  • contracts/src/pool/crypto/MerkleTree.sol
  • contracts/src/pool/crypto/PoseidonT3.sol
  • contracts/src/pool/interfaces/IUTXOVerifier.sol
  • contracts/src/pool/verifier/UTXOVerifier.sol
  • contracts/test/unit/pool/MARKPool.t.sol
💤 Files with no reviewable changes (3)
  • contracts/src/pool/crypto/MerkleTree.sol
  • contracts/src/pool/PoolPublicInputs.sol
  • contracts/src/pool/crypto/PoseidonT3.sol

Comment thread contracts/src/pool/MARKPool.sol
Comment thread contracts/test/unit/pool/MARKPool.t.sol
@iap iap merged commit fc29df6 into dev May 11, 2026
19 checks passed
@iap iap deleted the fix/mark-pool-rewrite branch May 11, 2026 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant