feat(confidential): wrapper contract + compliance extension#735
Open
brozorec wants to merge 7 commits into
Open
feat(confidential): wrapper contract + compliance extension#735brozorec wants to merge 7 commits into
brozorec wants to merge 7 commits into
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR implements a complete confidential token wrapper contract system for Soroban, adding Grumpkin cryptographic primitives (generator constant and scalar multiplication), the core wrapper with account registration, deposits, withdrawals, transfers, and operator delegation, plus a compliance extension layer supporting policy-based authorization and account freezing, all backed by comprehensive test coverage. ChangesConfidential Token Wrapper with Compliance
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #735 +/- ##
==========================================
- Coverage 96.71% 96.66% -0.06%
==========================================
Files 67 69 +2
Lines 6798 7344 +546
==========================================
+ Hits 6575 7099 +524
- Misses 223 245 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 4
🧹 Nitpick comments (1)
packages/tokens/src/confidential/wrapper/compliance/storage.rs (1)
59-67: ⚡ Quick winReturn the stored flag when extending frozen TTL.
This helper hardcodes
trueon key presence instead of returning the persistedbool, so it diverges from the repository’s required TTL-read pattern and bakes in an invariant that the storage value can never differ.Suggested change
pub fn is_frozen(e: &Env, account: &Address) -> bool { let key = ComplianceStorageKey::Frozen(account.clone()); - if e.storage().persistent().get::<_, bool>(&key).is_some() { + if let Some(is_frozen) = e.storage().persistent().get::<_, bool>(&key) { e.storage().persistent().extend_ttl(&key, FROZEN_TTL_THRESHOLD, FROZEN_EXTEND_AMOUNT); - true + is_frozen } else { false } }As per coding guidelines, "For library-owned persistent/temporary storage reads, extend TTL using pattern:
if let Some(value) = e.storage().persistent().get::<_, T>(&key) { e.storage().persistent().extend_ttl(&key, FOO_TTL_THRESHOLD, FOO_EXTEND_AMOUNT); value }with argument order always(&key, TTL_THRESHOLD, EXTEND_AMOUNT)"🤖 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 `@packages/tokens/src/confidential/wrapper/compliance/storage.rs` around lines 59 - 67, The is_frozen helper currently hardcodes true when the Frozen key exists; change it to read and return the persisted bool instead by using the repository TTL-read pattern: in is_frozen, call e.storage().persistent().get::<_, bool>(&ComplianceStorageKey::Frozen(account.clone())) and if let Some(value) = ... { e.storage().persistent().extend_ttl(&key, FROZEN_TTL_THRESHOLD, FROZEN_EXTEND_AMOUNT); value } else { false }, ensuring you reference ComplianceStorageKey::Frozen, FROZEN_TTL_THRESHOLD and FROZEN_EXTEND_AMOUNT exactly as in the diff.
🤖 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 `@packages/tokens/src/confidential/wrapper/compliance/storage.rs`:
- Around line 96-172: The _no_auth helpers currently perform state mutation and
emit events; change them to be pure state-mutators only: update signatures for
set_compliance_config_no_auth, freeze_no_auth, and unfreeze_no_auth to accept an
extra caller: &Address parameter (but do not perform auth or emit events),
remove the emit_compliance_config_changed / emit_frozen / emit_unfrozen calls
from these functions so they only perform storage operations and the
compliance_config check, and move the corresponding emit_* calls into the
authenticated entrypoint functions that invoke these helpers so events are
emitted by the high-level auth-bearing entrypoints.
In `@packages/tokens/src/confidential/wrapper/mod.rs`:
- Around line 552-558: Add two exported constants for instance-storage TTL
defaults: define INSTANCE_EXTEND_AMOUNT as 30 * DAY_IN_LEDGERS and
INSTANCE_TTL_THRESHOLD as INSTANCE_EXTEND_AMOUNT - DAY_IN_LEDGERS, and export
them (pub const) alongside ACCOUNT_EXTEND_AMOUNT and DELEGATION_EXTEND_AMOUNT so
integrators have canonical values; do not add any calls to
instance().extend_ttl()—just expose INSTANCE_EXTEND_AMOUNT and
INSTANCE_TTL_THRESHOLD as defaults next to DAY_IN_LEDGERS,
ACCOUNT_EXTEND_AMOUNT, ACCOUNT_TTL_THRESHOLD, DELEGATION_EXTEND_AMOUNT, and
DELEGATION_TTL_THRESHOLD.
In `@packages/tokens/src/confidential/wrapper/storage.rs`:
- Line 450: The storage layer function calling emit_register(e, account,
auditor_id) (and other emit_* calls in the same file) must stop emitting events;
remove all emit_* invocations from the *_no_auth helpers in
packages/tokens/src/confidential/wrapper/storage.rs and ensure those helpers
accept caller: &Address only as a parameter for potential event context but
perform no auth or event emission; then add corresponding emit_* calls into the
high-level trait entrypoints in packages/tokens/src/confidential/wrapper/mod.rs
so that the public trait methods perform auth and emit events after calling the
low-level _no_auth functions (apply the same change to the other occurrences
noted in the review: the helper functions around the ranges referenced so they
remain pure state/proof transitions and all event emission is moved up to the
trait implementations).
- Around line 1041-1043: The setters set_verifier (and the analogous set_auditor
at 1061-1063) currently overwrite existing values; change them to be write-once
like set_token/set_wrap by first checking
storage.instance().get(&WrapperStorageKey::Verifier) (and
WrapperStorageKey::Auditor) and rejecting the call if a value already exists
(return/panic with a clear error message) so repeated calls cannot replace the
verifier/auditor; if rotation is desired instead, implement an explicit
authenticated rotate_* flow with an event rather than allowing direct overwrite
here.
---
Nitpick comments:
In `@packages/tokens/src/confidential/wrapper/compliance/storage.rs`:
- Around line 59-67: The is_frozen helper currently hardcodes true when the
Frozen key exists; change it to read and return the persisted bool instead by
using the repository TTL-read pattern: in is_frozen, call
e.storage().persistent().get::<_,
bool>(&ComplianceStorageKey::Frozen(account.clone())) and if let Some(value) =
... { e.storage().persistent().extend_ttl(&key, FROZEN_TTL_THRESHOLD,
FROZEN_EXTEND_AMOUNT); value } else { false }, ensuring you reference
ComplianceStorageKey::Frozen, FROZEN_TTL_THRESHOLD and FROZEN_EXTEND_AMOUNT
exactly as in the diff.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36b7c738-978c-4256-88c4-f3e051806b7c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlpackages/contract-utils/src/crypto/grumpkin.rspackages/contract-utils/src/crypto/test/grumpkin.rspackages/tokens/Cargo.tomlpackages/tokens/src/confidential/mod.rspackages/tokens/src/confidential/wrapper/compliance/mod.rspackages/tokens/src/confidential/wrapper/compliance/storage.rspackages/tokens/src/confidential/wrapper/compliance/test.rspackages/tokens/src/confidential/wrapper/mod.rspackages/tokens/src/confidential/wrapper/storage.rspackages/tokens/src/confidential/wrapper/test.rs
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
Adds the confidential token wrapper — a SEP-41-backed contract with confidential balances and proof-gated transfers — together with a deployer-configurable compliance layer.
The wrapper exposes eleven entry points (register / deposit / merge / withdraw / two transfer variants / four operator-management methods / read accessors) via the
ConfidentialTokenWrappertrait. Confidential balances are stored as Pedersen commitments on Grumpkin; every state-changing op that consumes private state ships an UltraHonk proof that the wrapper verifies through a separate verifier contract, and every transfer emits dual auditor ciphertexts.A
Hookslifecycle trait runs synchronously at each entry point between auth and storage mutation — the canonical extension point for compliance gates, audit mirroring, rate limiting, etc.The new
wrapper::compliancesubmodule layers four controls (per-account freezing, pluggable external policy, SACauthorized()passthrough, unregistered-deposit carve-out) on top of those hooks. It ships a turnkeyComplianceHooks(wire astype Hooks = ComplianceHooks;) and an admin traitConfidentialCompliancefor freeze/unfreeze/config rotation.What's in this PR
packages/contract-utils/src/crypto/grumpkin.rs— adds scalar multiplication (Grumpkin::mul) on the affine API, with tests, needed by the wrapper's deposit commitment.packages/tokens/src/confidential/wrapper/— the new module:mod.rs—ConfidentialTokenWrappertrait,Hookstrait +NoHooks, all errors / events / payload types.storage.rs—*_no_authorchestration helpers, public-input assembly per DESIGN §7, cross-contract verifier/auditor calls.test.rs— wrapper-level unit tests.packages/tokens/src/confidential/wrapper/compliance/— the compliance submodule:mod.rs—Policyinterface,ComplianceError, events,ComplianceHooks,ConfidentialCompliancetrait.storage.rs—ComplianceConfig, storage keys,*_no_authadmin helpers, gating primitives (gate_account/check_policy/check_sac).test.rs— 29 tests covering the no-config short-circuit, freeze flow, policy gate, SAC passthrough, unregistered-deposit carve-out, config rotation, theConfidentialCompliancetrait via its generated client, and end-to-end hook dispatch through the wrapper.docs— module-level## Underlying Token Requirementssection documenting the exact-transfer-semantics requirement on the wrapped SEP-41 token (SAC or OZfungibleare conformant);deposit_no_auth/withdraw_no_authcarry matching# Notesblocks.Design choices worth reviewing
sac_passthrough.Policyinterface. A single registry contract can serve multiple wrappers —is_authorized(account, wrapper)receives the wrapper's own address.permit_unregistered_depositis set,on_depositskips freeze + policy on a depositor with no confidential account, but still runs SAC passthrough (since the underlying SEP-41 transfer would fail anyway if the SAC has unauthorized them).freeze/unfreeze/set_compliance_configacceptadmin: Addressand require the contract author to override — a default would either double-auth under#[only_owner]/#[only_role]or leave the entry point ungated.Frozen(account)extend TTL; writes don't (per CLAUDE.md). Compliance config sits in instance storage.Test plan
cargo test --package stellar-tokens— 607 passing (29 new in compliance).cargo +nightly fmt --all -- --check— clean.cargo clippy --release --locked --package stellar-tokens -- -D warnings— clean.cargo llvm-cov --workspace --fail-under-lines 90)ComplianceHooks::on_deposit(carve-out branch), public-input order instorage.rsagainst DESIGN §7, the "no default body" rationale in theConfidentialCompliancetrait docstringNotes for reviewers
ConfidentialVerifier::verify_proof, whose UltraHonk backend is still unaudited — module docstring already carries the "Not Production Ready" warning.token.transferand credit/debit by the delta. Happy to swap docs → runtime measurement if preferred.Summary by CodeRabbit
New Features
Tests