feat(pool): add MARKPool ZK UTXO pool domain#100
Open
iap wants to merge 20 commits into
Open
Conversation
Introduces the full pool domain for private RYLA transfers:
Contracts:
- MARKPool: ZK UTXO pool with Merkle tree, fee policy, bridge-out/in,
withdraw binding, AccessManaged access control
- MARKWithdrawAdapter: EIP-712 signature-based withdrawal adapter
- RYLACreditLedger: ICreditLedger adapter bridging MARKPool to RYLA
mint/burn; restricted to pool caller only (onlyPool)
- PoolFeePolicy, PoolPublicInputs, PoolValidation: pool support libraries
- MARKPoolVerifier: Groth16 verifier generated from MARKPool circuit
(13 public signals, pot15 trusted setup)
Interfaces: ICreditLedger, IVerifier, IPoolBridge, IPoolNullifier
Crypto: MerkleTree (Poseidon, depth-20), ProofUtils, PoseidonT3
Circuit:
- circuits/mark/MARKPool.circom: MARK-native UTXO circuit (depth=20,
2-in/2-out, 13 public signals); renamed from prototype utxo.circom,
domain constants documented as permanent, hardcoded fee policy removed
- circuits/setup.mjs: trusted setup script (pot15)
- circuits/test/MARKPool.test.mjs: 13 witness tests
CI: circuits-ci.yml runs witness tests on every PR
Tests: MARKPool.t.sol (22), MARKWithdrawAdapter.t.sol (9),
RYLACreditLedger.t.sol (8)
- PoolErrors.sol: rewrite to match Pool.sol, PoolValidation.sol, and MerkleTree.sol — adds 25 missing errors (build was broken), removes 18 errors only used by the old MARKPool prototype - MARKPool.sol: rename domain separator Pool.WithdrawBinding.v1 to MARKPool.WithdrawBinding.v1 (permanent, must be set before deploy) - MARKWithdrawAdapter.sol: rename domain separator WithdrawAdapter.Intent.v1 to MARKWithdrawAdapter.Intent.v1 - UTXOVerifier.sol: delete (built for old 4-signal circuit, wrong interface, superseded by MARKPoolVerifier.sol) - IUTXOVerifier.sol: delete (superseded by IVerifier.sol) - UTXOSettlement.circom: delete (superseded by MARKPool.circom) - Groth16SettlementVerifier.sol: update stale comment - KNOWN_ISSUES.md: add KI-7 (two-circuit architecture), KI-8 (pool domain access control model) - foundry.toml: via_ir = true for pool domain compilation
Dependency ReviewThe following issues were found:
License Issues.github/workflows/circuits-ci.yml
OpenSSF Scorecard
Scanned Files
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughReplaces the previous UTXO circuit and operator-driven pool with a Merkle-tree-backed MARKPool ZK‑UTXO system: adds a 13-signal Circom circuit, trusted-setup and witness tests, CI, regenerated Groth16 verifier, Merkle/proof utilities, ICreditLedger + RYLACreditLedger, MARKWithdrawAdapter, rewritten MARKPool contract, tests, deploy/release scripts, docs, and architecture-guard updates. ChangesCircuits, build, trusted setup, and witness testing
Merkle/verifier primitives, pool contracts, withdraw flow, infra and tests
Sequence Diagram(s)sequenceDiagram
participant User
participant Witness as Off-chain Witness Calc
participant Pool as MARKPool
participant Verifier as MARKPoolVerifier
participant Tree as MerkleTree
participant Ledger as RYLACreditLedger
User->>Witness: build proof (nullifiers, outCommitments, merkleRoot, fee, ...)
Witness-->>User: proof + public signals
User->>Pool: transact(merkleRoot, nullifiers, outCommitments, fee, relayer, proof)
Pool->>Pool: validate ranges, nullifier freshness, root usability
Pool->>Verifier: verifyProof(13 public signals, proof)
Verifier-->>Pool: proof valid
Pool->>Pool: mark nullifiers used
Pool->>Tree: insert(outCommitment[0])
Pool->>Tree: insert(outCommitment[1])
Tree-->>Pool: root updated
Pool->>Ledger: credit(relayer, relayerAmount)
Ledger-->>Pool: credited
Pool-->>User: transaction accepted
sequenceDiagram
participant Owner
participant Intent
participant User
participant Adapter as MARKWithdrawAdapter
participant Pool as IPoolNullifier
participant Ledger as RYLACreditLedger
Owner->>Owner: sign owner signature
Intent->>Intent: sign intent signature
User->>Adapter: withdrawWithSig(nullifiers, ownerSig, intentSig, nonce, deadline)
Adapter->>Pool: isNullifierUsedGlobal(nullifier)
Pool-->>Adapter: not used
Adapter->>Pool: nullifierWithdrawBinding(nullifier)
Pool-->>Adapter: binding hash
Adapter->>Adapter: verify signatures, nonce, deadline, liquidity
Adapter->>Adapter: mark nullifiers claimed, increment nonce
Adapter->>Ledger: debit(creditOwner, amount)
Ledger-->>Adapter: debited
Adapter-->>User: ETH transferred to recipient
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/package.json`:
- Around line 7-8: The test script runs before the circom build so CI fails with
MODULE_NOT_FOUND for build/MARKPool_js/witness_calculator.js; update the
package.json scripts so the build runs before tests—either add a "pretest"
script that runs the existing "build" (or change "test" to run "npm run build &&
node test/MARKPool.test.mjs") so that the MARKPool.circom artifacts are
generated prior to invoking MARKPool.test.mjs; update the scripts referencing
"build" and "test" accordingly.
In `@contracts/KNOWN_ISSUES.md`:
- Around line 77-102: KI-7 is duplicated and contains stale/conflicting details
(absolute path, mixed legacy circuit names); consolidate into one canonical
entry that references the current symbols: Pool, MARKSettlementModule,
Groth16SettlementVerifier, AttestedSettlementVerifier, and utxo.circom; remove
the duplicate KI number and any mention of UTXOSettlement.circom except as an
explicit legacy note, replace the absolute artifact path with a repo-relative
path (e.g., ./circuits/artifacts/prod/), and make a single authoritative
Status/Impact block that states the pool and verifier use the 13-signal layout
(list merkleRoot, chainId, dstChainId, protocolEpoch, fee, relayer,
nullifier[2], outCommitment[2], withdrawOwner, withdrawRecipient,
withdrawAmount), note that Groth16SettlementVerifier still needs the generated
verifier contract integrated, and keep AttestedSettlementVerifier described as
the signature fallback.
In `@contracts/src/pool/MARKPool.sol`:
- Around line 345-356: bridgeIn lacks replay protection: add a unique source
message identifier parameter (e.g., bytes32 srcMessageId) to bridgeIn, record
processed messages on-chain and reject duplicates before calling
_insertCommitments; implement a storage mapping to track processed messages
keyed by srcChainId and srcMessageId, revert with a clear error (e.g.,
AlreadyProcessed) if the id was seen, mark the id processed after successful
insertion, and include the srcMessageId in the BridgeIn event so relayers can
correlate deliveries and retries.
In `@contracts/src/pool/PoolFeePolicy.sol`:
- Around line 6-12: Add explicit precondition checks at the start of the split
function to prevent panic-reverts: require that maxFeeBurnBps != 0 and that
feeBurnBps <= maxFeeBurnBps, each with a descriptive revert message (e.g.,
"maxFeeBurnBps>0" and "feeBurnBps<=maxFeeBurnBps"); then proceed with the
existing arithmetic and return burnAmount and relayerAmount as before.
In `@contracts/src/settlement/verifier/Groth16SettlementVerifier.sol`:
- Around line 13-14: Update the NatSpec comment in Groth16SettlementVerifier so
the circuit name is consistent: replace any mention of "UTXOSettlement circuit"
with the finalized circuit name "MARKPool" (or use "MARKPool circuit") and
ensure the example verifier name MARKPoolVerifier and the interface
IUTXOSettlementVerifier provenance are both referenced consistently in the
docblock; update the two lines to state that this contract delegates to a
Groth16 verifier (e.g. MARKPoolVerifier) generated from the MARKPool circuit so
integrators have an unambiguous circuit/verifier mapping.
- Around line 84-93: The isMint parameter is currently ignored (no-op) which
allows wrong-direction settlements; update the verifier to fail-closed by
explicitly validating isMint in the verifier entrypoint (the symbol isMint in
the Groth16SettlementVerifier logic) — e.g., require the value equals the
expected direction for settlement (reject/more strictly: revert when isMint
indicates a mint) until a dedicated direction public signal or separate verifier
path exists; add a clear revert message referencing isMint to aid debugging.
In `@contracts/src/withdraw/MARKWithdrawAdapter.sol`:
- Around line 111-120: The current computeWithdrawIntentDigest uses
MessageHashUtils.toEthSignedMessageHash (EIP-191/personal_sign) which is
incompatible with EIP-712; replace this pattern by inheriting OpenZeppelin's
EIP712 and compute the digest via _hashTypedDataV4(structHash) instead of
toEthSignedMessageHash, then use ECDSA.recover(...) when verifying signatures;
update computeWithdrawIntentDigest (and the analogous logic referenced around
lines 208–215, e.g., any functions calling computeWithdrawIntentHash or
performing signature recovery) to build the typed-data structHash with
computeWithdrawIntentHash, call _hashTypedDataV4(structHash) to get the
domain-aware digest, and use ECDSA.recover for verification so clients using
EIP-712 will produce matching signatures.
- Line 147: The adapter currently calls assetLedger.debit(creditOwner, amount)
directly, but RYLACreditLedger.debit() is protected by onlyPool so the adapter
will be Unauthorized(); either route the debit through the Pool or grant the
adapter pool-level authorization: modify MARKWithdrawAdapter to call the Pool
contract to perform the debit (e.g., invoke a Pool method that in turn calls
assetLedger.debit) instead of calling assetLedger.debit directly, or update
RYLACreditLedger access control to allow this adapter (or add an
adapter-specific entrypoint) so that calls originate from the POOL-authorized
context; target references: assetLedger.debit, RYLACreditLedger.debit, onlyPool
and the POOL address.
In `@contracts/test/unit/pool/MARKPool.t.sol`:
- Around line 292-296: Replace the broad vm.expectRevert() in
testBridgeInRevertsWhenCallerNotRestricted with an expectation for the pool
contract's specific revert selector/error (the error emitted when non-restricted
callers call bridgeIn). Update the test to call vm.expectRevert(...) with
abi.encodeWithSelector(TheContractSpecificError.selector, /* args if any */)
before calling pool.bridgeIn(901, commitments) so the test asserts the exact
revert reason from bridgeIn rather than any revert.
In `@contracts/test/unit/withdraw/MARKWithdrawAdapter.t.sol`:
- Around line 156-182: The three placeholder tests must actually invoke the
adapter withdrawal path and assert outcomes: in
testWithdrawRequiresSufficientLiquidity deploy MARKWithdrawAdapter and call
adapter.withdraw(...) (or the actual withdraw-entry method) with valid dummy
params under a vm.prank(user) and assert it reverts with "Insufficient
liquidity" when the adapter has 0 balance; in testWithdrawNonceIncrement fund
the adapter with native tokens, perform a successful adapter.withdraw(...) and
then assert adapter.withdrawNonce(user) == 1; in
testNullifierCannotBeClaimedTwice call adapter.withdraw(...) (or the internal
claim method) once with a chosen nullifier and assert
adapter.claimedNullifiers(nullifier) is true, then attempt the same
withdraw/claim again and assert it reverts (duplicate nullifier). Use the
existing MARKWithdrawAdapter instance, adapter.withdraw (or the contract’s
public withdraw method), adapter.withdrawNonce, and adapter.claimedNullifiers to
locate the code to modify.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b075a90-6f6e-49cd-85c1-f7bd952b3edf
⛔ Files ignored due to path filters (1)
contracts/src/crypto/generated/PoseidonT3.solis excluded by!**/generated/**
📒 Files selected for processing (29)
.github/workflows/circuits-ci.ymlcircuits/.gitignorecircuits/mark/MARKPool.circomcircuits/package.jsoncircuits/setup.mjscircuits/test/MARKPool.test.mjscircuits/utxo/UTXOSettlement.circomcontracts/KNOWN_ISSUES.mdcontracts/foundry.tomlcontracts/src/crypto/MerkleTree.solcontracts/src/crypto/ProofUtils.solcontracts/src/interfaces/ICreditLedger.solcontracts/src/interfaces/IPoolBridge.solcontracts/src/interfaces/IPoolNullifier.solcontracts/src/interfaces/IVerifier.solcontracts/src/pool/MARKPool.solcontracts/src/pool/PoolFeePolicy.solcontracts/src/pool/PoolPublicInputs.solcontracts/src/pool/PoolValidation.solcontracts/src/pool/RYLACreditLedger.solcontracts/src/pool/errors/PoolErrors.solcontracts/src/pool/interfaces/IUTXOVerifier.solcontracts/src/pool/verifier/MARKPoolVerifier.solcontracts/src/settlement/verifier/Groth16SettlementVerifier.solcontracts/src/withdraw/MARKWithdrawAdapter.solcontracts/src/withdraw/MARKWithdrawErrors.solcontracts/test/unit/pool/MARKPool.t.solcontracts/test/unit/pool/RYLACreditLedger.t.solcontracts/test/unit/withdraw/MARKWithdrawAdapter.t.sol
💤 Files with no reviewable changes (2)
- contracts/src/pool/interfaces/IUTXOVerifier.sol
- circuits/utxo/UTXOSettlement.circom
Comment on lines
+6
to
+12
| function split(uint256 fee, uint256 feeBurnBps, uint256 maxFeeBurnBps) | ||
| internal | ||
| pure | ||
| returns (uint256 burnAmount, uint256 relayerAmount) | ||
| { | ||
| burnAmount = fee * feeBurnBps / maxFeeBurnBps; | ||
| relayerAmount = fee - burnAmount; |
There was a problem hiding this comment.
Guard fee-split invariants before arithmetic.
Line 11 and Line 12 can panic-revert for invalid inputs (maxFeeBurnBps == 0 or feeBurnBps > maxFeeBurnBps). Add explicit precondition checks so failures are deterministic and easier to diagnose.
Suggested patch
function split(uint256 fee, uint256 feeBurnBps, uint256 maxFeeBurnBps)
internal
pure
returns (uint256 burnAmount, uint256 relayerAmount)
{
+ require(maxFeeBurnBps != 0, "PoolFeePolicy: zero max bps");
+ require(feeBurnBps <= maxFeeBurnBps, "PoolFeePolicy: burn bps too high");
burnAmount = fee * feeBurnBps / maxFeeBurnBps;
relayerAmount = fee - burnAmount;
}🤖 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/src/pool/PoolFeePolicy.sol` around lines 6 - 12, Add explicit
precondition checks at the start of the split function to prevent panic-reverts:
require that maxFeeBurnBps != 0 and that feeBurnBps <= maxFeeBurnBps, each with
a descriptive revert message (e.g., "maxFeeBurnBps>0" and
"feeBurnBps<=maxFeeBurnBps"); then proceed with the existing arithmetic and
return burnAmount and relayerAmount as before.
Comment on lines
+13
to
14
| /// @dev Implements IUTXOSettlementVerifier by delegating to a Groth16 verifier contract (e.g. MARKPoolVerifier) | ||
| /// generated by snarkjs from the UTXOSettlement circuit (13 public signals). |
There was a problem hiding this comment.
Clarify circuit naming in NatSpec to avoid integration confusion.
Line 13 references MARKPoolVerifier, but Line 14 still says UTXOSettlement circuit. Please align both lines to the same finalized circuit naming so verifier provenance is unambiguous.
Suggested doc-only patch
-/// generated by snarkjs from the UTXOSettlement circuit (13 public signals).
+/// generated by snarkjs from the MARKPool settlement-compatible circuit (13 public signals).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// @dev Implements IUTXOSettlementVerifier by delegating to a Groth16 verifier contract (e.g. MARKPoolVerifier) | |
| /// generated by snarkjs from the UTXOSettlement circuit (13 public signals). | |
| /// `@dev` Implements IUTXOSettlementVerifier by delegating to a Groth16 verifier contract (e.g. MARKPoolVerifier) | |
| /// generated by snarkjs from the MARKPool settlement-compatible circuit (13 public signals). |
🤖 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/src/settlement/verifier/Groth16SettlementVerifier.sol` around lines
13 - 14, Update the NatSpec comment in Groth16SettlementVerifier so the circuit
name is consistent: replace any mention of "UTXOSettlement circuit" with the
finalized circuit name "MARKPool" (or use "MARKPool circuit") and ensure the
example verifier name MARKPoolVerifier and the interface IUTXOSettlementVerifier
provenance are both referenced consistently in the docblock; update the two
lines to state that this contract delegates to a Groth16 verifier (e.g.
MARKPoolVerifier) generated from the MARKPool circuit so integrators have an
unambiguous circuit/verifier mapping.
Comment on lines
+84
to
+93
| // isMint direction is not directly validated in settlement mode. The circuit's balance | ||
| // equation (sum(inputs) = sum(outputs) + fee + withdrawAmount) and withdrawal binding | ||
| // enforce correctness. For pool usage, the circuit validates the full UTXO flow including | ||
| // Merkle membership and nullifier uniqueness. For settlement usage, we use a simplified | ||
| // mapping where intentId maps to merkleRoot/nullifier and account maps to withdrawOwner/ | ||
| // withdrawRecipient. The isMint parameter is passed for interface compatibility but the | ||
| // circuit enforces correctness via the withdrawal amount and balance constraints. | ||
| // TODO: add an explicit isMint signal check once the settlement-specific circuit finalizes | ||
| // a dedicated direction signal. | ||
| (isMint); |
There was a problem hiding this comment.
isMint is a no-op today, which can admit wrong-direction settlements.
Line 62 accepts direction, but Line 84-Line 93 explicitly skip validation. That creates an interface/behavior mismatch at a security boundary. Prefer fail-closed until a dedicated public signal is available (or split direction-specific verifier paths).
Minimal fail-closed guard (interim)
- // TODO: add an explicit isMint signal check once the settlement-specific circuit finalizes
- // a dedicated direction signal.
- (isMint);
+ // Direction is not encoded in current settlement signals; fail closed to avoid
+ // silently accepting ambiguous mode until circuit support lands.
+ if (isMint) return false;🤖 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/src/settlement/verifier/Groth16SettlementVerifier.sol` around lines
84 - 93, The isMint parameter is currently ignored (no-op) which allows
wrong-direction settlements; update the verifier to fail-closed by explicitly
validating isMint in the verifier entrypoint (the symbol isMint in the
Groth16SettlementVerifier logic) — e.g., require the value equals the expected
direction for settlement (reject/more strictly: revert when isMint indicates a
mint) until a dedicated direction public signal or separate verifier path
exists; add a clear revert message referencing isMint to aid debugging.
Comment on lines
+111
to
+120
| function computeWithdrawIntentDigest( | ||
| address creditOwner, | ||
| address recipient, | ||
| uint256 amount, | ||
| bytes32[2] calldata nullifiers, | ||
| uint256 nonce, | ||
| uint256 deadline | ||
| ) external view returns (bytes32) { | ||
| bytes32 intentHash = computeWithdrawIntentHash(creditOwner, recipient, amount, nullifiers, nonce, deadline); | ||
| return MessageHashUtils.toEthSignedMessageHash(intentHash); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does OpenZeppelin's MessageHashUtils.toEthSignedMessageHash implement EIP-191 (personal_sign) or EIP-712 typed-data hashing, and what OpenZeppelin pattern should be used in Solidity to verify EIP-712 signatures?
💡 Result:
Yes: OpenZeppelin’s MessageHashUtils.toEthSignedMessageHash implements EIP-191 “personal_sign” (ERC-191 version 0x45 with prefix “\x19Ethereum Signed Message:\n32” for bytes32 input), not EIP-712 typed-data hashing. [1][2] For EIP-712 typed-data signature verification in Solidity with OpenZeppelin, use the EIP712 pattern that hashes typed data via _hashTypedDataV4(...) and then recovers with ECDSA.recover. Concretely: inherit from OpenZeppelin’s EIP712 contract, compute the structHash for your typed message, then do: bytes32 digest = _hashTypedDataV4(structHash); address signer = ECDSA.recover(digest, signature); OpenZeppelin documents this flow as the “final step” for EIP-712: _hashTypedDataV4 returns the fully encoded EIP-712 message digest for a given domain, and it can be used with ECDSA.recover to obtain the signer. [3][4]
Citations:
- 1: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/MessageHashUtils.sol
- 2: https://evmauth.io/lib/openzeppelin-contracts-upgradeable/lib/openzeppelin-contracts/contracts/utils/cryptography/MessageHashUtils.sol/library.MessageHashUtils.html
- 3: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol
- 4: https://docs.openzeppelin.com/community-contracts/utilities
Switch to OpenZeppelin's EIP712 pattern instead of personal_sign.
MessageHashUtils.toEthSignedMessageHash() implements EIP-191 ("personal_sign"), not EIP-712. If the PR intends to support EIP-712 typed-data signatures, any client following that flow will produce a different signature and fail verification here. Instead, inherit from OpenZeppelin's EIP712 contract and use _hashTypedDataV4(structHash) followed by ECDSA.recover() to properly verify EIP-712 signatures.
Also applies to lines 208–215.
🤖 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/src/withdraw/MARKWithdrawAdapter.sol` around lines 111 - 120, The
current computeWithdrawIntentDigest uses MessageHashUtils.toEthSignedMessageHash
(EIP-191/personal_sign) which is incompatible with EIP-712; replace this pattern
by inheriting OpenZeppelin's EIP712 and compute the digest via
_hashTypedDataV4(structHash) instead of toEthSignedMessageHash, then use
ECDSA.recover(...) when verifying signatures; update computeWithdrawIntentDigest
(and the analogous logic referenced around lines 208–215, e.g., any functions
calling computeWithdrawIntentHash or performing signature recovery) to build the
typed-data structHash with computeWithdrawIntentHash, call
_hashTypedDataV4(structHash) to get the domain-aware digest, and use
ECDSA.recover for verification so clients using EIP-712 will produce matching
signatures.
| emit WithdrawIntentAuthorized(intentSigner, intentHash, creditOwner); | ||
| emit NullifierClaimed(nullifiers[0], creditOwner); | ||
| emit NullifierClaimed(nullifiers[1], creditOwner); | ||
| assetLedger.debit(creditOwner, amount); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ICreditLedger =="
fd -i 'ICreditLedger.sol' contracts | xargs -r sed -n '1,220p'
echo
echo "== RYLACreditLedger =="
fd -i 'RYLACreditLedger.sol' contracts | xargs -r sed -n '1,260p'
echo
echo "== debit/onlyPool references =="
rg -n -C3 --glob 'contracts/**/*.sol' '\b(contract\s+RYLACreditLedger|function\s+debit\s*\(|modifier\s+onlyPool\b|onlyPool\b)'Repository: trade/mark
Length of output: 6766
🏁 Script executed:
fd -i 'MARKWithdrawAdapter.sol' contractsRepository: trade/mark
Length of output: 101
🏁 Script executed:
wc -l contracts/src/withdraw/MARKWithdrawAdapter.solRepository: trade/mark
Length of output: 105
🏁 Script executed:
cat -n contracts/src/withdraw/MARKWithdrawAdapter.solRepository: trade/mark
Length of output: 11084
🏁 Script executed:
rg -l 'implements\s+ICreditLedger|is\s+ICreditLedger' contracts --type solRepository: trade/mark
Length of output: 82
🏁 Script executed:
rg 'is\s+ICreditLedger' contracts --glob '*.sol'Repository: trade/mark
Length of output: 218
🏁 Script executed:
find contracts/test -name '*MARKWithdrawAdapter*' -type fRepository: trade/mark
Length of output: 109
🏁 Script executed:
wc -l contracts/test/unit/withdraw/MARKWithdrawAdapter.t.solRepository: trade/mark
Length of output: 113
🏁 Script executed:
head -100 contracts/test/unit/withdraw/MARKWithdrawAdapter.t.solRepository: trade/mark
Length of output: 3418
🏁 Script executed:
sed -n '100,230p' contracts/test/unit/withdraw/MARKWithdrawAdapter.t.solRepository: trade/mark
Length of output: 4025
🏁 Script executed:
grep -A 5 'contract MockLedger' contracts/test/unit/pool/MARKPool.t.solRepository: trade/mark
Length of output: 300
Fix adapter authorization to debit from RYLACreditLedger or route debit through the pool.
Line 147 calls assetLedger.debit(creditOwner, amount) directly, but RYLACreditLedger.debit() restricts calls to the POOL address via onlyPool. When the adapter calls debit, msg.sender will be the adapter address, not the pool, causing the transaction to revert with Unauthorized(). Withdrawals will fail unless either the adapter is explicitly authorized or the debit is routed through the pool.
🤖 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/src/withdraw/MARKWithdrawAdapter.sol` at line 147, The adapter
currently calls assetLedger.debit(creditOwner, amount) directly, but
RYLACreditLedger.debit() is protected by onlyPool so the adapter will be
Unauthorized(); either route the debit through the Pool or grant the adapter
pool-level authorization: modify MARKWithdrawAdapter to call the Pool contract
to perform the debit (e.g., invoke a Pool method that in turn calls
assetLedger.debit) instead of calling assetLedger.debit directly, or update
RYLACreditLedger access control to allow this adapter (or add an
adapter-specific entrypoint) so that calls originate from the POOL-authorized
context; target references: assetLedger.debit, RYLACreditLedger.debit, onlyPool
and the POOL address.
Comment on lines
+292
to
296
| function testBridgeInRevertsWhenCallerNotRestricted() public { | ||
| bytes32[2] memory commitments = [C0, C1]; | ||
| vm.expectRevert(); | ||
| pool.bridgeIn(901, commitments); | ||
| } |
There was a problem hiding this comment.
Use a specific revert expectation for restricted bridgeIn calls.
The bare vm.expectRevert() is too broad and can mask incorrect failure reasons; assert a concrete revert selector/error to harden this test.
🤖 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 292 - 296, Replace the
broad vm.expectRevert() in testBridgeInRevertsWhenCallerNotRestricted with an
expectation for the pool contract's specific revert selector/error (the error
emitted when non-restricted callers call bridgeIn). Update the test to call
vm.expectRevert(...) with
abi.encodeWithSelector(TheContractSpecificError.selector, /* args if any */)
before calling pool.bridgeIn(901, commitments) so the test asserts the exact
revert reason from bridgeIn rather than any revert.
Comment on lines
+156
to
+182
| /// @notice Test that adapter checks native token balance | ||
| function testWithdrawRequiresSufficientLiquidity() public { | ||
| // Deploy adapter with no native tokens | ||
| MARKWithdrawAdapter emptyAdapter = new MARKWithdrawAdapter( | ||
| address(accessManager), | ||
| address(ledger), | ||
| address(pool) | ||
| ); | ||
|
|
||
| // Verify it has no balance | ||
| assertEq(address(emptyAdapter).balance, 0); | ||
|
|
||
| // Withdrawal would fail with "Insufficient liquidity" | ||
| } | ||
|
|
||
| /// @notice Test that nonce increments correctly | ||
| function testWithdrawNonceIncrement() public view { | ||
| assertEq(adapter.withdrawNonce(user), 0); | ||
| // After successful withdrawal, nonce should be 1 | ||
| } | ||
|
|
||
| /// @notice Test that nullifiers cannot be claimed twice | ||
| function testNullifierCannotBeClaimedTwice() public view { | ||
| bytes32 nullifier = keccak256("test"); | ||
| assertFalse(adapter.claimedNullifiers(nullifier)); | ||
| // After claiming, should be true | ||
| } |
There was a problem hiding this comment.
Placeholder withdrawal tests are giving false confidence.
testWithdrawRequiresSufficientLiquidity, testWithdrawNonceIncrement, and testNullifierCannotBeClaimedTwice currently don’t execute a withdrawal path or assert post-withdraw state transitions/reverts. These should run a real withdraw attempt and validate the claimed behavior.
🤖 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/withdraw/MARKWithdrawAdapter.t.sol` around lines 156 -
182, The three placeholder tests must actually invoke the adapter withdrawal
path and assert outcomes: in testWithdrawRequiresSufficientLiquidity deploy
MARKWithdrawAdapter and call adapter.withdraw(...) (or the actual withdraw-entry
method) with valid dummy params under a vm.prank(user) and assert it reverts
with "Insufficient liquidity" when the adapter has 0 balance; in
testWithdrawNonceIncrement fund the adapter with native tokens, perform a
successful adapter.withdraw(...) and then assert adapter.withdrawNonce(user) ==
1; in testNullifierCannotBeClaimedTwice call adapter.withdraw(...) (or the
internal claim method) once with a chosen nullifier and assert
adapter.claimedNullifiers(nullifier) is true, then attempt the same
withdraw/claim again and assert it reverts (duplicate nullifier). Use the
existing MARKWithdrawAdapter instance, adapter.withdraw (or the contract’s
public withdraw method), adapter.withdrawNonce, and adapter.claimedNullifiers to
locate the code to modify.
![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: 02c9da0121
ℹ️ 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".
| emit WithdrawIntentAuthorized(intentSigner, intentHash, creditOwner); | ||
| emit NullifierClaimed(nullifiers[0], creditOwner); | ||
| emit NullifierClaimed(nullifiers[1], creditOwner); | ||
| assetLedger.debit(creditOwner, amount); |
There was a problem hiding this comment.
withdrawWithSig debits credits via assetLedger.debit(creditOwner, amount), but the new RYLACreditLedger implementation only allows calls from its single POOL address (onlyPool). In the expected deployment where that ledger is attached to MARKPool (so pool fee credits work), every adapter withdrawal will revert Unauthorized, making signed native withdrawals unusable in production unless a separate ledger architecture is introduced.
Useful? React with 👍 / 👎.
- MARKPool, MARKWithdrawAdapter: rename immutables to SCREAMING_SNAKE_CASE (assetLedger->ASSET_LEDGER, proofPool->PROOF_POOL) - MARKPool: remove _assetLedger from constructor; add setAssetLedger() one-time restricted setter to break circular deploy dependency with RYLACreditLedger - DeployMARKPool.s.sol: full deployment script for pool domain (AccessManager, MARKPool, RYLACreditLedger, MARKWithdrawAdapter) - MARKPool.sol: add withdrawal flow NatSpec (burn-to-claim model) - ARCHITECTURE.md: add pool/withdraw domains, dependency rules, and withdrawal flow section - MARKPoolInvariants.t.sol: 3 invariants (nullifiers never unspent, withdraw bindings immutable, root queue only grows) - architecture-guard.sh: add pool->settlement/bridge and withdraw->settlement/bridge isolation rules
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
contracts/src/pool/MARKPool.sol (1)
379-388:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
bridgeInstill lacks explicit replay protection for delivered bridge messages.This path has no
srcMessageId/processed-message guard, so duplicate relay deliveries can be accepted again.🤖 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/src/pool/MARKPool.sol` around lines 379 - 388, The bridgeIn function currently accepts duplicate deliveries because it lacks a processed-message guard; update bridgeIn to accept a unique srcMessageId (e.g., add a bytes32 or uint256 srcMessageId parameter), check a processed mapping (e.g., processedBridgeMessages[srcChainId][srcMessageId]) and revert on replay, mark the srcMessageId as processed before calling _insertCommitments(outCommitments), and then emit BridgeIn as before; ensure you add the storage mapping and any accessor/helper (processedBridgeMessages) used to record and check processed messages.contracts/src/withdraw/MARKWithdrawAdapter.sol (2)
111-121:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftIntent digest path is EIP-191 (
personal_sign), not EIP-712 typed data.Line 120 and Line 209 use
toEthSignedMessageHash, so EIP-712 clients will produce non-matching signatures despite the typed-intent semantics.#!/bin/bash set -euo pipefail echo "== Adapter digest code ==" sed -n '85,220p' contracts/src/withdraw/MARKWithdrawAdapter.sol echo echo "== OpenZeppelin MessageHashUtils semantics ==" fd -i 'MessageHashUtils.sol' | xargs -r sed -n '1,220p' | rg -n 'toEthSignedMessageHash|Ethereum Signed Message' echo echo "== OpenZeppelin EIP712 typed-data helper ==" fd -i 'EIP712.sol' | xargs -r rg -n '_hashTypedDataV4'Also applies to: 208-215
🤖 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/src/withdraw/MARKWithdrawAdapter.sol` around lines 111 - 121, The contract is using OpenZeppelin's MessageHashUtils.toEthSignedMessageHash (EIP-191) for computeWithdrawIntentDigest (and the other usage around the withdraw verification logic), but the intent is typed data (EIP-712); replace the toEthSignedMessageHash call with the EIP-712 domain+typed-data flow by computing the struct hash for the withdraw intent (use a WITHDRAW_INTENT_TYPEHASH and computeWithdrawIntentHash or build keccak256(abi.encode(WITHDRAW_INTENT_TYPEHASH, creditOwner, recipient, amount, nullifiers, nonce, deadline))) and then call _hashTypedDataV4(structHash) from the EIP712 base (instead of MessageHashUtils.toEthSignedMessageHash). Update computeWithdrawIntentDigest and any other places that call toEthSignedMessageHash (e.g., the verify/validation code that reads the intent digest) to use _hashTypedDataV4 so signatures match EIP-712 typed-data semantics.
147-147:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift
withdrawWithSigcan hard-revert on ledger debit authorization.Line 147 calls
ASSET_LEDGER.debitdirectly, but this PR’s ledger model is pool-restricted. If the ledger enforces pool-only caller checks, all withdrawals fail at debit.🤖 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/src/withdraw/MARKWithdrawAdapter.sol` at line 147, withdrawWithSig currently calls ASSET_LEDGER.debit directly which will hard-revert under the pool-restricted ledger model; replace the direct debit with the ledger’s pool-authorized debit API (e.g. ASSET_LEDGER.debitFromPool or ASSET_LEDGER.debitFrom(poolAddress/ poolId, creditOwner, amount)) or route the debit through the Pool contract (e.g. POOL.performDebit/ poolAuthorizedDebit) so the ledger sees an authorized pool caller; update withdrawWithSig to use that pool-authorized method and pass the adapter’s pool identifier/address when invoking the ledger.
🤖 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/script/ci/architecture-guard.sh`:
- Around line 47-54: The existing forbidden-import regex used in the
check_no_imports calls (the pattern starting
'^import\s+.*"(?:\.\.\/settlement\/|\.\.\/bridge\/|src\/settlement\/|src\/bridge\/)')
doesn't match nested paths like ../../src/settlement or ../../settlement; update
both regex occurrences passed to check_no_imports (the pool -> settlement/bridge
and withdraw -> settlement/bridge checks) to allow any number of "../" segments
and an optional "src/" prefix before "settlement" or "bridge" (e.g. use a
pattern that accepts (?:\.\.\/)+ and optional src/ before (?:settlement|bridge)
), so imports like ../../src/settlement/... are caught.
In `@contracts/script/deploy/pool/DeployMARKPool.s.sol`:
- Around line 60-77: The deployment broadcasts as cfg.deployer but constructs
AccessManager with cfg.owner and then makes admin-only calls (e.g.,
d.accessManager.grantRole(POOL_ADMIN_ROLE,...)) as cfg.deployer, causing reverts
when cfg.owner != cfg.deployer; fix by executing the admin-scoped operations
under cfg.owner (either wrap the admin sequence that interacts with
AccessManager — including grantRole on AccessManager and any subsequent admin
calls related to AccessManager, MARKPool, RYLACreditLedger, MARKWithdrawAdapter
setup — in a vm.prank(cfg.owner) or switch the broadcast to cfg.owner for that
section) so the AccessManager sees msg.sender == cfg.owner during those calls.
In `@contracts/src/pool/MARKPool.sol`:
- Around line 465-472: Add an explicit guard in _applyFee to ensure ASSET_LEDGER
is configured before it's used: check that address(ASSET_LEDGER) != address(0)
(and revert with a clear message like "Asset ledger not set") before calling
ASSET_LEDGER.credit or emitting FeePaid; place this check early in _applyFee
(e.g., immediately after the fee==0 return or before the relayerAmount > 0
branch) so attempts to use ASSET_LEDGER prior to setAssetLedger() fail with a
readable error instead of an opaque external-call revert.
---
Duplicate comments:
In `@contracts/src/pool/MARKPool.sol`:
- Around line 379-388: The bridgeIn function currently accepts duplicate
deliveries because it lacks a processed-message guard; update bridgeIn to accept
a unique srcMessageId (e.g., add a bytes32 or uint256 srcMessageId parameter),
check a processed mapping (e.g.,
processedBridgeMessages[srcChainId][srcMessageId]) and revert on replay, mark
the srcMessageId as processed before calling _insertCommitments(outCommitments),
and then emit BridgeIn as before; ensure you add the storage mapping and any
accessor/helper (processedBridgeMessages) used to record and check processed
messages.
In `@contracts/src/withdraw/MARKWithdrawAdapter.sol`:
- Around line 111-121: The contract is using OpenZeppelin's
MessageHashUtils.toEthSignedMessageHash (EIP-191) for
computeWithdrawIntentDigest (and the other usage around the withdraw
verification logic), but the intent is typed data (EIP-712); replace the
toEthSignedMessageHash call with the EIP-712 domain+typed-data flow by computing
the struct hash for the withdraw intent (use a WITHDRAW_INTENT_TYPEHASH and
computeWithdrawIntentHash or build
keccak256(abi.encode(WITHDRAW_INTENT_TYPEHASH, creditOwner, recipient, amount,
nullifiers, nonce, deadline))) and then call _hashTypedDataV4(structHash) from
the EIP712 base (instead of MessageHashUtils.toEthSignedMessageHash). Update
computeWithdrawIntentDigest and any other places that call
toEthSignedMessageHash (e.g., the verify/validation code that reads the intent
digest) to use _hashTypedDataV4 so signatures match EIP-712 typed-data
semantics.
- Line 147: withdrawWithSig currently calls ASSET_LEDGER.debit directly which
will hard-revert under the pool-restricted ledger model; replace the direct
debit with the ledger’s pool-authorized debit API (e.g.
ASSET_LEDGER.debitFromPool or ASSET_LEDGER.debitFrom(poolAddress/ poolId,
creditOwner, amount)) or route the debit through the Pool contract (e.g.
POOL.performDebit/ poolAuthorizedDebit) so the ledger sees an authorized pool
caller; update withdrawWithSig to use that pool-authorized method and pass the
adapter’s pool identifier/address when invoking the ledger.
🪄 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: 127a5593-2d3f-46b8-8269-88eb97907e79
📒 Files selected for processing (7)
contracts/ARCHITECTURE.mdcontracts/script/ci/architecture-guard.shcontracts/script/deploy/pool/DeployMARKPool.s.solcontracts/src/pool/MARKPool.solcontracts/src/withdraw/MARKWithdrawAdapter.solcontracts/test/invariant/pool/MARKPoolInvariants.t.solcontracts/test/unit/pool/MARKPool.t.sol
Comment on lines
+47
to
+54
| '^import\s+.*"(?:\.\.\/settlement\/|\.\.\/bridge\/|src\/settlement\/|src\/bridge\/)' \ | ||
| "pool -> settlement/bridge" | ||
|
|
||
| # Withdraw contracts must not depend on settlement or bridge concrete contracts. | ||
| check_no_imports \ | ||
| "src/withdraw" \ | ||
| '^import\s+.*"(?:\.\.\/settlement\/|\.\.\/bridge\/|src\/settlement\/|src\/bridge\/)' \ | ||
| "withdraw -> settlement/bridge" |
There was a problem hiding this comment.
Forbidden-import regex misses nested ../../src/... paths.
Line 47 and Line 53 don’t match imports like ../../src/settlement/... or ../../src/bridge/..., so nested contracts can bypass this CI rule.
Suggested hardening
- '^import\s+.*"(?:\.\.\/settlement\/|\.\.\/bridge\/|src\/settlement\/|src\/bridge\/)' \
+ '^import\s+.*"(?:\.\.\/(?:\.\.\/)*src\/(?:settlement|bridge)\/|\.\.\/(?:settlement|bridge)\/|src\/(?:settlement|bridge)\/)' \🤖 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/script/ci/architecture-guard.sh` around lines 47 - 54, The existing
forbidden-import regex used in the check_no_imports calls (the pattern starting
'^import\s+.*"(?:\.\.\/settlement\/|\.\.\/bridge\/|src\/settlement\/|src\/bridge\/)')
doesn't match nested paths like ../../src/settlement or ../../settlement; update
both regex occurrences passed to check_no_imports (the pool -> settlement/bridge
and withdraw -> settlement/bridge checks) to allow any number of "../" segments
and an optional "src/" prefix before "settlement" or "bridge" (e.g. use a
pattern that accepts (?:\.\.\/)+ and optional src/ before (?:settlement|bridge)
), so imports like ../../src/settlement/... are caught.
- DeployMARKPool.s.sol: grant POOL_ADMIN_ROLE to deployer during setup so setAssetLedger/setIntentSigner calls succeed when deployer != owner; revoke deployer role after setup completes - MARKPool._applyFee: revert InvalidAssetLedger if ASSET_LEDGER is not set and a non-zero fee is applied (prevents silent call to address(0))
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 220-228: The FixedFeePolicy error used in setMinFee is ambiguous
given the function constrains newMinFee to 0 or 1; either rename the error to
something descriptive like MinFeeTooLarge (or MinFeeExceedsMaximum) and update
all references, or add a clarifying NatSpec/comment to the FixedFeePolicy error
definition explaining it means "min fee must be 0 or 1". Update the revert in
setMinFee (and any other uses) to use the new error name or keep the same name
but ensure the error's definition includes the explanatory note; refer to
setMinFee, minFee, FixedFeePolicy and MinFeeSet when making the changes.
🪄 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: 21cd4d11-ba48-4a30-bf5a-dd54e6c4b7e4
📒 Files selected for processing (2)
contracts/script/deploy/pool/DeployMARKPool.s.solcontracts/src/pool/MARKPool.sol
Comment on lines
+220
to
+228
| function setMinFee(uint256 newMinFee) external restricted { | ||
| // Circuit enforces percentage fee; runtime floor is a narrow safety guard only. | ||
| // Allowed values are intentionally constrained to 0/1 credit unit. | ||
| if (newMinFee > 1) revert FixedFeePolicy(); | ||
| if (newMinFee != minFee) { | ||
| minFee = newMinFee; | ||
| emit MinFeeSet(newMinFee); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
The setMinFee comment says 0 or 1, but the error name FixedFeePolicy may be confusing.
The comment explains that values are constrained to 0/1, but the error FixedFeePolicy doesn't clearly convey "value must be ≤ 1". Consider a more descriptive error like MinFeeTooLarge or add a brief note in the error definition.
🤖 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/src/pool/MARKPool.sol` around lines 220 - 228, The FixedFeePolicy
error used in setMinFee is ambiguous given the function constrains newMinFee to
0 or 1; either rename the error to something descriptive like MinFeeTooLarge (or
MinFeeExceedsMaximum) and update all references, or add a clarifying
NatSpec/comment to the FixedFeePolicy error definition explaining it means "min
fee must be 0 or 1". Update the revert in setMinFee (and any other uses) to use
the new error name or keep the same name but ensure the error's definition
includes the explanatory note; refer to setMinFee, minFee, FixedFeePolicy and
MinFeeSet when making the changes.
circuits/build/ is gitignored so the WASM and witness_calculator.js are not in the repo. Add circom install and npm run build steps before npm test so CI compiles the circuit fresh on each run.
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 @.github/workflows/circuits-ci.yml:
- Around line 33-35: The workflow currently writes circom to /usr/local/bin
using the commands "curl -L ... -o /usr/local/bin/circom" and "chmod +x
/usr/local/bin/circom", which can fail due to permission errors; update the step
to either (A) run the install commands with elevation (prefix curl and chmod
with sudo) or (B) install into a user-writable location like $HOME/.local/bin
(create it with mkdir -p $HOME/.local/bin, download circom there, chmod +x, and
export PATH="$HOME/.local/bin:$PATH") before running "circom --version" so the
runner can write the file without root permission issues.
- Around line 23-27: The CI step named "Setup Node.js" currently pins
node-version '20' using the actions/setup-node@v6 action; update the workflow to
use Node.js 24 by changing the node-version value to '24' (keep the step name
"Setup Node.js" and the same action unless you also want to update the action
version), ensuring the GitHub Actions runner will use the Active LTS Node
runtime.
🪄 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: c0042e34-e542-4997-9525-f7dbbdc5bc57
📒 Files selected for processing (1)
.github/workflows/circuits-ci.yml
Comment on lines
+23
to
+27
| - name: Setup Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: '20' | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to nodejs.org, what is the EOL date for Node.js 20 (Iron), and which Node.js major versions are currently Active LTS?
💡 Result:
EOL date for Node.js 20 (“Iron”): 2026-03-24 [1] Node.js major versions currently Active LTS (as of 2026-05-12): v24 (“Krypton”) is Active LTS; v22 is Maintenance LTS; v20 is Maintenance LTS [2][3][4]. So, the currently Active LTS major version is: Node.js 24 (Krypton) [2][3].
Citations:
- 1: https://nodejs.org/en/eol
- 2: https://nodejs.dev/en/about/releases/
- 3: https://github.com/nodejs/release
- 4: https://github.com/nodejs/Release
Use an Active LTS Node runtime in CI.
Line 26 pins Node.js 20, which reached EOL on March 24, 2026. As of May 2026, only Node.js 24 ("Krypton") is in Active LTS; Node.js 22 is in Maintenance LTS. Please upgrade to Node.js 24.
Suggested change
- name: Setup Node.js
uses: actions/setup-node@v6
with:
- node-version: '20'
+ node-version: '24'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Setup Node.js | |
| uses: actions/setup-node@v6 | |
| with: | |
| node-version: '20' | |
| - name: Setup Node.js | |
| uses: actions/setup-node@v6 | |
| with: | |
| node-version: '24' |
🤖 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 @.github/workflows/circuits-ci.yml around lines 23 - 27, The CI step named
"Setup Node.js" currently pins node-version '20' using the actions/setup-node@v6
action; update the workflow to use Node.js 24 by changing the node-version value
to '24' (keep the step name "Setup Node.js" and the same action unless you also
want to update the action version), ensuring the GitHub Actions runner will use
the Active LTS Node runtime.
- Rename immutables to SCREAMING_SNAKE_CASE: assetLedger->ASSET_LEDGER, proofPool->PROOF_POOL (MARKPool.sol, MARKWithdrawAdapter.sol) - MARKPool: remove _assetLedger from constructor, add setAssetLedger() one-time restricted setter to break circular deploy dependency with RYLACreditLedger - MARKPool: add withdrawal flow documentation to contract NatSpec - ARCHITECTURE.md: add pool/withdraw domains, dependency rules, and withdrawal flow explanation - DeployMARKPool.s.sol: deployment script for MARKPool, RYLACreditLedger, MARKWithdrawAdapter with AccessManager configuration - MARKPoolInvariants.t.sol: 3 invariants (nullifiers never unspent, withdraw bindings immutable, root queue only grows) - architecture-guard.sh: add pool and withdraw domain isolation rules
- circuits-ci.yml: updated to run MARKPool witness tests - circuits/package.json: build/test scripts point to MARKPool.circom - circuits/setup.mjs: updated for MARKPool.circom trusted setup - circuits/test/MARKPool.test.mjs: cleaned up test file - contracts/KNOWN_ISSUES.md: updated KI-7 for current two-circuit state - contracts/src/pool/errors/PoolErrors.sol: add missing blank line
| const out1Secret = 777n; const out1Blinding = 888n; const out1Amount = 100n; | ||
| const fee = 500n; // 500 = 500 (in0+in1=1000, out0+out1=500, fee=500, withdraw=0) | ||
|
|
||
| const path0 = buildMerklePath(in0.commitment, DEPTH); |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
contracts/src/pool/MARKPool.sol (1)
72-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope
bridgeInreplay protection by source chain.At Line 390–Line 391, replay protection is keyed only by
messageId. If two source chains emit the same ID format, one can block the other. Key replay state by(srcChainId, messageId).Suggested fix
- mapping(bytes32 => bool) public processedBridgeMessages; + mapping(bytes32 => bool) public processedBridgeMessages; @@ - if (processedBridgeMessages[messageId]) revert BridgeMessageAlreadyProcessed(); - processedBridgeMessages[messageId] = true; + bytes32 replayKey = keccak256(abi.encode(srcChainId, messageId)); + if (processedBridgeMessages[replayKey]) revert BridgeMessageAlreadyProcessed(); + processedBridgeMessages[replayKey] = true;Also applies to: 390-391
🤖 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/src/pool/MARKPool.sol` around lines 72 - 73, The replay-protection mapping processedBridgeMessages is only keyed by messageId which allows collisions across source chains; update the logic to scope replay state by (srcChainId, messageId) instead: change the mapping key usage in the bridgeIn processing and any reads/writes (references: processedBridgeMessages and any place setting/checking it, e.g., inside bridgeIn) to use a composite key like keccak256(abi.encodePacked(srcChainId, messageId)), and do the same for nullifierWithdrawBinding if it is tied to bridge messages; ensure all checks and assignments that previously used messageId alone now use the composite key so different srcChainIds cannot conflict.contracts/test/unit/pool/MARKPool.t.sol (1)
294-297:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the exact revert in unauthorized
bridgeIntest.Line 296 uses a broad
vm.expectRevert(), which can pass on unrelated failures. Assert the specific AccessManaged unauthorized selector instead.🤖 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 294 - 297, Replace the broad vm.expectRevert() in testBridgeInRevertsWhenCallerNotRestricted with an assertion for the specific AccessManaged unauthorized error: call vm.expectRevert(abi.encodeWithSelector(AccessManaged.Unauthorized.selector)) before invoking pool.bridgeIn(...) so the test only passes for the intended unauthorized revert coming from the AccessManaged contract..github/workflows/circuits-ci.yml (1)
30-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInstall
circomin a user-writable path (or usesudo).Line 30 writes to
/usr/local/bindirectly; this commonly fails on GitHub-hosted runners due to permissions.Suggested fix
- name: Install circom run: | - curl -L https://github.com/iden3/circom/releases/latest/download/circom-linux-amd64 -o /usr/local/bin/circom - chmod +x /usr/local/bin/circom + mkdir -p "$HOME/.local/bin" + curl -fsSL https://github.com/iden3/circom/releases/latest/download/circom-linux-amd64 -o "$HOME/.local/bin/circom" + chmod +x "$HOME/.local/bin/circom" + echo "$HOME/.local/bin" >> "$GITHUB_PATH"🤖 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 @.github/workflows/circuits-ci.yml around lines 30 - 31, The workflow writes circom to /usr/local/bin which can fail due to permissions; change the install to a user-writable path (e.g., $HOME/.local/bin or $GITHUB_WORKSPACE/.local/bin) or prefix the curl/chmod commands with sudo. Update the two commands that reference /usr/local/bin/circom (the curl -o and chmod +x lines) to write to and set executable on the user-writable path (and ensure that path is added to PATH in the workflow) or use sudo before both commands if you intentionally want to write to /usr/local/bin.
🤖 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 @.github/workflows/circuits-ci.yml:
- Line 30: Replace the mutable download of circom by pinning to a specific
release and verifying its integrity: change the curl invocation that fetches
"circom-linux-amd64" (the line using curl -L ... -o /usr/local/bin/circom) to
download from a fixed tag URL (e.g.
/releases/download/vX.Y.Z/circom-linux-amd64), add a step to download the
corresponding published checksum (or vendor-provided SHA256/GPG signature),
verify the binary with sha256sum --check (or gpg --verify) before moving it into
/usr/local/bin, and use curl --fail and chmod +x only after verification to
ensure reproducible and secure installation.
In `@circuits/setup.mjs`:
- Around line 14-15: The contribution call using powersOfTau.contribute
currently constructs entropy with the predictable string 'markpool-entropy-' +
Date.now(); replace this with cryptographically secure randomness (e.g.,
generate random bytes via Node's crypto.randomBytes or Web Crypto
getRandomValues and encode as hex/base64) and use that value in both
powersOfTau.contribute calls so the entropy argument is unpredictable; update
the code paths that create the 'markpool-entropy-...' value to use the secure
random generator and propagate the encoded result into the contribute() calls.
In `@contracts/KNOWN_ISSUES.md`:
- Line 85: The current text calls MARKPoolVerifier "the settlement-specific
verifier contract", which is misleading; update the wording in KNOWN_ISSUES.md
so it states that MARKPoolVerifier is the shared pool verifier used for both
pool operations and settlement verification (e.g., replace "settlement-specific
verifier contract (MARKPoolVerifier)" with "shared pool verifier contract
(MARKPoolVerifier) used for both pool operations and settlement verification"),
and ensure the surrounding sentence that introduces MARKPoolVerifier as part of
the pool system (the paragraph referencing MARKPoolVerifier) reflects this
shared usage.
---
Duplicate comments:
In @.github/workflows/circuits-ci.yml:
- Around line 30-31: The workflow writes circom to /usr/local/bin which can fail
due to permissions; change the install to a user-writable path (e.g.,
$HOME/.local/bin or $GITHUB_WORKSPACE/.local/bin) or prefix the curl/chmod
commands with sudo. Update the two commands that reference /usr/local/bin/circom
(the curl -o and chmod +x lines) to write to and set executable on the
user-writable path (and ensure that path is added to PATH in the workflow) or
use sudo before both commands if you intentionally want to write to
/usr/local/bin.
In `@contracts/src/pool/MARKPool.sol`:
- Around line 72-73: The replay-protection mapping processedBridgeMessages is
only keyed by messageId which allows collisions across source chains; update the
logic to scope replay state by (srcChainId, messageId) instead: change the
mapping key usage in the bridgeIn processing and any reads/writes (references:
processedBridgeMessages and any place setting/checking it, e.g., inside
bridgeIn) to use a composite key like keccak256(abi.encodePacked(srcChainId,
messageId)), and do the same for nullifierWithdrawBinding if it is tied to
bridge messages; ensure all checks and assignments that previously used
messageId alone now use the composite key so different srcChainIds cannot
conflict.
In `@contracts/test/unit/pool/MARKPool.t.sol`:
- Around line 294-297: Replace the broad vm.expectRevert() in
testBridgeInRevertsWhenCallerNotRestricted with an assertion for the specific
AccessManaged unauthorized error: call
vm.expectRevert(abi.encodeWithSelector(AccessManaged.Unauthorized.selector))
before invoking pool.bridgeIn(...) so the test only passes for the intended
unauthorized revert coming from the AccessManaged contract.
🪄 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: a78a9e44-734a-4f37-bb6c-63973aefe5ab
📒 Files selected for processing (8)
.github/workflows/circuits-ci.ymlcircuits/package.jsoncircuits/setup.mjscircuits/test/MARKPool.test.mjscontracts/KNOWN_ISSUES.mdcontracts/src/pool/MARKPool.solcontracts/src/pool/errors/PoolErrors.solcontracts/test/unit/pool/MARKPool.t.sol
- circuits-ci.yml: fix circom install permissions (use sudo mv to /usr/local/bin instead of direct write which fails on GH Actions) - PoolErrors.sol: add clarifying comment to FixedFeePolicy explaining it fires when minFee > 1 (not a fee-rate policy, a range guard) - MARKWithdrawAdapter.sol: document personal_sign intent on computeWithdrawIntentDigest (EIP-191 is intentional, not EIP-712) bridgeIn replay protection finding: already fixed in current code (processedBridgeMessages mapping + check at line 390) — stale finding.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
contracts/KNOWN_ISSUES.md (1)
84-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify verifier ownership wording at Line 84.
MARKPoolVerifieris described as part of the pool system (Line 83), so calling it “settlement-specific” is misleading. Reword as a shared pool verifier wired into settlement.Proposed wording
-- **Settlement system** (`MARKSettlementModule` + `Groth16SettlementVerifier`): expects the same 13-signal layout via `IGroth16Verifier`. The settlement-specific verifier contract (`MARKPoolVerifier`) needs to be wired into `Groth16SettlementVerifier.setVerifierContract()` before ZK-based settlement is active. `AttestedSettlementVerifier` is the production-safe fallback until that wiring is complete. +- **Settlement system** (`MARKSettlementModule` + `Groth16SettlementVerifier`): expects the same 13-signal layout via `IGroth16Verifier`. The shared pool verifier contract (`MARKPoolVerifier`) must be wired into `Groth16SettlementVerifier.setVerifierContract()` before ZK-based settlement is active. `AttestedSettlementVerifier` is the production-safe fallback until that wiring is complete.🤖 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/KNOWN_ISSUES.md` at line 84, Update the wording to clarify that MARKPoolVerifier is a shared pool verifier (not "settlement-specific") that must be wired into the settlement flow; specifically, replace the phrase "The settlement-specific verifier contract (MARKPoolVerifier)" with wording that MARKPoolVerifier is the pool's shared verifier and must be set on Groth16SettlementVerifier via setVerifierContract() so the MARKSettlementModule (which uses IGroth16Verifier's 13-signal layout) can perform ZK-based settlement, with AttestedSettlementVerifier remaining the fallback until wiring is complete.circuits/setup.mjs (1)
14-15:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse cryptographically secure entropy for setup contributions.
Lines 15 and 25 derive entropy from
Date.now(), which is predictable and weakens trusted-setup guarantees.Suggested fix
+import { randomBytes } from 'crypto'; import { zKey, powersOfTau } from 'snarkjs'; import { writeFileSync } from 'fs'; @@ await powersOfTau.contribute('build/pot15_0000.ptau', 'build/pot15_final.ptau', - 'MARK Protocol', 'markpool-entropy-' + Date.now()); + 'MARK Protocol', `markpool-entropy-${randomBytes(32).toString('hex')}`); @@ await zKey.contribute('build/markpool_0000.zkey', 'build/markpool_final.zkey', - 'MARK Protocol MARKPool', 'markpool-zkey-entropy-' + Date.now()); + 'MARK Protocol MARKPool', `markpool-zkey-entropy-${randomBytes(32).toString('hex')}`);Also applies to: 24-25
🤖 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 `@circuits/setup.mjs` around lines 14 - 15, The contribution call to powersOfTau.contribute currently builds entropy using the predictable Date.now() string ('markpool-entropy-' + Date.now()), which weakens the trusted setup; replace that construction with cryptographically secure randomness (e.g., generate a secure random buffer/hex string via Node's crypto.randomBytes or Web Crypto getRandomValues) and pass that entropy string to powersOfTau.contribute (update all occurrences that use 'markpool-entropy-' + Date.now()), ensuring the entropy is unpredictable and properly encoded before calling the contribute method..github/workflows/circuits-ci.yml (2)
23-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse an Active LTS Node runtime in CI.
Line 26 still pins Node
20; this is past EOL and should be upgraded to the current Active LTS line.Suggested fix
- name: Setup Node.js uses: actions/setup-node@v6 with: - node-version: '20' + node-version: '24'🤖 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 @.github/workflows/circuits-ci.yml around lines 23 - 27, The CI workflow step using actions/setup-node@v6 currently pins node-version: '20' which is past EOL; update the node-version value under the "Setup Node.js" step (actions/setup-node@v6) to an active LTS (either a specific supported major like '22' or use the dynamic alias 'lts/*') so the runner uses a current, supported Node runtime.
28-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin and verify the
circombinary instead of downloadinglatest.Line 30 pulls a mutable artifact (
releases/latest) with no checksum validation, which is a CI supply-chain risk.Suggested fix
- name: Install circom run: | - curl -L https://github.com/iden3/circom/releases/latest/download/circom-linux-amd64 -o circom + CIRCOM_VERSION="v2.1.9" + CIRCOM_URL="https://github.com/iden3/circom/releases/download/${CIRCOM_VERSION}/circom-linux-amd64" + CIRCOM_SHA256="<official_sha256>" + curl -fsSL "$CIRCOM_URL" -o circom + echo "${CIRCOM_SHA256} circom" | sha256sum -c - chmod +x circom sudo mv circom /usr/local/bin/circom🤖 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 @.github/workflows/circuits-ci.yml around lines 28 - 32, The CI step named "Install circom" currently downloads the mutable releases/latest binary via curl with no verification; change it to download a pinned release (use the specific tag URL like .../download/vX.Y.Z/circom) and verify the binary checksum before installing: add a step to fetch or embed the expected SHA256 (from a maintained source or CI secret/variable), compute the downloaded file's checksum (sha256sum) and fail if it doesn't match, then chmod +x and sudo mv /usr/local/bin/circom only after successful verification so the curl command in the "Install circom" step is both version-pinned and integrity-checked.contracts/test/unit/pool/MARKPool.t.sol (1)
294-297:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the exact revert for unauthorized
bridgeIn.A bare
vm.expectRevert()will pass on any failure, so this test won’t catch regressions wherebridgeInreverts for the wrong reason. Use the concreteAccessManaged/AccessManagerselector instead.In OpenZeppelin Contracts v5, what custom error is reverted when an unauthorized caller invokes an `AccessManaged` function guarded by `restricted`?🤖 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 294 - 297, Replace the bare vm.expectRevert() in testBridgeInRevertsWhenCallerNotRestricted with a concrete expectRevert that matches the AccessManaged unauthorized custom error; specifically use vm.expectRevert(abi.encodeWithSelector(AccessManaged.Unauthorized.selector)) (or AccessManager.Unauthorized.selector if your contract imports that type) before calling pool.bridgeIn(...) so the test asserts the exact "Unauthorized" revert from AccessManaged rather than any revert.contracts/src/withdraw/MARKWithdrawAdapter.sol (1)
152-152:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
withdrawWithSigdebits from the wrong caller context.Line 152 calls
ASSET_LEDGER.debit(creditOwner, amount)from the adapter, but this PR’sRYLACreditLedger.debitis described asonlyPool. With the production ledger wired in, withdrawals will revert here becausemsg.senderis the adapter, notMARKPool. Route the burn through the pool or explicitly authorize the adapter in the ledger flow.🤖 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/src/withdraw/MARKWithdrawAdapter.sol` at line 152, withdrawWithSig currently calls ASSET_LEDGER.debit(creditOwner, amount) directly which fails because RYLACreditLedger.debit is onlyPool and expects MARKPool as msg.sender; instead route the burn through the pool or authorize the adapter. Update withdrawWithSig to call the MARKPool entrypoint that performs the ledger debit (e.g., MARKPool.withdrawCredit / MARKPool.burnCredit / whatever pool method exists that then calls ASSET_LEDGER.debit) so the ledger sees the pool as caller, or alternatively add explicit adapter authorization in RYLACreditLedger (modify RYLACreditLedger.debit’s access control to accept this adapter) and remove the direct ASSET_LEDGER.debit call. Ensure you reference withdrawWithSig, ASSET_LEDGER.debit, RYLACreditLedger.debit and MARKPool in your change.
🤖 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/setup.mjs`:
- Around line 32-33: The dynamic import with an import assertion for the EJS
template (const { default: solidityTemplate } = await import(...)) is
incompatible with Node 20+ and .ejs text imports; replace it by reading the
template as text (e.g., use readFile from 'node:fs/promises' to read
'node_modules/snarkjs/templates/verifier_groth16.sol.ejs' with 'utf8' and assign
the resulting string to solidityTemplate) and then call
zKey.exportSolidityVerifier('build/markpool_final.zkey', { groth16:
solidityTemplate }); alternatively, if you prefer, use createRequire from
'module' to resolve the module path and then read the file with fs; ensure
solidityTemplate is a string passed into zKey.exportSolidityVerifier.
- Around line 11-12: The script calls powersOfTau.newAccumulator('bn128', 15,
'build/pot15_0000.ptau') but never creates the build directory so the write can
ENOENT; before that call ensure the output directory exists (e.g., call
fs.promises.mkdir('build', { recursive: true }) or fs.mkdirSync('build', {
recursive: true })) and handle/await the promise so the directory is created
prior to invoking newAccumulator; add the mkdir call just before the
powersOfTau.newAccumulator invocation to guarantee the path for
'build/pot15_0000.ptau' exists.
In `@contracts/src/pool/MARKPool.sol`:
- Around line 29-32: Update the NatSpec in MARKPool.sol to correctly reflect the
signature scheme: change the line that claims
MARKWithdrawAdapter.withdrawWithSig() verifies EIP-712 signatures to state it
verifies EIP-191 / personal_sign signatures (via
MessageHashUtils.toEthSignedMessageHash). Mention
MARKWithdrawAdapter.withdrawWithSig() and
MessageHashUtils.toEthSignedMessageHash by name so integrators know the contract
expects personal_sign/EIP-191-style signatures rather than EIP-712 typed-data
signatures.
In `@contracts/test/unit/pool/MARKPool.t.sol`:
- Around line 292-305: Add a regression test that proves replay-protection on
messageId by invoking pool.bridgeIn twice with the same messageId and expecting
the second call to revert with BridgeMessageAlreadyProcessed; implement a new
test (e.g., testBridgeInRevertsOnReplayOfMessageId) that sets up commitments =
[C0, C1], uses vm.prank(admin) to call pool.bridgeIn(targetChainId,
bytes32(uint256(1)), commitments) once successfully and then calls it again with
the same messageId and wraps the second call with
vm.expectRevert(BridgeMessageAlreadyProcessed.selector) to assert the replay is
rejected.
---
Duplicate comments:
In @.github/workflows/circuits-ci.yml:
- Around line 23-27: The CI workflow step using actions/setup-node@v6 currently
pins node-version: '20' which is past EOL; update the node-version value under
the "Setup Node.js" step (actions/setup-node@v6) to an active LTS (either a
specific supported major like '22' or use the dynamic alias 'lts/*') so the
runner uses a current, supported Node runtime.
- Around line 28-32: The CI step named "Install circom" currently downloads the
mutable releases/latest binary via curl with no verification; change it to
download a pinned release (use the specific tag URL like
.../download/vX.Y.Z/circom) and verify the binary checksum before installing:
add a step to fetch or embed the expected SHA256 (from a maintained source or CI
secret/variable), compute the downloaded file's checksum (sha256sum) and fail if
it doesn't match, then chmod +x and sudo mv /usr/local/bin/circom only after
successful verification so the curl command in the "Install circom" step is both
version-pinned and integrity-checked.
In `@circuits/setup.mjs`:
- Around line 14-15: The contribution call to powersOfTau.contribute currently
builds entropy using the predictable Date.now() string ('markpool-entropy-' +
Date.now()), which weakens the trusted setup; replace that construction with
cryptographically secure randomness (e.g., generate a secure random buffer/hex
string via Node's crypto.randomBytes or Web Crypto getRandomValues) and pass
that entropy string to powersOfTau.contribute (update all occurrences that use
'markpool-entropy-' + Date.now()), ensuring the entropy is unpredictable and
properly encoded before calling the contribute method.
In `@contracts/KNOWN_ISSUES.md`:
- Line 84: Update the wording to clarify that MARKPoolVerifier is a shared pool
verifier (not "settlement-specific") that must be wired into the settlement
flow; specifically, replace the phrase "The settlement-specific verifier
contract (MARKPoolVerifier)" with wording that MARKPoolVerifier is the pool's
shared verifier and must be set on Groth16SettlementVerifier via
setVerifierContract() so the MARKSettlementModule (which uses IGroth16Verifier's
13-signal layout) can perform ZK-based settlement, with
AttestedSettlementVerifier remaining the fallback until wiring is complete.
In `@contracts/src/withdraw/MARKWithdrawAdapter.sol`:
- Line 152: withdrawWithSig currently calls ASSET_LEDGER.debit(creditOwner,
amount) directly which fails because RYLACreditLedger.debit is onlyPool and
expects MARKPool as msg.sender; instead route the burn through the pool or
authorize the adapter. Update withdrawWithSig to call the MARKPool entrypoint
that performs the ledger debit (e.g., MARKPool.withdrawCredit /
MARKPool.burnCredit / whatever pool method exists that then calls
ASSET_LEDGER.debit) so the ledger sees the pool as caller, or alternatively add
explicit adapter authorization in RYLACreditLedger (modify
RYLACreditLedger.debit’s access control to accept this adapter) and remove the
direct ASSET_LEDGER.debit call. Ensure you reference withdrawWithSig,
ASSET_LEDGER.debit, RYLACreditLedger.debit and MARKPool in your change.
In `@contracts/test/unit/pool/MARKPool.t.sol`:
- Around line 294-297: Replace the bare vm.expectRevert() in
testBridgeInRevertsWhenCallerNotRestricted with a concrete expectRevert that
matches the AccessManaged unauthorized custom error; specifically use
vm.expectRevert(abi.encodeWithSelector(AccessManaged.Unauthorized.selector)) (or
AccessManager.Unauthorized.selector if your contract imports that type) before
calling pool.bridgeIn(...) so the test asserts the exact "Unauthorized" revert
from AccessManaged rather than any revert.
🪄 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: a8f51f1a-f215-4ac2-865d-6e829f502b3f
📒 Files selected for processing (9)
.github/workflows/circuits-ci.ymlcircuits/package.jsoncircuits/setup.mjscircuits/test/MARKPool.test.mjscontracts/KNOWN_ISSUES.mdcontracts/src/pool/MARKPool.solcontracts/src/pool/errors/PoolErrors.solcontracts/src/withdraw/MARKWithdrawAdapter.solcontracts/test/unit/pool/MARKPool.t.sol
Comment on lines
+292
to
+305
| // --- bridgeIn access control --- | ||
|
|
||
| function testBridgeInRevertsWhenCallerNotRestricted() public { | ||
| bytes32[2] memory commitments = [C0, C1]; | ||
| vm.expectRevert(); | ||
| pool.bridgeIn(901, bytes32(uint256(1)), commitments); | ||
| } | ||
|
|
||
| function testWithdrawRevertsOnAmountMismatch() public { | ||
| vm.prank(operator); | ||
| pool.commit(COMMITMENT, AMOUNT); | ||
| function testBridgeInRevertsOnSameChain() public { | ||
| bytes32[2] memory commitments = [C0, C1]; | ||
| vm.prank(admin); | ||
| vm.expectRevert(PoolErrors.SourceIsDestination.selector); | ||
| pool.bridgeIn(block.chainid, bytes32(uint256(1)), commitments); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a replay-protection regression test for messageId.
This suite covers bridgeIn access control and same-chain rejection, but it never proves that a second delivery of the same messageId reverts with BridgeMessageAlreadyProcessed. That is the core replay fix in this PR, so it should be locked in with a direct test.
🤖 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 292 - 305, Add a
regression test that proves replay-protection on messageId by invoking
pool.bridgeIn twice with the same messageId and expecting the second call to
revert with BridgeMessageAlreadyProcessed; implement a new test (e.g.,
testBridgeInRevertsOnReplayOfMessageId) that sets up commitments = [C0, C1],
uses vm.prank(admin) to call pool.bridgeIn(targetChainId, bytes32(uint256(1)),
commitments) once successfully and then calls it again with the same messageId
and wraps the second call with
vm.expectRevert(BridgeMessageAlreadyProcessed.selector) to assert the replay is
rejected.
- setup.mjs: use crypto.randomBytes for ceremony entropy (Date.now is predictable), add mkdirSync for build/, fix EJS template loading to use readFileSync instead of dynamic import with assert (unsupported in Node 20/22/24 ESM) - circuits-ci.yml: pin circom to v2.2.3 instead of latest, add version verification step - KNOWN_ISSUES.md: fix misleading 'settlement-specific verifier' wording — MARKPoolVerifier is a shared pool verifier, not settlement-specific - MARKPool.sol: fix NatSpec EIP-712 reference to EIP-191 (personal_sign)
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/circuits-ci.yml:
- Around line 11-19: The workflow job "circuits-test" lacks an explicit
permissions block; add a top-level permissions mapping under the circuits-test
job that declares only the least-privilege scopes this job needs (e.g., set
contents: read and any other minimal tokens the job actually uses such as
id-token: write or checks: read only if required), placing the permissions block
directly under the job definition so the job no longer inherits broader repo/org
defaults.
In `@circuits/setup.mjs`:
- Around line 38-39: The code uses URL.pathname which can produce invalid
Windows paths and won't decode percent-encoding; instead convert the file URL to
a proper file system path using fileURLToPath before reading. Replace usage of
new URL(...).pathname when computing templatePath with fileURLToPath(new
URL('node_modules/snarkjs/templates/verifier_groth16.sol.ejs', import.meta.url))
and then pass that result to readFileSync to set solidityTemplate so
readFileSync receives a correct platform-safe path.
In `@contracts/src/pool/MARKPool.sol`:
- Line 389: The check ensuring messageId != bytes32(0) currently reverts with
InvalidRoot(), which is semantically wrong; add a new error InvalidMessageId()
to PoolErrors (or the contract's errors list) and replace the revert on
messageId validation in MARKPool (the line using messageId == bytes32(0)) to
revert InvalidMessageId() so the error accurately reflects the failing
condition.
🪄 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: 6005412f-9ccd-41bc-bda7-2bad36b81d91
📒 Files selected for processing (4)
.github/workflows/circuits-ci.ymlcircuits/setup.mjscontracts/KNOWN_ISSUES.mdcontracts/src/pool/MARKPool.sol
Comment on lines
+11
to
+19
| jobs: | ||
| circuits-test: | ||
| name: Circuits Witness Tests | ||
| runs-on: ubuntu-latest | ||
| defaults: | ||
| run: | ||
| working-directory: circuits | ||
|
|
||
| steps: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Set explicit least-privilege token permissions for the job.
Define permissions explicitly to avoid inheriting broader repo/org defaults.
Suggested change
jobs:
circuits-test:
name: Circuits Witness Tests
runs-on: ubuntu-latest
+ permissions:
+ contents: read
defaults:
run:
working-directory: circuits🤖 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 @.github/workflows/circuits-ci.yml around lines 11 - 19, The workflow job
"circuits-test" lacks an explicit permissions block; add a top-level permissions
mapping under the circuits-test job that declares only the least-privilege
scopes this job needs (e.g., set contents: read and any other minimal tokens the
job actually uses such as id-token: write or checks: read only if required),
placing the permissions block directly under the job definition so the job no
longer inherits broader repo/org defaults.
Comment on lines
+38
to
+39
| const templatePath = new URL('node_modules/snarkjs/templates/verifier_groth16.sol.ejs', import.meta.url).pathname; | ||
| const solidityTemplate = readFileSync(templatePath, 'utf8'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js ESM, is using new URL(...).pathnamesafe forfs.readFileSyncpaths across platforms, or shouldfileURLToPath(new URL(...)) be used? Please include Windows/path-encoding behavior.
💡 Result:
Not safe: in Node.js ESM, using new URL(...).pathname as a filesystem path is not portable and can produce incorrect/invalid Windows paths (e.g., leading /C:/... instead of C:\..., and missing proper percent-decoding). Use fileURLToPath(new URL(...)) (or fileURLToPath(import.meta.url) / import.meta.dirname) when you need a path for fs.readFileSync. Why pathname is unsafe - URL.prototype.pathname is URL-path text, not a platform-native (or even platform-correct) path string. On Windows it can yield an extra leading slash; e.g. new URL('.', import.meta.url).pathname becomes /C:/... but the correct path should be C:\...—Node explicitly recommends converting via fileURLToPath first. [1] - Node’s own fileURLToPath documentation calls out that pathname gives incorrect results for Windows file URLs like file:///C:/path/ (incorrect: /C:/path/; correct: C:\path\). [2] - Node’s fileURLToPath documentation also shows that pathname does not correctly percent-decode (e.g. /hello%20world vs /hello world). [2] Windows/path-encoding behavior (what goes wrong) - For Windows drive-letter file URLs, the pathname string retains URL-style leading slash and separators, whereas fileURLToPath returns a platform-valid Windows path. Example from Node docs: new URL('file:///C:/path/').pathname // Incorrect: /C:/path/ vs fileURLToPath('file:///C:/path/') // Correct: C:\path\. [2] - fileURLToPath also performs correct decoding of percent-encoded characters in the file URL (e.g., hello%20world → hello world). [2] UNC note - Node issue discussions around Windows also mention that fileURLToPath is the correct conversion for Windows edge cases; specifically, converting Windows/UNC-related file URL paths requires proper handling that pathname alone won’t provide. [1] Safe pattern for fs.readFileSync - Convert first: - import { fileURLToPath } from 'node:url'; - const p = fileURLToPath(new URL('./relative/file.txt', import.meta.url)); - const data = fs.readFileSync(p, 'utf8'); This aligns with Node’s guidance to convert the URL to a path before using it with filesystem APIs. [1][2] (You can also pass a URL object to fs methods, but only for file: URLs; the general guidance for this question remains: don’t use .pathname directly.) Node issues note fs.readFile accepts URL instances (from new URL) rather than URL strings, but .pathname is still not the portable way to obtain a path. [3][4]
Citations:
- 1: new URL('.', import.meta.url).pathname produces erroneous path on Windows 10 nodejs/node#37845
- 2: https://nodejs.org/api/url.html
- 3: Node.js APIs are inconvenient to use with URL strings nodejs/node#48994
- 4:
fs.fileReadSyncdoesn't understandfile://URL strings nodejs/node#20357
🏁 Script executed:
# Check if the file exists and verify the code context
if [ -f "circuits/setup.mjs" ]; then
echo "=== File exists, showing lines 35-42 ==="
sed -n '35,42p' circuits/setup.mjs
else
echo "File circuits/setup.mjs not found"
fi
# Check for similar URL/pathname patterns in the file
echo -e "\n=== Checking for other URL/pathname patterns ==="
rg "\.pathname" circuits/setup.mjs || echo "No other .pathname usages found"Repository: trade/mark
Length of output: 743
Use fileURLToPath for template file resolution.
URL.pathname produces invalid filesystem paths on Windows (e.g., /C:/... instead of C:\...) and does not properly decode percent-encoded characters. Convert the file URL using fileURLToPath() before readFileSync.
Suggested fix
+import { fileURLToPath } from 'url';
@@
-const templatePath = new URL('node_modules/snarkjs/templates/verifier_groth16.sol.ejs', import.meta.url).pathname;
+const templatePath = fileURLToPath(
+ new URL('node_modules/snarkjs/templates/verifier_groth16.sol.ejs', import.meta.url)
+);
const solidityTemplate = readFileSync(templatePath, 'utf8');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const templatePath = new URL('node_modules/snarkjs/templates/verifier_groth16.sol.ejs', import.meta.url).pathname; | |
| const solidityTemplate = readFileSync(templatePath, 'utf8'); | |
| import { fileURLToPath } from 'url'; | |
| import { randomBytes } from 'crypto'; | |
| import { mkdirSync, writeFileSync } from 'fs'; | |
| import { zKey, powersOfTau } from 'snarkjs'; | |
| mkdirSync('build', { recursive: true }); | |
| console.log('Step 1: Powers of Tau (pot15)...'); | |
| await powersOfTau.newAccumulator('bn128', 15, 'build/pot15_0000.ptau'); | |
| console.log('Step 2: Powers of Tau Contribution...'); | |
| await powersOfTau.contribute('build/pot15_0000.ptau', 'build/pot15_final.ptau', | |
| 'MARK Protocol', `markpool-entropy-${randomBytes(32).toString('hex')}`); | |
| console.log('Step 3: zKey (Groth16)...'); | |
| await zKey.newZKey('circuits/markpool.r1cs', 'build/pot15_final.ptau', 'build/markpool_0000.zkey'); | |
| console.log('Step 4: zKey Contribution...'); | |
| await zKey.contribute('build/markpool_0000.zkey', 'build/markpool_final.zkey', | |
| 'MARK Protocol MARKPool', `markpool-zkey-entropy-${randomBytes(32).toString('hex')}`); | |
| console.log('Step 5: Export Solidity Verifier...'); | |
| const templatePath = fileURLToPath( | |
| new URL('node_modules/snarkjs/templates/verifier_groth16.sol.ejs', import.meta.url) | |
| ); | |
| const solidityTemplate = readFileSync(templatePath, 'utf8'); |
🤖 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 `@circuits/setup.mjs` around lines 38 - 39, The code uses URL.pathname which
can produce invalid Windows paths and won't decode percent-encoding; instead
convert the file URL to a proper file system path using fileURLToPath before
reading. Replace usage of new URL(...).pathname when computing templatePath with
fileURLToPath(new URL('node_modules/snarkjs/templates/verifier_groth16.sol.ejs',
import.meta.url)) and then pass that result to readFileSync to set
solidityTemplate so readFileSync receives a correct platform-safe path.
| bytes32 commitmentHash, | ||
| if (srcChainId == 0) revert InvalidSource(); | ||
| if (srcChainId == block.chainid) revert SourceIsDestination(); | ||
| if (messageId == bytes32(0)) revert InvalidRoot(); |
There was a problem hiding this comment.
Incorrect error used for messageId validation.
Line 389 validates that messageId != bytes32(0) but reverts with InvalidRoot(). This is misleading since InvalidRoot semantically relates to Merkle roots, not message identifiers. Consider adding a dedicated error like InvalidMessageId() for clarity.
Suggested fix
In PoolErrors.sol, add:
error InvalidMessageId();Then update line 389:
-if (messageId == bytes32(0)) revert InvalidRoot();
+if (messageId == bytes32(0)) revert InvalidMessageId();🤖 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/src/pool/MARKPool.sol` at line 389, The check ensuring messageId !=
bytes32(0) currently reverts with InvalidRoot(), which is semantically wrong;
add a new error InvalidMessageId() to PoolErrors (or the contract's errors list)
and replace the revert on messageId validation in MARKPool (the line using
messageId == bytes32(0)) to revert InvalidMessageId() so the error accurately
reflects the failing condition.
RYLACreditLedger:
- Separate credit (pool-only) and debit (adapter-only) callers
- Add setAdapter() one-time setter to break circular deploy dependency
(adapter constructor needs ledger, ledger needs adapter address)
- Add AdapterAlreadySet error
DeployMARKPool.s.sol:
- Call ledger.setAdapter(adapter) after adapter deployment
Tests:
- RYLACreditLedger.t.sol: updated for new caller model, 11 tests
- MARKWithdrawAdapter.t.sol: add setAdapter call in setUp
- MARKPoolE2E.t.sol: full withdrawal flow E2E test (3 tests)
- testFullWithdrawalFlow: mint RYLA -> transactWithWithdrawBinding
-> withdrawWithSig -> verify RYLA burned, ETH received
- testNullifierReplayRejected
- testBindingMismatchRejected
134/134 tests pass
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/RYLACreditLedger.sol`:
- Around line 74-76: The view totalCreditsOutstanding() can revert if
_totalBurned > _totalMinted; change it to guard against underflow by checking
the two storage counters and returning 0 when _totalBurned >= _totalMinted,
otherwise return the difference (_totalMinted - _totalBurned). Update the
implementation in the totalCreditsOutstanding function that references
_totalMinted and _totalBurned so the view never underflows and always returns a
safe uint256.
- Around line 42-47: The setAdapter function currently allows anyone to call and
set ADAPTER once, enabling frontrunners to hijack debit authorization; restrict
or remove public access by either (a) moving ADAPTER initialization into the
constructor and removing setAdapter entirely, or (b) adding an access control
check to setAdapter (e.g., require(msg.sender == owner || onlyOwner modifier)
before the existing checks) so only the contract owner/admin can call
setAdapter, keep existing ZeroAddress() and AdapterAlreadySet() guards, and
still emit AdapterSet(adapter_) upon authorized update.
In `@contracts/test/e2e/pool/MARKPoolE2E.t.sol`:
- Around line 194-217: The second withdrawWithSig call currently reuses
signatures made for the original nonce causing a stale-signature revert rather
than exercising nullifier replay protection; recompute the intent hash and
digest using nonce + 1 (call adapter.computeWithdrawIntentHash and
MessageHashUtils.toEthSignedMessageHash with the updated nonce), re-sign with
vm.sign (producing new v/r/s for ownerPk and intentSignerPk) and pass those new
signatures to adapter.withdrawWithSig, and update vm.expectRevert to assert the
specific nullifier-replay error selector returned by the contract instead of a
generic revert.
🪄 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: 791f9f8b-ce69-402b-bca4-32fa104c77bd
📒 Files selected for processing (5)
contracts/script/deploy/pool/DeployMARKPool.s.solcontracts/src/pool/RYLACreditLedger.solcontracts/test/e2e/pool/MARKPoolE2E.t.solcontracts/test/unit/pool/RYLACreditLedger.t.solcontracts/test/unit/withdraw/MARKWithdrawAdapter.t.sol
- ReleasePool.s.sol: release orchestrator for pool stack following the same pattern as ReleaseMARK.s.sol — preflight checks, deploy via DeployMARKPool, post-deploy verification (wiring checks + RYLA roles), JSON artifact write - .env.example: add pool stack env vars (MARK_POOL_VERIFIER, MARK_POOL_OWNER, MARK_POOL_INTENT_SIGNER, release flags, artifact path, post-deploy verify addresses)
RYLACreditLedger: - Add OWNER immutable (set to msg.sender in constructor) - Restrict setAdapter to OWNER to prevent front-running between deployment and the setAdapter call in the release script - Add testSetAdapterRevertsForNonOwner test - Add clarifying NatSpec to totalCreditsOutstanding explaining it tracks only flows through this ledger, not total RYLA supply MARKWithdrawAdapter: - Move ETH transfer before ASSET_LEDGER.debit — if ETH transfer fails, RYLA is no longer burned (was a loss-of-funds bug) MARKPool: - Remove dead _seedRoot function (defined but never called) - Add NatSpec to computePublicInputsWithWithdraw clarifying chainId vs dstChainId semantics
testNullifierReplayRejected was reusing signatures computed for nonce N in the second withdrawWithSig call with nonce N+1, causing a NonceMismatch revert instead of exercising nullifier replay protection. Now recomputes the intent hash and signs with the updated nonce so the revert is caused by NullifierAlreadyClaimed as intended.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (3)
contracts/src/pool/MARKPool.sol (2)
389-389:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a semantically correct error for
messageIdvalidation.Line 389 validates
messageId != bytes32(0)but reverts withInvalidRoot(), which semantically relates to Merkle roots, not bridge message identifiers.Suggested fix
Add to
PoolErrors.sol:error InvalidMessageId();Then update line 389:
-if (messageId == bytes32(0)) revert InvalidRoot(); +if (messageId == bytes32(0)) revert InvalidMessageId();🤖 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/src/pool/MARKPool.sol` at line 389, The revert in MARKPool.sol checks messageId == bytes32(0) but uses the semantically wrong InvalidRoot() error; add a new error InvalidMessageId() to PoolErrors.sol and change the revert in MARKPool (the check around messageId) to revert InvalidMessageId() instead of InvalidRoot() so the error reflects a bad bridge message identifier.
221-229: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider renaming
FixedFeePolicyerror for clarity.The comment at lines 222-223 explains that
minFeeis "intentionally constrained to 0/1 credit unit," but the error nameFixedFeePolicydoesn't clearly convey this constraint. A more descriptive name likeMinFeeTooLargewould improve readability.Optional refactor
-if (newMinFee > 1) revert FixedFeePolicy(); +if (newMinFee > 1) revert MinFeeTooLarge();And add the error to
PoolErrors.sol:error MinFeeTooLarge();🤖 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/src/pool/MARKPool.sol` around lines 221 - 229, The FixedFeePolicy error name in setMinFee is ambiguous; rename it to a clearer identifier (e.g., MinFeeTooLarge) and update all references: declare error MinFeeTooLarge() in PoolErrors.sol, replace the revert FixedFeePolicy() in function setMinFee with revert MinFeeTooLarge(), and update any other files/tests that import or reference FixedFeePolicy so compilation and error behavior remain consistent.contracts/src/pool/RYLACreditLedger.sol (1)
77-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
totalCreditsOutstanding()can still underflow despite the NatSpec claim.The comment at lines 80-81 states "Cannot underflow" because debit burns tokens "previously credited," but this reasoning is incorrect. Line 60 calls
safeTransferFrom(from, ...)then burns — the tokens being burned may have been minted through other paths (e.g.,MARKSettlementModule, as noted in line 79), not via this ledger'scredit(). Therefore_totalBurnedcan legitimately exceed_totalMinted, causing the subtraction at line 83 to revert and breaking integrations that query this view.🧮 Safer implementation
function totalCreditsOutstanding() external view returns (uint256) { - return _totalMinted - _totalBurned; + return _totalMinted > _totalBurned ? _totalMinted - _totalBurned : 0; }And update the NatSpec to clarify the behavior:
/// `@notice` Returns net credits flowing through this ledger (minted minus burned). /// `@dev` Tracks only flows via credit() and debit() on this contract. RYLA minted /// through other paths (e.g. MARKSettlementModule) is not reflected here. -/// Cannot underflow: debit() burns tokens that were previously credited or -/// held by the user, so _totalBurned never exceeds _totalMinted in normal operation. +/// Returns 0 if _totalBurned exceeds _totalMinted (can occur when debit burns +/// tokens minted outside this ledger).🤖 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/src/pool/RYLACreditLedger.sol` around lines 77 - 84, totalCreditsOutstanding() currently assumes _totalMinted >= _totalBurned and will underflow if tokens burned via safeTransferFrom/debit() originated outside this ledger (e.g., MARKSettlementModule); change the getter to guard against underflow by returning the capped net (if _totalMinted >= _totalBurned return the difference, otherwise return 0) and update the NatSpec on totalCreditsOutstanding() to remove "Cannot underflow" and explicitly state that the result is minted minus burned capped at zero, referencing that credit(), debit(), safeTransferFrom() and external mint paths (MARKSettlementModule) can affect counts.
🤖 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.
Duplicate comments:
In `@contracts/src/pool/MARKPool.sol`:
- Line 389: The revert in MARKPool.sol checks messageId == bytes32(0) but uses
the semantically wrong InvalidRoot() error; add a new error InvalidMessageId()
to PoolErrors.sol and change the revert in MARKPool (the check around messageId)
to revert InvalidMessageId() instead of InvalidRoot() so the error reflects a
bad bridge message identifier.
- Around line 221-229: The FixedFeePolicy error name in setMinFee is ambiguous;
rename it to a clearer identifier (e.g., MinFeeTooLarge) and update all
references: declare error MinFeeTooLarge() in PoolErrors.sol, replace the revert
FixedFeePolicy() in function setMinFee with revert MinFeeTooLarge(), and update
any other files/tests that import or reference FixedFeePolicy so compilation and
error behavior remain consistent.
In `@contracts/src/pool/RYLACreditLedger.sol`:
- Around line 77-84: totalCreditsOutstanding() currently assumes _totalMinted >=
_totalBurned and will underflow if tokens burned via safeTransferFrom/debit()
originated outside this ledger (e.g., MARKSettlementModule); change the getter
to guard against underflow by returning the capped net (if _totalMinted >=
_totalBurned return the difference, otherwise return 0) and update the NatSpec
on totalCreditsOutstanding() to remove "Cannot underflow" and explicitly state
that the result is minted minus burned capped at zero, referencing that
credit(), debit(), safeTransferFrom() and external mint paths
(MARKSettlementModule) can affect counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b1ef7761-8b1c-4176-93ca-45e3da6cea05
📒 Files selected for processing (7)
contracts/.env.examplecontracts/script/ops/pool/ReleasePool.s.solcontracts/src/pool/MARKPool.solcontracts/src/pool/RYLACreditLedger.solcontracts/src/withdraw/MARKWithdrawAdapter.solcontracts/test/e2e/pool/MARKPoolE2E.t.solcontracts/test/unit/pool/RYLACreditLedger.t.sol
contracts-ci.yml: - Add pool release dry-run and execute smoke steps to the contracts-release-check job, reusing the Anvil instance and RYLA token deployed by the settlement release step - Assert pool release artifact schema (pool, ledger, adapter addresses) MARKPoolDeployScripts.t.sol: - testDeployMARKPoolWiresAllContracts: verifies all contract wiring (pool<->ledger, ledger<->adapter, RYLA roles) - testDeployMARKPoolSetsIntentSignerWhenProvided: verifies intent signer is configured when MARK_POOL_INTENT_SIGNER is set - testDeployMARKPoolRevertsWhenMissingTokenAdmin: verifies preflight check rejects deployer without RYLA admin role 138/138 tests pass
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 @.github/workflows/contracts-ci.yml:
- Around line 123-129: The CI step "Run pool release orchestrator dry-run"
currently swallows failures via the trailing "|| true" on the forge invocation
for script/ops/pool/ReleasePool.s.sol; remove that "|| true" so the job fails on
real regressions, and if the dry-run is expected to fail in some cases instead
add a separate negative test step that explicitly asserts the expected failure
(e.g., run the same forge script and check for the specific error/exit code or
output) rather than masking all errors.
In `@contracts/src/pool/RYLACreditLedger.sol`:
- Around line 77-81: Update the NatSpec on the ledger summary to remove the
incorrect invariant claiming "_totalBurned never exceeds _totalMinted" and
instead accurately describe the accounting scope: state that
totalCreditsOutstanding() reports net credits tracked via this contract's
credit() and debit() flows (variables _totalMinted and _totalBurned), but
external mints/burns (e.g., MARKSettlementModule or direct burns of user-held
RYLA) may cause _totalBurned to exceed _totalMinted as measured here; reference
the credit(), debit(), _totalMinted, and _totalBurned symbols so readers
understand the limited scope of this metric.
🪄 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: 85d49fd2-d679-47c8-9a8f-a322df5fd02e
📒 Files selected for processing (3)
.github/workflows/contracts-ci.ymlcontracts/src/pool/RYLACreditLedger.solcontracts/test/unit/MARKPoolDeployScripts.t.sol
- contracts-ci.yml: remove '|| true' from pool release dry-run step; use the deployed settlement module address as verifier (a real contract) so the preflight code.length check passes without masking failures - RYLACreditLedger.sol: fix NatSpec on totalCreditsOutstanding to accurately describe accounting scope — _totalBurned can exceed _totalMinted if RYLA is burned via other paths (e.g. settlement module)
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/contracts-ci.yml:
- Around line 145-149: The jq checks currently compare .pool, .ledger, and
.adapter to the zero address string which returns true for null values, so
update the condition to explicitly reject null and validate proper hex address
format; replace the existing predicates with checks like (.pool != null and
(.pool | test("^0x[0-9a-fA-F]{40}$"))) and do the same for .ledger and .adapter,
while also keeping (.chainId != null) and (.timestamp != null) intact, so the
workflow's jq filter only accepts non-null, 0x-prefixed 40-hex-char addresses.
🪄 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: 31435c79-b511-4a0c-a969-de00cee8fd34
📒 Files selected for processing (2)
.github/workflows/contracts-ci.ymlcontracts/src/pool/RYLACreditLedger.sol
contracts-ci.yml: - Add --skip-simulation to pool release broadcast — PoseidonT3 (55,856 bytes) exceeds EIP-170 limit and cannot be deployed without refactoring to a linked library; --skip-simulation tests script orchestration only - Fix jq assertion to use regex validation instead of zero-address check, rejecting null values and validating hex address format KNOWN_ISSUES.md: - Add KI-8 documenting PoseidonT3 contract size issue and required fix before mainnet (deploy as standalone contract, call via interface)
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/contracts-ci.yml (1)
147-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject zero-address values in pool artifact assertions.
Line 147–149 validate shape but still allow
0x0000000000000000000000000000000000000000, so a failed deploy can pass this gate.Suggested jq tightening
jq -e ' .chainId != null and .timestamp != null and - (.pool != null and (.pool | test("^0x[0-9a-fA-F]{40}$"))) and - (.ledger != null and (.ledger | test("^0x[0-9a-fA-F]{40}$"))) and - (.adapter != null and (.adapter | test("^0x[0-9a-fA-F]{40}$"))) + (.pool != null and .pool != "0x0000000000000000000000000000000000000000" and (.pool | test("^0x[0-9a-fA-F]{40}$"))) and + (.ledger != null and .ledger != "0x0000000000000000000000000000000000000000" and (.ledger | test("^0x[0-9a-fA-F]{40}$"))) and + (.adapter != null and .adapter != "0x0000000000000000000000000000000000000000" and (.adapter | test("^0x[0-9a-fA-F]{40}$"))) ' broadcast/mark-pool-release-ci.json🤖 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 @.github/workflows/contracts-ci.yml around lines 147 - 149, The jq assertions currently allow the zero address; update the three checks for .pool, .ledger, and .adapter to also reject "0x0000000000000000000000000000000000000000". Keep the existing regex test("^0x[0-9a-fA-F]{40}$") and append an explicit inequality for each symbol (e.g. (.pool != null and (.pool | test("^0x[0-9a-fA-F]{40}$")) and .pool != "0x0000000000000000000000000000000000000000")) so that .pool, .ledger and .adapter cannot be the zero-address.
🤖 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/KNOWN_ISSUES.md`:
- Line 77: Update KI-7 to remove the implication of distinct circuit designs and
state that both systems use the same MARKPool circuit; specifically, change the
title and wording to reflect a single shared circuit (reference KI-7, MARKPool,
"13-signal layout", and "shared pool verifier contract") so the doc consistently
explains that both the pool and settlement systems use the identical circuit and
verifier rather than different designs.
In `@contracts/src/pool/RYLACreditLedger.sol`:
- Around line 34-49: The constructor and setAdapter currently only check for
non-zero addresses, allowing EOAs to be set as TOKEN, POOL, or ADAPTER; add
checks that the addresses are contracts (code length > 0) before storing them.
In the constructor (constructor) after the non-zero checks validate token_ and
pool_ with address(token_).code.length > 0 (or assembly extcodesize) and revert
(add or reuse an error like ZeroAddress/Unauthorized or introduce a new
InvalidContract error) if either has no code; then cast/store TOKEN and POOL as
before. In setAdapter (setAdapter) after the non-zero and ADAPTER unset checks
validate address(adapter_).code.length > 0 and revert if it’s an EOA, then
assign ADAPTER and emit AdapterSet. Ensure TOKEN is cast to IRYLA only after the
contract check.
---
Duplicate comments:
In @.github/workflows/contracts-ci.yml:
- Around line 147-149: The jq assertions currently allow the zero address;
update the three checks for .pool, .ledger, and .adapter to also reject
"0x0000000000000000000000000000000000000000". Keep the existing regex
test("^0x[0-9a-fA-F]{40}$") and append an explicit inequality for each symbol
(e.g. (.pool != null and (.pool | test("^0x[0-9a-fA-F]{40}$")) and .pool !=
"0x0000000000000000000000000000000000000000")) so that .pool, .ledger and
.adapter cannot be the zero-address.
🪄 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: b71dd4a8-c05b-4c56-8cac-a8ca2a5092fb
📒 Files selected for processing (3)
.github/workflows/contracts-ci.ymlcontracts/KNOWN_ISSUES.mdcontracts/src/pool/RYLACreditLedger.sol
contracts-ci.yml: - Remove pool release execute smoke step — MARKPool (24,841 bytes) and PoseidonT3 (55,856 bytes) exceed EIP-170 limit and cannot be broadcast to Anvil; pool deploy requires PoseidonT3 refactor (KI-8) first - Keep pool release dry-run only (validates script logic and preflight) - Remove the now-unused artifact assertion step KNOWN_ISSUES.md: - Fix KI-7: both pool and settlement systems use the same MARKPool circuit — remove implication of distinct circuit designs
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
contracts/KNOWN_ISSUES.md (1)
77-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKI-7 heading contradicts the body.
Line 77 says “different circuit designs,” but Lines 81-84 state both domains share the same MARKPool 13-signal circuit. Please align the title with the actual architecture.
Proposed fix
-## KI-7: Two separate ZK systems with different circuit designs +## KI-7: Pool and settlement domains share the same ZK circuit design🤖 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/KNOWN_ISSUES.md` at line 77, The KI-7 heading is inconsistent with its body: update the heading "KI-7: Two separate ZK systems with different circuit designs" to reflect that both domains share the same MARKPool 13-signal circuit (as described in the body); change the title to something like "KI-7: Two separate ZK systems sharing the MARKPool 13-signal circuit" or equivalent wording so the heading and the body (which references the MARKPool 13-signal circuit) are aligned..github/workflows/contracts-ci.yml (1)
125-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate extracted addresses before invoking
forge script.Lines 125-129 trust
jq -routput directly; missing keys become"null"and can turn this into a confusing downstream failure. Fail early by requiring valid 0x-addresses at extraction.Proposed fix
- TOKEN=$(jq -r '.token' broadcast/mark-release-ci.json) - MODULE=$(jq -r '.module' broadcast/mark-release-ci.json) + TOKEN=$(jq -er '.token | select(type=="string" and test("^0x[0-9a-fA-F]{40}$"))' broadcast/mark-release-ci.json) + MODULE=$(jq -er '.module | select(type=="string" and test("^0x[0-9a-fA-F]{40}$"))' broadcast/mark-release-ci.json) MARK_RYLA_TOKEN="$TOKEN" \ MARK_POOL_VERIFIER="$MODULE" \ forge script script/ops/pool/ReleasePool.s.sol --rpc-url $RPC_URL -vv🤖 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 @.github/workflows/contracts-ci.yml around lines 125 - 129, The script currently assigns TOKEN and MODULE from jq and passes them as MARK_RYLA_TOKEN and MARK_POOL_VERIFIER into the forge invocation without validation; add a validation step after extracting TOKEN and MODULE to check they are non-empty and match an Ethereum address pattern (starts with 0x followed by 40 hex chars), and if either check fails print a clear error including which variable is invalid and exit with non-zero status before running forge script (keep the variable names TOKEN, MODULE, MARK_RYLA_TOKEN, MARK_POOL_VERIFIER and the forge script invocation unchanged).
🤖 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/KNOWN_ISSUES.md`:
- Around line 98-99: Update the KI-8 entry in KNOWN_ISSUES.md to remove the
stale claim that CI uses `--skip-simulation` for a pool smoke step and replace
it with the current behavior: the workflow no longer runs a pool execute smoke
and instead performs only a pool dry-run (dry-run/simulation only). Ensure the
Impact/description for `MARKPool` and the CI behavior note mention the omission
of pool execute smoke and the use of dry-run, and remove any reference to
`--skip-simulation`.
---
Duplicate comments:
In @.github/workflows/contracts-ci.yml:
- Around line 125-129: The script currently assigns TOKEN and MODULE from jq and
passes them as MARK_RYLA_TOKEN and MARK_POOL_VERIFIER into the forge invocation
without validation; add a validation step after extracting TOKEN and MODULE to
check they are non-empty and match an Ethereum address pattern (starts with 0x
followed by 40 hex chars), and if either check fails print a clear error
including which variable is invalid and exit with non-zero status before running
forge script (keep the variable names TOKEN, MODULE, MARK_RYLA_TOKEN,
MARK_POOL_VERIFIER and the forge script invocation unchanged).
In `@contracts/KNOWN_ISSUES.md`:
- Line 77: The KI-7 heading is inconsistent with its body: update the heading
"KI-7: Two separate ZK systems with different circuit designs" to reflect that
both domains share the same MARKPool 13-signal circuit (as described in the
body); change the title to something like "KI-7: Two separate ZK systems sharing
the MARKPool 13-signal circuit" or equivalent wording so the heading and the
body (which references the MARKPool 13-signal circuit) are aligned.
🪄 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: 8205e122-a497-4cd4-a836-7128f3563a6a
📒 Files selected for processing (2)
.github/workflows/contracts-ci.ymlcontracts/KNOWN_ISSUES.md
Comment on lines
+98
to
+99
| **Impact:** `MARKPool` cannot be deployed as-is. The CI pool release smoke test uses `--skip-simulation` to bypass the size check and test script orchestration only. | ||
|
|
There was a problem hiding this comment.
KI-8 CI behavior note is stale.
Line 98 says CI uses --skip-simulation for a pool smoke step, but current workflow omits pool execute smoke and runs only a pool dry-run. This should be updated to avoid audit confusion.
Proposed fix
-**Impact:** `MARKPool` cannot be deployed as-is. The CI pool release smoke test uses `--skip-simulation` to bypass the size check and test script orchestration only.
+**Impact:** `MARKPool` cannot be deployed as-is. CI currently omits pool execute smoke and runs only the pool release orchestrator dry-run to validate script flow until the Poseidon refactor lands.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Impact:** `MARKPool` cannot be deployed as-is. The CI pool release smoke test uses `--skip-simulation` to bypass the size check and test script orchestration only. | |
| **Impact:** `MARKPool` cannot be deployed as-is. CI currently omits pool execute smoke and runs only the pool release orchestrator dry-run to validate script flow until the Poseidon refactor lands. |
🤖 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/KNOWN_ISSUES.md` around lines 98 - 99, Update the KI-8 entry in
KNOWN_ISSUES.md to remove the stale claim that CI uses `--skip-simulation` for a
pool smoke step and replace it with the current behavior: the workflow no longer
runs a pool execute smoke and instead performs only a pool dry-run
(dry-run/simulation only). Ensure the Impact/description for `MARKPool` and the
CI behavior note mention the omission of pool execute smoke and the use of
dry-run, and remove any reference to `--skip-simulation`.
… setAdapter Prevents EOAs from being set as TOKEN, POOL, or ADAPTER. Adds InvalidContract error. 3 new tests cover the EOA rejection cases. setUp uses vm.etch to give mock addresses contract bytecode.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Summary
Introduces the full pool domain for private RYLA transfers using ZK UTXO proofs with Merkle tree membership.
Changes
Contracts
Circuit
Fixes and cleanup
CI
Scope
contracts, circuits, workflows
Verification
Risk
Medium. New pool domain is additive — no changes to existing settlement or bridge contracts. RYLACreditLedger access control fix (onlyPool) is a security improvement. Domain separator rename is permanent and must land before any pool deployment.
Summary by CodeRabbit
New Features
Documentation
Tests / CI
Chores