[Contract] Implement donor eligibility verification via Merkle commitment + nullifier scheme - #601
Open
SMOGZ6783 wants to merge 1 commit into
Open
Conversation
|
@SMOGZ6783 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Contributor
📝 WalkthroughWalkthroughAdds a new Soroban donor-verification crate and contract using fixed-depth Merkle proofs, admin-managed roots, donor authorization, nullifier replay protection, persistent TTLs, verification events, unit tests, and contract documentation. ChangesAnonymous donor verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Donor
participant DonorVerificationContract
participant PersistentStorage
participant EventSink
Donor->>DonorVerificationContract: verify_and_donate(leaf, proof, leaf_index, nullifier)
DonorVerificationContract->>DonorVerificationContract: Require authorization and compute Merkle root
DonorVerificationContract->>PersistentStorage: Read root and nullifier marker
DonorVerificationContract->>PersistentStorage: Store used nullifier with TTL
DonorVerificationContract->>EventSink: Publish verified event
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/donor-verification/src/test.rs (1)
167-183: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest missing authorization separately.
This only exercises the
admin != stored_adminbranch; it cannot catch removal ofadmin.require_auth(). Add cases using the stored admin without authorization, and a validverify_and_donatecall without donor authorization.🤖 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/donor-verification/src/test.rs` around lines 167 - 183, Extend test_update_root_requires_admin to disable mocked authorizations and call update_root with the stored admin, asserting the authorization failure separately from the Error::NotAdmin case. Add a test case for verify_and_donate using a donor without authorization, asserting it also fails authorization while preserving the existing valid-authority behavior.
🤖 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/donor-verification/src/lib.rs`:
- Around line 195-207: Validate leaf_index is less than 1u32 shifted by
TREE_DEPTH before entering proof verification, rejecting indices outside the
configured tree rather than truncating higher bits. Add a boundary test covering
both the maximum valid index and the first invalid index at 1u32 << TREE_DEPTH,
while preserving existing verification behavior for valid indices.
- Around line 120-154: The verify_and_donate flow in
contracts/donor-verification/src/lib.rs, specifically verify_and_donate, must
replace transparent Merkle inclusion with verification of a ZK statement binding
the secret, enrolled leaf, Merkle path, and nullifier hash; use relayed
submission without requiring a donor address when anonymity is required. Update
docs/contracts/donor-verification.md bash lines 3-19 to remove the unlinkability
claim and describe the current verifier as providing only public Merkle
membership until the ZK verifier is implemented.
---
Nitpick comments:
In `@contracts/donor-verification/src/test.rs`:
- Around line 167-183: Extend test_update_root_requires_admin to disable mocked
authorizations and call update_root with the stored admin, asserting the
authorization failure separately from the Error::NotAdmin case. Add a test case
for verify_and_donate using a donor without authorization, asserting it also
fails authorization while preserving the existing valid-authority behavior.
🪄 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: e3198617-6fa7-44ab-93d7-c06f6c62e637
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
contracts/donor-verification/Cargo.tomlcontracts/donor-verification/src/lib.rscontracts/donor-verification/src/test.rsdocs/contracts/donor-verification.md bash
Comment on lines
+120
to
+154
| pub fn verify_and_donate( | ||
| env: Env, | ||
| donor: Address, | ||
| leaf: BytesN<32>, | ||
| proof: Vec<BytesN<32>>, | ||
| leaf_index: u32, | ||
| nullifier: BytesN<32>, | ||
| ) -> Result<(), Error> { | ||
| donor.require_auth(); | ||
|
|
||
| if proof.len() != TREE_DEPTH { | ||
| return Err(Error::InvalidProofLength); | ||
| } | ||
|
|
||
| let nullifier_key = DataKey::Nullifier(nullifier.clone()); | ||
| if env.storage().persistent().has(&nullifier_key) { | ||
| return Err(Error::NullifierAlreadyUsed); | ||
| } | ||
|
|
||
| let root: BytesN<32> = Self::get_root(&env)?; | ||
| let computed_root = Self::compute_merkle_root(&env, &leaf, &proof, leaf_index); | ||
|
|
||
| if computed_root != root { | ||
| return Err(Error::InvalidMerkleProof); | ||
| } | ||
|
|
||
| // Record the nullifier as spent. Only the nullifier is stored — | ||
| // never the donor's address or the leaf/commitment itself. | ||
| env.storage().persistent().set(&nullifier_key, &true); | ||
| env.storage() | ||
| .persistent() | ||
| .extend_ttl(&nullifier_key, STORAGE_TTL_THRESHOLD, STORAGE_TTL_EXTEND_TO); | ||
|
|
||
| env.events() | ||
| .publish((symbol_short!("verified"),), nullifier); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Replace the transparent proof flow; it does not provide anonymous, one-time eligibility.
Merkle inclusion only proves that the supplied leaf is in root; it neither proves knowledge of a secret nor binds nullifier to that leaf. An enrolled donor can reuse one valid leaf/proof with unlimited fresh nullifiers. Further, donor, leaf, and proof are public arguments of the same invocation, so they are linkable on-chain.
contracts/donor-verification/src/lib.rs#L120-L154: verify a ZK statement binding the secret, enrolled leaf, Merkle path, and nullifier hash; use relayed submission rather than a donor address when anonymity is required.docs/contracts/donor-verification.md bash#L3-L19: remove the unlinkability claim and describe this as public Merkle membership until that verifier exists.
📍 Affects 2 files
contracts/donor-verification/src/lib.rs#L120-L154(this comment)docs/contracts/donor-verification.md bash#L3-L19
🤖 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/donor-verification/src/lib.rs` around lines 120 - 154, The
verify_and_donate flow in contracts/donor-verification/src/lib.rs, specifically
verify_and_donate, must replace transparent Merkle inclusion with verification
of a ZK statement binding the secret, enrolled leaf, Merkle path, and nullifier
hash; use relayed submission without requiring a donor address when anonymity is
required. Update docs/contracts/donor-verification.md bash lines 3-19 to remove
the unlinkability claim and describe the current verifier as providing only
public Merkle membership until the ZK verifier is implemented.
Comment on lines
+195
to
+207
| let mut index = leaf_index; | ||
|
|
||
| for sibling in proof.iter() { | ||
| let mut buf = Bytes::new(env); | ||
| if index % 2 == 0 { | ||
| buf.append(&Bytes::from_array(env, ¤t.to_array())); | ||
| buf.append(&Bytes::from_array(env, &sibling.to_array())); | ||
| } else { | ||
| buf.append(&Bytes::from_array(env, &sibling.to_array())); | ||
| buf.append(&Bytes::from_array(env, ¤t.to_array())); | ||
| } | ||
| current = env.crypto().sha256(&buf).into(); | ||
| index /= 2; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject indices outside the configured tree.
Only the low 20 bits are consumed, so indices such as 0 and 1 << TREE_DEPTH compute the same path. Validate leaf_index < (1u32 << TREE_DEPTH) before verification and add a boundary 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/donor-verification/src/lib.rs` around lines 195 - 207, Validate
leaf_index is less than 1u32 shifted by TREE_DEPTH before entering proof
verification, rejecting indices outside the configured tree rather than
truncating higher bits. Add a boundary test covering both the maximum valid
index and the first invalid index at 1u32 << TREE_DEPTH, while preserving
existing verification behavior for valid indices.
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
Implements anonymous donor eligibility verification in a new Soroban contract at
contracts/donor-verification, so a donor's eligibility can be validated without disclosing their private account address on-chain.Closes #515
Approach
A full zk-SNARK circuit (Groth16/PLONK) requires pairing-curve arithmetic and a trusted setup outside the scope of a single contract PR. This implementation instead uses a Merkle-commitment + nullifier scheme — the same privacy primitive used by systems like Tornado Cash:
hash(secret, nullifier)), included as Merkle tree leaves; only the root is published on-chain.verify_and_donatechecks a submitted Merkle inclusion proof against the stored root and records the donor's nullifier, without ever storing or linking their account address to the commitment.This is flagged clearly in code comments as a scoped, testable first step. A true SNARK verifier could be swapped in behind the same entry point later if stronger unlinkability is required — happy to discuss scope with maintainers if that's the intended direction.
Changes
contracts/donor-verification/src/lib.rs: contract withinitialize,update_root,verify_and_donate,is_nullifier_used,get_current_rootcontracts/donor-verification/src/test.rs: unit tests covering success, invalid proof, reused nullifier, wrong proof length, unauthorized root update, and pre-initialization callscontracts/donor-verification/Cargo.tomlAcceptance criteria
cargo build --target wasm32-unknown-unknown --releasecargo testrequire_auth()checks oninitialize,update_root,verify_and_donateErrorenum with distinct error codesextend_ttlon instance and persistent storageSummary by CodeRabbit
New Features
Documentation
Tests