feat(event): add zkPassport on-chain verification with nullifier commitment - #134
Merged
Conversation
Replaces the single-organizer payout assumption with configurable revenue splits and co-host escrow handling (closes BuidlZone-Labs#122). - Event creation accepts revenue_splits: Vec<(Address, u32)> (basis points), validated to 1-5 recipients summing to 10000, no duplicates/zeros, with the primary organizer pinned at index 0. Splits are set once and immutable. - The split is synced to the payments contract, which settles each event once (deducting the platform fee first) and lets every recipient withdraw their allocated share independently via withdraw_split. Rounding dust accrues to the primary organizer so the full net is always distributed. - Primary organizer can flag a compromised co-host wallet; the flagged share is held in escrow and cannot be withdrawn. Admin resolves a dispute by either releasing the share to the recipient or reassigning it to the primary. - Legacy single-organizer withdrawal paths are rejected for split events to prevent double payout. Adds 17 payments unit tests and 3 event integration tests covering the happy path, fee ordering, independent withdrawals, the delay/dispute windows, split validation, immutability, flagging, and dispute resolution. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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:
📝 WalkthroughWalkthroughAdds zkPassport-gated attendance and revenue-split support to the contracts, with new types, storage, errors, events, entrypoints, and tests. It also reorders several event, payments, ticket, privacy-utils, and factory control flows and trims comment scaffolding in tests. ChangeszkPassport and Revenue Splits
Control-Flow, Tests, and Comment Cleanup
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
codeZe-us
changed the title
Resolve conflicts in error enums and payments storage keys by keeping both postponement and revenue-split additions with non-overlapping codes. Add revenue_splits to anon-claims test fixtures from upstream. Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve conflicts with postponement, anon-claims, and merged PR BuidlZone-Labs#132 (zkEmail commitments). Keep both revenue-split and commitment APIs with non-overlapping PaymentError codes (35–42). Co-authored-by: Cursor <cursoragent@cursor.com>
…itment and verify_and_attend entry point
codeZe-us
marked this pull request as ready for review
June 27, 2026 23:44
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
contracts/event/src/test_zk_passport.rs (1)
8-9: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the emitted zk attendance event does not leak proof material.
AC-6 is listed as covered, but the happy-path test only checks
sold_count. Please also assert that the emittedZkVerifiedAttendancepayload excludesclaim.nullifierandclaim.proof, so a future event-schema change cannot silently break anonymity guarantees.Also applies to: 103-132
🤖 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/event/src/test_zk_passport.rs` around lines 8 - 9, The happy-path zk attendance test currently only verifies `sold_count`, so it can miss regressions where the emitted `ZkVerifiedAttendance` event leaks proof material. Update the relevant test(s) around `ZkVerifiedAttendance` to explicitly assert the event payload does not include `claim.nullifier` or `claim.proof`, alongside the existing issuance checks, so future schema changes cannot reintroduce those fields.
🤖 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/event/src/events.rs`:
- Around line 226-229: Update the privacy note in the event docs to narrow the
claim about the nullifier: in the comments near the event definition in
events.rs, keep the point that the nullifier is omitted from the event payload,
but remove or rephrase any wording implying it is unobservable on-chain. Use the
existing event documentation block around the nullifier mention to explain only
that it is not duplicated in logs and that callers can read it from storage.
In `@contracts/event/src/lib.rs`:
- Around line 1279-1319: The verify_and_attend flow accepts claim.proof without
validating it, so arbitrary bytes can pass as long as the nullifier is fresh. In
verify_and_attend, add an actual zk proof verification step before any
attendance side effects, using the existing ZkPassportClaim and event/zk config
context, and return ZkProofInvalid on failure. Make sure this happens before
nullifier consumption, capacity checks, or payment/attendance mutations so only
a verified proof can proceed.
- Around line 1279-1284: The zk attendance flow in verify_and_attend and the
downstream pay_for_ticket/mint_ticket path must not process paid tiers without
an authenticated payer/recipient. Update the attendance API to require explicit
payer/recipient inputs or otherwise reject paid/minted registrations until a
privacy-preserving recipient flow exists, and make sure sold_count is only
incremented after a valid payment/mint succeeds. Also replace the current use of
env.current_contract_address() with nonce 0 in the ticket minting flow so the
attendee is uniquely identified and nonce collisions cannot occur.
In `@contracts/event/src/storage.rs`:
- Around line 41-43: The spent-nullifier storage is currently event-scoped, so
the same nullifier can be reused across different events. Update the nullifier
tracking in storage and replay checks to use a global tombstone/key instead of
being tied to event_id, and adjust has_zk_nullifier, save_zk_nullifier, and
their callers so the nullifier is checked and recorded globally.
- Around line 453-456: The nullifier persistence in save_zk_nullifier currently
uses a fixed TTL_BUMP that may expire before the proof replay window ends,
allowing the same nullifier to be accepted again. Update the logic around
save_zk_nullifier and the claim.expiry_ledger handling so the stored tombstone
lifetime is tied to the full intended replay window, either by capping
expiry_ledger against the maximum event window or by extending the persistent
storage TTL accordingly. Ensure the relevant symbols save_zk_nullifier,
claim.expiry_ledger, and extend_ttl are adjusted together so the nullifier
remains valid for the entire replay period.
In `@contracts/event/src/test_zk_passport.rs`:
- Around line 3-9: Add a cross-event replay test for the ZkPassport flow: the
current coverage around ZkPassportClaim and verify_and_attend only reuses a
proof within one event, so it misses the bug where nullifier tracking is still
scoped by event_id. Extend the test suite to create two distinct event_ids,
submit the same claim/proof/nullifier to the first event, then attempt the same
submission on the second event and assert it is rejected because the nullifier
is already used globally. Make sure the test exercises the on-chain nullifier
persistence path rather than only same-event replay behavior.
In `@contracts/event/src/types.rs`:
- Around line 50-58: The ZkVerificationConfig docs are inconsistent about the
sentinel for accepting any claim type: replace the misleading “None” wording
with `ZkClaimType::Any` in the struct-level comments, and update the matching
`set_zk_config` documentation to describe the same `Any` sentinel. Use the
existing `ZkVerificationConfig` and `set_zk_config` symbols to keep the
terminology aligned across the config API.
---
Nitpick comments:
In `@contracts/event/src/test_zk_passport.rs`:
- Around line 8-9: The happy-path zk attendance test currently only verifies
`sold_count`, so it can miss regressions where the emitted
`ZkVerifiedAttendance` event leaks proof material. Update the relevant test(s)
around `ZkVerifiedAttendance` to explicitly assert the event payload does not
include `claim.nullifier` or `claim.proof`, alongside the existing issuance
checks, so future schema changes cannot reintroduce those fields.
🪄 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: ecd64815-54b9-4557-be0c-f511c784d7b5
📒 Files selected for processing (7)
.gitignorecontracts/event/src/errors.rscontracts/event/src/events.rscontracts/event/src/lib.rscontracts/event/src/storage.rscontracts/event/src/test_zk_passport.rscontracts/event/src/types.rs
Comment on lines
+226
to
+229
| /// # Privacy design | ||
| /// - The **nullifier** is deliberately omitted from the event payload. Publishing | ||
| /// it on-chain would allow any observer to correlate proof submissions across | ||
| /// events. Callers who need the nullifier can read it from storage directly. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Narrow the nullifier privacy claim.
The event omits the nullifier, but the docs also say it is readable from storage. This avoids duplicating it in event logs; it does not make the nullifier unobservable on-chain.
Proposed docs fix
-/// - The **nullifier** is deliberately omitted from the event payload. Publishing
-/// it on-chain would allow any observer to correlate proof submissions across
-/// events. Callers who need the nullifier can read it from storage directly.
+/// - The **nullifier** is deliberately omitted from the event payload to avoid
+/// duplicating replay-prevention state in indexer-friendly logs. The spent
+/// nullifier remains contract storage state.📝 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
| /// # Privacy design | |
| /// - The **nullifier** is deliberately omitted from the event payload. Publishing | |
| /// it on-chain would allow any observer to correlate proof submissions across | |
| /// events. Callers who need the nullifier can read it from storage directly. | |
| /// # Privacy design | |
| /// - The **nullifier** is deliberately omitted from the event payload to avoid | |
| /// duplicating replay-prevention state in indexer-friendly logs. The spent | |
| /// nullifier remains contract storage state. |
🤖 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/event/src/events.rs` around lines 226 - 229, Update the privacy
note in the event docs to narrow the claim about the nullifier: in the comments
near the event definition in events.rs, keep the point that the nullifier is
omitted from the event payload, but remove or rephrase any wording implying it
is unobservable on-chain. Use the existing event documentation block around the
nullifier mention to explain only that it is not duplicated in logs and that
callers can read it from storage.
Comment on lines
+1279
to
+1284
| pub fn verify_and_attend( | ||
| env: Env, | ||
| event_id: Symbol, | ||
| tier_id: u32, | ||
| claim: ZkPassportClaim, | ||
| ) -> Result<(), EventError> { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Do not sell or mint zk tickets without an authenticated payer/recipient.
For paid tiers, unlinked contracts skip payment and still increment sold_count. When linked, pay_for_ticket and mint_ticket use env.current_contract_address() plus nonce 0, so the attendee neither pays nor owns the ticket, and paid registrations can collide on nonce. Add authenticated payer/recipient inputs or reject paid/minted zk attendance until a privacy-preserving recipient flow exists.
Also applies to: 1346-1370
🤖 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/event/src/lib.rs` around lines 1279 - 1284, The zk attendance flow
in verify_and_attend and the downstream pay_for_ticket/mint_ticket path must not
process paid tiers without an authenticated payer/recipient. Update the
attendance API to require explicit payer/recipient inputs or otherwise reject
paid/minted registrations until a privacy-preserving recipient flow exists, and
make sure sold_count is only incremented after a valid payment/mint succeeds.
Also replace the current use of env.current_contract_address() with nonce 0 in
the ticket minting flow so the attendee is uniquely identified and nonce
collisions cannot occur.
Comment on lines
+1279
to
+1319
| pub fn verify_and_attend( | ||
| env: Env, | ||
| event_id: Symbol, | ||
| tier_id: u32, | ||
| claim: ZkPassportClaim, | ||
| ) -> Result<(), EventError> { | ||
| let mut event = storage::get_event(&env, &event_id)?; | ||
|
|
||
| // 1. Event must be Active. | ||
| if event.status != EventStatus::Active { | ||
| return Err(EventError::EventNotActive); | ||
| } | ||
|
|
||
| // 2. This path is only for verification-gated events. | ||
| if !event.requires_verification { | ||
| return Err(EventError::ZkVerificationRequired); | ||
| } | ||
|
|
||
| // 3. Organizer must have explicitly enabled zkPassport for this event. | ||
| let zk_config = storage::get_zk_verification_config(&env, &event_id); | ||
| if !zk_config.enabled { | ||
| return Err(EventError::ZkVerificationRequired); | ||
| } | ||
|
|
||
| // 4. Proof must not be expired. | ||
| let current_ledger = env.ledger().sequence(); | ||
| if claim.expiry_ledger < current_ledger { | ||
| return Err(EventError::ZkProofExpired); | ||
| } | ||
|
|
||
| // 5. Validate claim type if the organizer specified one. | ||
| if zk_config.required_claim_type != ZkClaimType::Any | ||
| && claim.claim_type != zk_config.required_claim_type | ||
| { | ||
| return Err(EventError::ZkClaimTypeMismatch); | ||
| } | ||
|
|
||
| // 6. Nullifier must be fresh — no proof reuse allowed. | ||
| if storage::has_zk_nullifier(&env, &event_id, &claim.nullifier) { | ||
| return Err(EventError::ZkNullifierReused); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Validate the zk proof before consuming attendance.
claim.proof is never read; the flow only checks expiry, claim type, and nullifier freshness. Any caller can submit arbitrary bytes with a fresh nullifier and satisfy a gated event. Call an on-chain verifier, or authenticate a trusted verifier/relayer, and return ZkProofInvalid before nullifier/capacity/payment side effects.
🤖 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/event/src/lib.rs` around lines 1279 - 1319, The verify_and_attend
flow accepts claim.proof without validating it, so arbitrary bytes can pass as
long as the nullifier is fresh. In verify_and_attend, add an actual zk proof
verification step before any attendance side effects, using the existing
ZkPassportClaim and event/zk config context, and return ZkProofInvalid on
failure. Make sure this happens before nullifier consumption, capacity checks,
or payment/attendance mutations so only a verified proof can proceed.
Comment on lines
+453
to
+456
| env.storage().persistent().set(&key, &true); | ||
| env.storage() | ||
| .persistent() | ||
| .extend_ttl(&key, TTL_THRESHOLD, TTL_BUMP); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all nullifier writes and proof-expiry bounds before deciding the final TTL policy.
rg -n "save_zk_nullifier|has_zk_nullifier|expiry_ledger|TTL_BUMP|event_end_ledger" contracts/event/srcRepository: BuidlZone-Labs/zicket-contract
Length of output: 5818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== storage.rs (nullifier helpers) =="
sed -n '430,470p' contracts/event/src/storage.rs | cat -n
echo
echo "== lib.rs (claim validation / save nullifier path) =="
sed -n '1248,1385p' contracts/event/src/lib.rs | cat -n
echo
echo "== types.rs (claim/event fields) =="
sed -n '1,150p' contracts/event/src/types.rs | cat -n
echo
echo "== errors.rs (expiry-related errors) =="
sed -n '1,90p' contracts/event/src/errors.rs | cat -nRepository: BuidlZone-Labs/zicket-contract
Length of output: 17898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== storage.rs constants and nearby helpers =="
sed -n '1,60p' contracts/event/src/storage.rs | cat -n
echo
echo "== claim construction / tests that populate expiry_ledger =="
rg -n "expiry_ledger\s*:" contracts/event/src/test*.rs contracts/event/src/integration_tests.rs contracts/event/src/lib.rs
echo
echo "== any validation against event_end_ledger or max expiry =="
rg -n "event_end_ledger.*expiry_ledger|expiry_ledger.*event_end_ledger|max.*expiry|expiry.*ledger" contracts/event/srcRepository: BuidlZone-Labs/zicket-contract
Length of output: 4072
Tie nullifier TTL to the proof replay window. save_zk_nullifier uses a fixed TTL_BUMP, but claim.expiry_ledger is only checked against the current ledger and is not capped by event_end_ledger or any other maximum. A proof can stay valid after the tombstone expires, which would let the same nullifier be accepted again. Bound expiry_ledger to the intended replay window or extend the nullifier storage to cover it.
🤖 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/event/src/storage.rs` around lines 453 - 456, The nullifier
persistence in save_zk_nullifier currently uses a fixed TTL_BUMP that may expire
before the proof replay window ends, allowing the same nullifier to be accepted
again. Update the logic around save_zk_nullifier and the claim.expiry_ledger
handling so the stored tombstone lifetime is tied to the full intended replay
window, either by capping expiry_ledger against the maximum event window or by
extending the persistent storage TTL accordingly. Ensure the relevant symbols
save_zk_nullifier, claim.expiry_ledger, and extend_ttl are adjusted together so
the nullifier remains valid for the entire replay period.
Comment on lines
+3
to
+9
| //! Acceptance criteria covered: | ||
| //! [AC-1] ZkPassportClaim struct (claim_type / proof / nullifier / expiry_ledger) | ||
| //! [AC-2] verify_and_attend entry point on the Event contract | ||
| //! [AC-3] Nullifier stored on-chain to prevent proof reuse across events | ||
| //! [AC-4] Proof expiry checked against current ledger sequence | ||
| //! [AC-5] Verification result gates ticket issuance | ||
| //! [AC-6] Proof bytes are NEVER stored; only nullifier is persisted |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a cross-event nullifier replay test.
The objective here is reuse prevention across events, but this suite only replays the same claim against one event_id. That gap currently misses the upstream bug in contracts/event/src/lib.rs:1279-1386, where nullifier lookup/persistence is still scoped by event_id, so the same proof can be replayed on a second event.
Suggested test shape
+#[test]
+fn test_nullifier_reuse_rejected_across_events() {
+ let env = setup_env();
+ let contract_id = env.register(EventContract, ());
+ let client = EventContractClient::new(&env, &contract_id);
+ let organizer = Address::generate(&env);
+
+ let event_a = setup_verified_event(&env, &client, &organizer);
+ let event_b = Symbol::new(&env, "ev_zk_02");
+ // create/activate/configure event_b the same way as event_a
+
+ let claim = make_claim(&env, ZkClaimType::Age, 42, 9_999);
+ client.verify_and_attend(&event_a, &0u32, &claim);
+
+ let result = client.try_verify_and_attend(&event_b, &0u32, &claim);
+ assert_eq!(result, Err(Ok(EventError::ZkNullifierReused)));
+}Also applies to: 138-165
🤖 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/event/src/test_zk_passport.rs` around lines 3 - 9, Add a
cross-event replay test for the ZkPassport flow: the current coverage around
ZkPassportClaim and verify_and_attend only reuses a proof within one event, so
it misses the bug where nullifier tracking is still scoped by event_id. Extend
the test suite to create two distinct event_ids, submit the same
claim/proof/nullifier to the first event, then attempt the same submission on
the second event and assert it is rejected because the nullifier is already used
globally. Make sure the test exercises the on-chain nullifier persistence path
rather than only same-event replay behavior.
Comment on lines
+50
to
+58
| /// - `required_claim_type`: which proof category attendees must present. | ||
| /// `None` means the event accepts any valid ZK claim type. | ||
| /// - `enabled`: master switch; when `false` the `verify_and_attend` path is | ||
| /// disabled even if `requires_verification` is set on the event. | ||
| #[contracttype] | ||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub struct ZkVerificationConfig { | ||
| /// Which proof category attendees must present. Use `ZkClaimType::Any` | ||
| /// to accept any valid ZK claim type. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use ZkClaimType::Any consistently in config docs.
required_claim_type is not optional, so the None wording can mislead integrators. Update this and the matching set_zk_config docs to describe the Any sentinel.
Proposed docs fix
-/// - `required_claim_type`: which proof category attendees must present.
-/// `None` means the event accepts any valid ZK claim type.
+/// - `required_claim_type`: which proof category attendees must present.
+/// `ZkClaimType::Any` means the event accepts any valid ZK claim type.📝 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
| /// - `required_claim_type`: which proof category attendees must present. | |
| /// `None` means the event accepts any valid ZK claim type. | |
| /// - `enabled`: master switch; when `false` the `verify_and_attend` path is | |
| /// disabled even if `requires_verification` is set on the event. | |
| #[contracttype] | |
| #[derive(Clone, Debug, Eq, PartialEq)] | |
| pub struct ZkVerificationConfig { | |
| /// Which proof category attendees must present. Use `ZkClaimType::Any` | |
| /// to accept any valid ZK claim type. | |
| /// - `required_claim_type`: which proof category attendees must present. | |
| /// `ZkClaimType::Any` means the event accepts any valid ZK claim type. | |
| /// - `enabled`: master switch; when `false` the `verify_and_attend` path is | |
| /// disabled even if `requires_verification` is set on the event. | |
| #[contracttype] | |
| #[derive(Clone, Debug, Eq, PartialEq)] | |
| pub struct ZkVerificationConfig { | |
| /// Which proof category attendees must present. Use `ZkClaimType::Any` | |
| /// to accept any valid ZK claim type. |
🤖 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/event/src/types.rs` around lines 50 - 58, The ZkVerificationConfig
docs are inconsistent about the sentinel for accepting any claim type: replace
the misleading “None” wording with `ZkClaimType::Any` in the struct-level
comments, and update the matching `set_zk_config` documentation to describe the
same `Any` sentinel. Use the existing `ZkVerificationConfig` and `set_zk_config`
symbols to keep the terminology aligned across the config API.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/payments/src/test.rs (1)
1769-1802: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThis test is still hitting
TransferFailed, not the duplicate-nonce path.Lines 1769-1777 call
pay_for_ticketbefore the payer is funded. The samesetup_contract_with_tokensetup is used at Line 1822 onward to prove an unfunded payer returnsPaymentError::TransferFailed, so this never reliably reaches the nonce assertion or replay check.Suggested fix
fn test_idempotent_payment() { let env = Env::default(); env.mock_all_auths(); let (_admin, token, client, _, _, _) = setup_contract_with_token(&env); let payer = Address::generate(&env); let event_id = symbol_short!("EVENT1"); let amount = 100_000_000i128; let nonce = 12345u64; + token::StellarAssetClient::new(&env, &token).mint(&payer, &amount); client.pay_for_ticket( &nonce, &payer, &event_id, @@ assert!( has_in_storage, "Nonce should be in storage after first call" ); - let nonce = 123456u64; - client.pay_for_ticket( - &nonce, - &payer, - &event_id, - &amount, - &None, - &token, - &PaymentPrivacy::Standard, - ); - client.pay_for_ticket( + let duplicate = client.try_pay_for_ticket( &nonce, &payer, &event_id, &amount, &None, &token, &PaymentPrivacy::Standard, ); + assert_eq!(duplicate.err(), Some(Ok(PaymentError::DuplicateRequest))); }🤖 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/payments/src/test.rs` around lines 1769 - 1802, The test is exercising the wrong failure path because `pay_for_ticket` is called before the payer has any token balance, so it trips `TransferFailed` before the nonce logic in `storage::has_nonce` can be validated. Update this test around `pay_for_ticket`/`setup_contract_with_token` so the payer is funded first (or reuse the funded setup used elsewhere in the test suite), then make the initial payment succeed and assert the stored nonce before calling `pay_for_ticket` again with the same nonce to hit the duplicate-nonce path.
🧹 Nitpick comments (1)
contracts/payments/src/test.rs (1)
2648-2653: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive these ledger boundaries from
MIN_DISPUTE_WINDOW_LEDGERS.The new
1050/1100literals bake the current dispute-window size into the test. Computing them from the shared constant will keep this assertion correct if the window changes 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/payments/src/test.rs` around lines 2648 - 2653, The test in the try_withdraw flow is hardcoding ledger sequence numbers for the escrow-expiration boundary, which should instead be derived from MIN_DISPUTE_WINDOW_LEDGERS. Update the setup around client.try_withdraw and env.ledger().with_mut so the pre- and post-boundary sequence_number values are computed from the shared constant rather than using fixed 1050 and 1100 literals, keeping the assertion aligned with the dispute window definition.
🤖 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/event/src/test_anon_claims.rs`:
- Line 36: Remove the stray standalone / tokens from test_anon_claims.rs; these
invalid Rust syntax lines are blocking compilation. Clean up the affected test
sections around the anonymous claims tests so only valid statements remain, and
verify the file still compiles after deleting the extra / lines.
In `@contracts/event/src/test_claims.rs`:
- Line 16: Remove the stray module-level slash tokens from the test file so it
compiles cleanly; the invalid `/` lines appear outside any Rust item and should
be deleted from the `test_claims` module near the affected sections. Keep the
surrounding test functions and imports intact, and verify there are no other
standalone `/` characters left in the module.
In `@contracts/event/src/test.rs`:
- Line 1174: The test module in contracts/event/src/test.rs contains a stray
slash token at module scope, which leaves invalid Rust syntax and breaks
compilation. Remove the extra token from the affected area in the test file,
keeping the surrounding test/module definitions intact.
In `@contracts/factory/src/lib.rs`:
- Around line 111-116: Remove the stray standalone “/” lines around
contract_version and migrate in the factory lib module; they are not valid Rust
syntax. Clean up the surrounding contract_version function block and the migrate
section so only valid Rust items and comments remain, using the existing
contract_version and migrate symbols to locate the affected area.
In `@contracts/payments/src/lib.rs`:
- Around line 653-656: The dispute-window unlock calculation in the escrow check
can wrap on large ledger values, so update the logic around cancel_ledger and
MIN_DISPUTE_WINDOW_LEDGERS to use saturating or checked addition before
comparing against current_ledger. Keep the guard in the same escrow-expiration
path in lib.rs, and ensure the fallback behavior preserves the closed escrow
state instead of allowing an incorrect unlock.
In `@contracts/ticket/src/lib.rs`:
- Around line 186-201: Remove the stray standalone “/” tokens around the public
query methods in lib.rs; they are invalid Rust syntax and prevent compilation.
Clean up the accidental slash lines immediately before get_ticket,
get_owner_tickets, and get_event_tickets (and the other matching occurrences
later in the file), leaving only the method definitions and their existing
bodies intact.
---
Outside diff comments:
In `@contracts/payments/src/test.rs`:
- Around line 1769-1802: The test is exercising the wrong failure path because
`pay_for_ticket` is called before the payer has any token balance, so it trips
`TransferFailed` before the nonce logic in `storage::has_nonce` can be
validated. Update this test around `pay_for_ticket`/`setup_contract_with_token`
so the payer is funded first (or reuse the funded setup used elsewhere in the
test suite), then make the initial payment succeed and assert the stored nonce
before calling `pay_for_ticket` again with the same nonce to hit the
duplicate-nonce path.
---
Nitpick comments:
In `@contracts/payments/src/test.rs`:
- Around line 2648-2653: The test in the try_withdraw flow is hardcoding ledger
sequence numbers for the escrow-expiration boundary, which should instead be
derived from MIN_DISPUTE_WINDOW_LEDGERS. Update the setup around
client.try_withdraw and env.ledger().with_mut so the pre- and post-boundary
sequence_number values are computed from the shared constant rather than using
fixed 1050 and 1100 literals, keeping the assertion aligned with the dispute
window definition.
🪄 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: a5dcc5e7-55e4-400a-b52a-92a8bbeeaf31
📒 Files selected for processing (26)
contracts/event/src/errors.rscontracts/event/src/events.rscontracts/event/src/integration_tests.rscontracts/event/src/lib.rscontracts/event/src/storage.rscontracts/event/src/test.rscontracts/event/src/test_anon_claims.rscontracts/event/src/test_claims.rscontracts/event/src/test_privacy.rscontracts/event/src/test_zk_passport.rscontracts/event/src/types.rscontracts/factory/src/lib.rscontracts/factory/src/storage.rscontracts/payments/src/errors.rscontracts/payments/src/events.rscontracts/payments/src/lib.rscontracts/payments/src/multi_token_test.rscontracts/payments/src/receipt_commitment_test.rscontracts/payments/src/storage.rscontracts/payments/src/test.rscontracts/payments/src/types.rscontracts/privacy-utils/src/lib.rscontracts/privacy-utils/src/test.rscontracts/ticket/src/lib.rscontracts/ticket/src/storage.rscontracts/ticket/src/test.rs
💤 Files with no reviewable changes (3)
- contracts/event/src/test_privacy.rs
- contracts/privacy-utils/src/test.rs
- contracts/payments/src/multi_token_test.rs
✅ Files skipped from review due to trivial changes (5)
- contracts/payments/src/events.rs
- contracts/factory/src/storage.rs
- contracts/privacy-utils/src/lib.rs
- contracts/ticket/src/storage.rs
- contracts/payments/src/storage.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- contracts/event/src/test_zk_passport.rs
- contracts/event/src/types.rs
- contracts/event/src/events.rs
- contracts/event/src/errors.rs
- contracts/event/src/storage.rs
- contracts/event/src/lib.rs
Comment on lines
653
to
656
| if let Some(cancel_ledger) = config.cancel_ledger { | ||
| let min_dispute_unlock = cancel_ledger + MIN_DISPUTE_WINDOW_LEDGERS; | ||
| if current_ledger < min_dispute_unlock { | ||
| return Err(PaymentError::EscrowNotExpired); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use non-wrapping arithmetic for the dispute-window unlock.
cancel_ledger + MIN_DISPUTE_WINDOW_LEDGERS can wrap on large ledger numbers and make this guard evaluate incorrectly. A saturating/checked add keeps the escrow closed instead of silently reopening it.
Suggested fix
- let min_dispute_unlock = cancel_ledger + MIN_DISPUTE_WINDOW_LEDGERS;
+ let min_dispute_unlock =
+ cancel_ledger.saturating_add(MIN_DISPUTE_WINDOW_LEDGERS);📝 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
| if let Some(cancel_ledger) = config.cancel_ledger { | |
| let min_dispute_unlock = cancel_ledger + MIN_DISPUTE_WINDOW_LEDGERS; | |
| if current_ledger < min_dispute_unlock { | |
| return Err(PaymentError::EscrowNotExpired); | |
| if let Some(cancel_ledger) = config.cancel_ledger { | |
| let min_dispute_unlock = | |
| cancel_ledger.saturating_add(MIN_DISPUTE_WINDOW_LEDGERS); | |
| if current_ledger < min_dispute_unlock { | |
| return Err(PaymentError::EscrowNotExpired); |
🤖 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/payments/src/lib.rs` around lines 653 - 656, The dispute-window
unlock calculation in the escrow check can wrap on large ledger values, so
update the logic around cancel_ledger and MIN_DISPUTE_WINDOW_LEDGERS to use
saturating or checked addition before comparing against current_ledger. Keep the
guard in the same escrow-expiration path in lib.rs, and ensure the fallback
behavior preserves the closed escrow state instead of allowing an incorrect
unlock.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/event/src/lib.rs (3)
775-779: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winBlock normal registration for zk-gated events.
register_for_eventnever checksevent.requires_verification, so callers can bypassverify_and_attendentirely for gated events. ReturnZkVerificationRequiredhere and force zk-gated attendance through the verified path.Proposed fix
if event.status != EventStatus::Active { return Err(EventError::EventNotActive); } + if event.requires_verification { + return Err(EventError::ZkVerificationRequired); + }🤖 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/event/src/lib.rs` around lines 775 - 779, Update register_for_event in the event registration flow so zk-gated events cannot use the normal path: after loading the event and checking EventStatus::Active, also inspect event.requires_verification and return EventError::ZkVerificationRequired when it is set. Make sure the verified attendance path (verify_and_attend) remains the only way to register for gated events, and use the existing event/status checks in register_for_event to place the new guard alongside them.
1188-1190: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake nullifier replay protection global, not per event.
The PR objective requires preventing proof reuse across events, but these calls use
(event_id, nullifier), so the same nullifier can be reused for a different event. Store/check the nullifier commitment globally, or add a separate global nullifier index.Also applies to: 1230-1230, 1270-1272
🤖 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/event/src/lib.rs` around lines 1188 - 1190, The replay check in the event claim flow still scopes nullifier protection to a single event, so the same proof can be reused under a different event. Update the nullifier lookup/write path used by the relevant claim handlers in `contracts/event/src/lib.rs` (including the `storage::has_zk_nullifier` call sites and the matching persistence logic) to use a global nullifier index or globally unique commitment instead of `(event_id, nullifier)`. Make sure the `ZkNullifierReused` guard is enforced against that global store everywhere nullifiers are accepted.
625-648: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCancel the exact ticket being refunded.
Line 647 refunds
ticket_id, but Lines 635-646 select the first valid ticket for the same attendee/event and Line 648 cancels that unrelated ID. With multiple tickets, this can leave the refunded ticket valid and cancel a different one. Use the exact ticket mapped to the refunded payment, or persist a payment→ticket mapping if the ID spaces differ.🤖 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/event/src/lib.rs` around lines 625 - 648, The refund flow in the event contract is cancelling the wrong ticket: the logic in the ticket lookup loop picks the first valid ticket for the attendee/event, but the refund is requested for the original ticket_id, so the canceled ticket may not match the refunded one. Update the refund path in the same function so the ticket canceled by TicketContractClient::cancel_ticket is the exact ticket associated with the payment refund, or introduce a payment-to-ticket mapping if PaymentsContractClient and TicketContractClient use different IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@contracts/event/src/lib.rs`:
- Around line 775-779: Update register_for_event in the event registration flow
so zk-gated events cannot use the normal path: after loading the event and
checking EventStatus::Active, also inspect event.requires_verification and
return EventError::ZkVerificationRequired when it is set. Make sure the verified
attendance path (verify_and_attend) remains the only way to register for gated
events, and use the existing event/status checks in register_for_event to place
the new guard alongside them.
- Around line 1188-1190: The replay check in the event claim flow still scopes
nullifier protection to a single event, so the same proof can be reused under a
different event. Update the nullifier lookup/write path used by the relevant
claim handlers in `contracts/event/src/lib.rs` (including the
`storage::has_zk_nullifier` call sites and the matching persistence logic) to
use a global nullifier index or globally unique commitment instead of
`(event_id, nullifier)`. Make sure the `ZkNullifierReused` guard is enforced
against that global store everywhere nullifiers are accepted.
- Around line 625-648: The refund flow in the event contract is cancelling the
wrong ticket: the logic in the ticket lookup loop picks the first valid ticket
for the attendee/event, but the refund is requested for the original ticket_id,
so the canceled ticket may not match the refunded one. Update the refund path in
the same function so the ticket canceled by TicketContractClient::cancel_ticket
is the exact ticket associated with the payment refund, or introduce a
payment-to-ticket mapping if PaymentsContractClient and TicketContractClient use
different IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ea4806d-fb85-437b-b8e2-80632a141b6d
📒 Files selected for processing (21)
contracts/event/src/errors.rscontracts/event/src/events.rscontracts/event/src/lib.rscontracts/event/src/storage.rscontracts/event/src/test.rscontracts/event/src/test_anon_claims.rscontracts/event/src/test_claims.rscontracts/event/src/test_zk_passport.rscontracts/event/src/types.rscontracts/factory/src/lib.rscontracts/factory/src/storage.rscontracts/payments/src/errors.rscontracts/payments/src/events.rscontracts/payments/src/lib.rscontracts/payments/src/receipt_commitment_test.rscontracts/payments/src/storage.rscontracts/payments/src/test.rscontracts/payments/src/types.rscontracts/privacy-utils/src/lib.rscontracts/ticket/src/lib.rscontracts/ticket/src/storage.rs
✅ Files skipped from review due to trivial changes (9)
- contracts/privacy-utils/src/lib.rs
- contracts/payments/src/types.rs
- contracts/payments/src/events.rs
- contracts/payments/src/errors.rs
- contracts/ticket/src/storage.rs
- contracts/factory/src/storage.rs
- contracts/event/src/test_claims.rs
- contracts/ticket/src/lib.rs
- contracts/event/src/test_anon_claims.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- contracts/event/src/types.rs
- contracts/event/src/events.rs
- contracts/event/src/test_zk_passport.rs
- contracts/factory/src/lib.rs
- contracts/payments/src/receipt_commitment_test.rs
- contracts/payments/src/storage.rs
- contracts/payments/src/lib.rs
- contracts/event/src/storage.rs
- contracts/payments/src/test.rs
- contracts/event/src/test.rs
…-Labs#122) - Align FlaggedShareResolved event topic with payload name - Settle cancelled split events from payment records after refunds - Clear aggregate EventRevenue on full split settlement - Refresh split storage TTL on reads - Require split index 0 to match event organizer - Add test for split withdrawal after attendee refund - Run cargo fmt Co-authored-by: Cursor <cursoragent@cursor.com>
…-organizer-revenue-split feat: multi-organizer revenue split and co-host wallet management
Contributor
|
@codeZe-us pls kindly resolve the merge conflicts |
DioChuks
self-requested a review
June 28, 2026 18:43
…itment and verify_and_attend entry point
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/event/src/lib.rs (3)
721-725: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winBlock the standard registration path for zk-gated events.
register_for_eventnever checksevent.requires_verification, so callers can bypassverify_and_attendentirely for gated events.Proposed fix
if event.status != EventStatus::Active { return Err(EventError::EventNotActive); } + if event.requires_verification { + return Err(EventError::ZkVerificationRequired); + }🤖 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/event/src/lib.rs` around lines 721 - 725, The standard registration path in register_for_event currently allows zk-gated events to be registered without verification. Add a guard after loading the event and checking EventStatus::Active that rejects registrations when event.requires_verification is true, and keep verify_and_attend as the only path for those events. Use the existing register_for_event and verify_and_attend flow, along with EventError, to route gated events away from the normal registration branch.
1060-1102: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftUse a global nullifier commitment, not an event-scoped one.
The PR objective requires proof reuse prevention across events, but both the reuse check and save include
event_id, allowing the same nullifier to be accepted by a different event.Conceptual fix
- if storage::has_zk_nullifier(&env, &event_id, &claim.nullifier) { + if storage::has_zk_nullifier_global(&env, &claim.nullifier) { return Err(EventError::ZkNullifierReused); } ... - storage::save_zk_nullifier(&env, &event_id, &claim.nullifier); + storage::save_zk_nullifier_global(&env, &claim.nullifier);🤖 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/event/src/lib.rs` around lines 1060 - 1102, The nullifier reuse check is scoped to the current event, so the same proof can be reused on another event. Update the logic around storage::has_zk_nullifier and storage::save_zk_nullifier in the ticket-claim flow to use a global nullifier commitment based on claim.nullifier only, not event_id. Make sure the lookup and persistence key the nullifier universally so reuse is prevented across all events while keeping the existing TicketContractClient and PaymentsContractClient flow unchanged.
390-394: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSync completed/active status to payments before split withdrawals.
withdraw_splitdepends on payments-side event status, butupdate_event_statusonly updates event storage. The new integration tests compensate by callingpayments_client.set_event_statusdirectly; production callers through the event front door will not.Proposed fix
update_event(&env, &event_id, &event)?; + if has_linked_contracts(&env) { + let payments_contract = get_payments_contract(&env)?; + let payments_client = PaymentsContractClient::new(&env, &payments_contract); + let payments_status = match new_status { + EventStatus::Active => payments_contract::EventStatus::Active, + EventStatus::Completed => payments_contract::EventStatus::Completed, + _ => return Err(EventError::InvalidStatusTransition), + }; + payments_client.set_event_status(&organizer, &event_id, &payments_status); + } emit_status_changed(&env, &event_id, &old_status, &new_status);🤖 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/event/src/lib.rs` around lines 390 - 394, The status update in update_event_status only writes to event storage, so withdrawals that rely on the payments-side status can see stale state. Update update_event_status to also synchronize the new status through the payments flow (for example by calling the payments client’s set_event_status path before emit_status_changed), using the existing event_id, old_status, and new_status context so front-door callers and withdraw_split stay consistent.
♻️ Duplicate comments (1)
contracts/payments/src/lib.rs (1)
374-384: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse non-wrapping ledger arithmetic in split settlement.
event_end_ledger + ...andcancel_ledger + MIN_DISPUTE_WINDOW_LEDGERScan wrap and incorrectly unlock escrow. Apply the same saturating/checked-add fix here too.Proposed fix
- let unlock_ledger = config.event_end_ledger - + config.withdrawal_delay_ledgers - + config.admin_delay_extension_ledgers; + let unlock_ledger = config + .event_end_ledger + .saturating_add(config.withdrawal_delay_ledgers) + .saturating_add(config.admin_delay_extension_ledgers); ... - if current_ledger < cancel_ledger + MIN_DISPUTE_WINDOW_LEDGERS { + if current_ledger < cancel_ledger.saturating_add(MIN_DISPUTE_WINDOW_LEDGERS) {🤖 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/payments/src/lib.rs` around lines 374 - 384, The split settlement expiry checks in the escrow status match use wrapping ledger arithmetic, which can incorrectly unlock funds if the additions overflow. Update the logic in the EventStatus::Cancelled and related unlock path to use the same saturating or checked-add pattern already used elsewhere, specifically around unlock_ledger and cancel_ledger + MIN_DISPUTE_WINDOW_LEDGERS. Keep the behavior consistent by comparing current_ledger against the safely computed ledger threshold in this branch of the status handling.
🤖 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/payments/src/errors.rs`:
- Around line 42-60: The enum discriminants in the payment error type were
renumbered, which breaks existing error-code stability. Keep
CommitmentAlreadySet and CommitmentNotAllowed on their current discriminants,
and move the new split-related errors in the payments error enum so they are
assigned after those existing codes. Update the relevant variants in
contracts/payments/src/errors.rs without changing the established numeric values
for the preexisting errors.
In `@contracts/payments/src/lib.rs`:
- Around line 1291-1296: Remove the unresolved merge-conflict markers from the
affected sections in contracts/payments/src/lib.rs and keep only the intended
implementation. In the conflicted blocks around ensure_no_splits and the nearby
escrow/comment logic, delete the <<<<<<<, =======, and >>>>>>> lines and
preserve the correct code path consistently across all reported occurrences,
including the other conflict locations referenced in the review.
- Around line 441-453: Read the event revenue before updating token revenue in
the withdrawal flow. In the branch that uses storage::set_event_token_revenue
and storage::set_event_revenue, capture current_rev with
storage::get_event_revenue(env, event_id) before any token revenue mutation,
then apply the token revenue decrement and use the pre-mutation value for the
total revenue update. Keep the same ordering in the corresponding else path so
get_event_revenue is never called after set_event_token_revenue in this
function.
---
Outside diff comments:
In `@contracts/event/src/lib.rs`:
- Around line 721-725: The standard registration path in register_for_event
currently allows zk-gated events to be registered without verification. Add a
guard after loading the event and checking EventStatus::Active that rejects
registrations when event.requires_verification is true, and keep
verify_and_attend as the only path for those events. Use the existing
register_for_event and verify_and_attend flow, along with EventError, to route
gated events away from the normal registration branch.
- Around line 1060-1102: The nullifier reuse check is scoped to the current
event, so the same proof can be reused on another event. Update the logic around
storage::has_zk_nullifier and storage::save_zk_nullifier in the ticket-claim
flow to use a global nullifier commitment based on claim.nullifier only, not
event_id. Make sure the lookup and persistence key the nullifier universally so
reuse is prevented across all events while keeping the existing
TicketContractClient and PaymentsContractClient flow unchanged.
- Around line 390-394: The status update in update_event_status only writes to
event storage, so withdrawals that rely on the payments-side status can see
stale state. Update update_event_status to also synchronize the new status
through the payments flow (for example by calling the payments client’s
set_event_status path before emit_status_changed), using the existing event_id,
old_status, and new_status context so front-door callers and withdraw_split stay
consistent.
---
Duplicate comments:
In `@contracts/payments/src/lib.rs`:
- Around line 374-384: The split settlement expiry checks in the escrow status
match use wrapping ledger arithmetic, which can incorrectly unlock funds if the
additions overflow. Update the logic in the EventStatus::Cancelled and related
unlock path to use the same saturating or checked-add pattern already used
elsewhere, specifically around unlock_ledger and cancel_ledger +
MIN_DISPUTE_WINDOW_LEDGERS. Keep the behavior consistent by comparing
current_ledger against the safely computed ledger threshold in this branch of
the status handling.
🪄 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: a6d4b015-b1c1-43e5-863b-a54e7cee6db5
📒 Files selected for processing (14)
contracts/event/src/errors.rscontracts/event/src/integration_tests.rscontracts/event/src/lib.rscontracts/event/src/test.rscontracts/event/src/test_anon_claims.rscontracts/event/src/test_claims.rscontracts/event/src/test_privacy.rscontracts/event/src/types.rscontracts/payments/src/errors.rscontracts/payments/src/events.rscontracts/payments/src/lib.rscontracts/payments/src/revenue_split_test.rscontracts/payments/src/storage.rscontracts/payments/src/types.rs
✅ Files skipped from review due to trivial changes (1)
- contracts/event/src/test_privacy.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/event/src/types.rs
- contracts/event/src/test_claims.rs
- contracts/event/src/test.rs
Comment on lines
+42
to
+60
| /// Revenue split configuration is invalid (bad sum, too many recipients, | ||
| /// duplicate or empty recipient, or an attempt to mutate an existing config). | ||
| InvalidSplitConfig = 35, | ||
| /// No revenue split has been configured for this event. | ||
| SplitsNotConfigured = 36, | ||
| /// The caller is not one of the configured split recipients. | ||
| NotASplitRecipient = 37, | ||
| /// This recipient has already withdrawn (or had reassigned) its split share. | ||
| SplitAlreadyWithdrawn = 38, | ||
| /// The recipient's share is frozen because the wallet has been flagged. | ||
| RecipientFlagged = 39, | ||
| /// The recipient is not currently flagged. | ||
| RecipientNotFlagged = 40, | ||
| /// A zkEmail commitment is already bound to this payment; commitments are | ||
| /// write-once and cannot be overwritten. | ||
| CommitmentAlreadySet = 35, | ||
| CommitmentAlreadySet = 41, | ||
| /// The payment is in a state that no longer accepts a commitment | ||
| /// (e.g. it has been refunded). | ||
| CommitmentNotAllowed = 36, | ||
| CommitmentNotAllowed = 42, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve existing error discriminants.
CommitmentAlreadySet and CommitmentNotAllowed were renumbered from their existing codes. Keep existing error codes stable and assign the new split errors after them.
Proposed fix
- InvalidSplitConfig = 35,
+ CommitmentAlreadySet = 35,
+ CommitmentNotAllowed = 36,
+ InvalidSplitConfig = 37,
- SplitsNotConfigured = 36,
+ SplitsNotConfigured = 38,
- NotASplitRecipient = 37,
+ NotASplitRecipient = 39,
- SplitAlreadyWithdrawn = 38,
+ SplitAlreadyWithdrawn = 40,
- RecipientFlagged = 39,
+ RecipientFlagged = 41,
- RecipientNotFlagged = 40,
+ RecipientNotFlagged = 42,
- CommitmentAlreadySet = 41,
- CommitmentNotAllowed = 42,📝 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
| /// Revenue split configuration is invalid (bad sum, too many recipients, | |
| /// duplicate or empty recipient, or an attempt to mutate an existing config). | |
| InvalidSplitConfig = 35, | |
| /// No revenue split has been configured for this event. | |
| SplitsNotConfigured = 36, | |
| /// The caller is not one of the configured split recipients. | |
| NotASplitRecipient = 37, | |
| /// This recipient has already withdrawn (or had reassigned) its split share. | |
| SplitAlreadyWithdrawn = 38, | |
| /// The recipient's share is frozen because the wallet has been flagged. | |
| RecipientFlagged = 39, | |
| /// The recipient is not currently flagged. | |
| RecipientNotFlagged = 40, | |
| /// A zkEmail commitment is already bound to this payment; commitments are | |
| /// write-once and cannot be overwritten. | |
| CommitmentAlreadySet = 35, | |
| CommitmentAlreadySet = 41, | |
| /// The payment is in a state that no longer accepts a commitment | |
| /// (e.g. it has been refunded). | |
| CommitmentNotAllowed = 36, | |
| CommitmentNotAllowed = 42, | |
| /// Revenue split configuration is invalid (bad sum, too many recipients, | |
| /// duplicate or empty recipient, or an attempt to mutate an existing config). | |
| CommitmentAlreadySet = 35, | |
| /// The payment is in a state that no longer accepts a commitment | |
| /// (e.g. it has been refunded). | |
| CommitmentNotAllowed = 36, | |
| InvalidSplitConfig = 37, | |
| /// No revenue split has been configured for this event. | |
| SplitsNotConfigured = 38, | |
| /// The caller is not one of the configured split recipients. | |
| NotASplitRecipient = 39, | |
| /// This recipient has already withdrawn (or had reassigned) its split share. | |
| SplitAlreadyWithdrawn = 40, | |
| /// The recipient's share is frozen because the wallet has been flagged. | |
| RecipientFlagged = 41, | |
| /// The recipient is not currently flagged. | |
| RecipientNotFlagged = 42, |
🧰 Tools
🪛 GitHub Actions: CI / 2_Test.txt
[error] cargo test --all-features failed because could not compile payments-contract (lib) due to 1 previous error.
🤖 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/payments/src/errors.rs` around lines 42 - 60, The enum
discriminants in the payment error type were renumbered, which breaks existing
error-code stability. Keep CommitmentAlreadySet and CommitmentNotAllowed on
their current discriminants, and move the new split-related errors in the
payments error enum so they are assigned after those existing codes. Update the
relevant variants in contracts/payments/src/errors.rs without changing the
established numeric values for the preexisting errors.
Comment on lines
+441
to
+453
| storage::set_event_token_revenue(env, event_id, &payout_token, 0); | ||
| let current_rev = storage::get_event_revenue(env, event_id); | ||
| storage::set_event_revenue(env, event_id, current_rev - total_to_withdraw); | ||
| } else { | ||
| let current_token_rev = storage::get_event_token_revenue(env, event_id, &payout_token); | ||
| storage::set_event_token_revenue( | ||
| env, | ||
| event_id, | ||
| &payout_token, | ||
| current_token_rev - total_to_withdraw, | ||
| ); | ||
| let current_rev = storage::get_event_revenue(env, event_id); | ||
| storage::set_event_revenue(env, event_id, current_rev - total_to_withdraw); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Read event revenue before mutating token revenue.
get_event_revenue is token-revenue-derived in this contract, so reading it after set_event_token_revenue can double-decrement or write a bad fallback snapshot.
Proposed fix
+ let current_rev = storage::get_event_revenue(env, event_id);
if withdrawable_ratio_bps == 10_000 {
for i in 0..payments_to_release.len() {
let mut payment = payments_to_release
.get(i)
.ok_or(PaymentError::PaymentNotFound)?;
payment.status = PaymentStatus::Released;
storage::update_payment(env, &payment)?;
}
storage::set_event_token_revenue(env, event_id, &payout_token, 0);
- let current_rev = storage::get_event_revenue(env, event_id);
storage::set_event_revenue(env, event_id, current_rev - total_to_withdraw);
} else {
let current_token_rev = storage::get_event_token_revenue(env, event_id, &payout_token);
storage::set_event_token_revenue(
env,
event_id,
&payout_token,
current_token_rev - total_to_withdraw,
);
- let current_rev = storage::get_event_revenue(env, event_id);
storage::set_event_revenue(env, event_id, current_rev - total_to_withdraw);
}📝 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
| storage::set_event_token_revenue(env, event_id, &payout_token, 0); | |
| let current_rev = storage::get_event_revenue(env, event_id); | |
| storage::set_event_revenue(env, event_id, current_rev - total_to_withdraw); | |
| } else { | |
| let current_token_rev = storage::get_event_token_revenue(env, event_id, &payout_token); | |
| storage::set_event_token_revenue( | |
| env, | |
| event_id, | |
| &payout_token, | |
| current_token_rev - total_to_withdraw, | |
| ); | |
| let current_rev = storage::get_event_revenue(env, event_id); | |
| storage::set_event_revenue(env, event_id, current_rev - total_to_withdraw); | |
| let current_rev = storage::get_event_revenue(env, event_id); | |
| storage::set_event_token_revenue(env, event_id, &payout_token, 0); | |
| storage::set_event_revenue(env, event_id, current_rev - total_to_withdraw); | |
| } else { | |
| let current_token_rev = storage::get_event_token_revenue(env, event_id, &payout_token); | |
| storage::set_event_token_revenue( | |
| env, | |
| event_id, | |
| &payout_token, | |
| current_token_rev - total_to_withdraw, | |
| ); | |
| storage::set_event_revenue(env, event_id, current_rev - total_to_withdraw); |
🧰 Tools
🪛 GitHub Actions: CI / 2_Test.txt
[error] cargo test --all-features failed because could not compile payments-contract (lib) due to 1 previous error.
🤖 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/payments/src/lib.rs` around lines 441 - 453, Read the event revenue
before updating token revenue in the withdrawal flow. In the branch that uses
storage::set_event_token_revenue and storage::set_event_revenue, capture
current_rev with storage::get_event_revenue(env, event_id) before any token
revenue mutation, then apply the token revenue decrement and use the
pre-mutation value for the total revenue update. Keep the same ordering in the
corresponding else path so get_event_revenue is never called after
set_event_token_revenue in this function.
DioChuks
approved these changes
Jun 29, 2026
This was referenced Jul 22, 2026
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
3 participants
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.
Closes the spec gap identified in #51 and #53 zkPassport was referenced in four places across the MVP spec but had zero on-chain backing. This PR adds the complete contract-level interface for submitting, validating, and consuming zkPassport proofs on the Zicket event contract.
Changes
contracts/event/src/types.rsZkClaimTypeenumAge,Location,CitizenshipZkPassportClaimstructclaim_type,proof(Bytes),nullifier(BytesN<32>),expiry_ledger(u32)ZkVerificationConfigstruct organizer-level toggle (enabled) and optionalrequired_claim_typecontracts/event/src/errors.rs30–34):ZkProofExpired,ZkNullifierReused,ZkVerificationRequired,ZkProofInvalid,ZkClaimTypeMismatchcontracts/event/src/storage.rsZkNullifier(Symbol, BytesN<32>)andZkVerificationConfig(Symbol)toDataKeyhas_zk_nullifier/save_zk_nullifiernullifier lifecycle, proof bytes are never passed to or stored in these functionsget_zk_verification_config/set_zk_verification_configorganizer config CRUD with TTL extensioncontracts/event/src/events.rsZkVerifiedAttendancecontractevent (topics = ["zk_attend"]) emitsclaim_type,tier_id,tickets_sold,registered_at. Nullifier is deliberately excluded from the event payload to prevent cross-event correlationcontracts/event/src/lib.rsverify_and_attend(event_id, tier_id, claim)primary gated attendance entry pointset_zk_config(organizer, event_id, config)organizer enables/configures ZK gatingget_zk_config(event_id)read-only view of ZK settingsis_nullifier_used(event_id, nullifier)relayer pre-screening querycontracts/event/src/test_zk_passport.rs(new)Privacy Guarantees
save_zk_nullifieronly acceptsBytesN<32>, making accidental proof persistence structurally impossible(event_id, nullifier)keyZkVerifiedAttendanceonly emitsclaim_typeto prevent observers correlating proof submissions across eventsTest Coverage
test_verify_and_attend_happy_path— Valid proof → ticket issued, sold_count incrementstest_nullifier_reuse_rejected— Same nullifier used twice →ZkNullifierReusedtest_expired_proof_rejected—expiry_ledger < sequence→ZkProofExpiredtest_non_gated_event_rejects_verify_and_attend—requires_verification = false→ZkVerificationRequiredtest_claim_type_mismatch_rejected— Wrong claim type →ZkClaimTypeMismatchtest_correct_claim_type_accepted— Exact match → successtest_zk_config_disabled_rejects—enabled = false→ZkVerificationRequiredtest_default_zk_config_rejects— Config never set →ZkVerificationRequiredtest_is_nullifier_used_query— Before/after attend reflects storagetest_only_organizer_can_set_zk_config— Intruder →Unauthorizedtest_get_zk_config_defaults— Unset config returnsenabled: false, required_claim_type: Nonetest_inactive_event_rejects_verify_and_attend— Upcoming event →EventNotActivetest_sold_out_event_rejects_verify_and_attend— Full tier →TierSoldOutcloses #118
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests