feat(core): contract root anchoring#134
feat(core): contract root anchoring#134Obiajulu-gif wants to merge 11 commits intoAlien-Protocol:mainfrom
Conversation
|
@Obiajulu-gif 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! 🚀 |
|
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 a CoreContract with init and submit_proof flows: persistent Merkle root and verifier storage, ZK-proof verification via a VerifierContract client, commitment deduplication, atomic root updates, contract events, new error enum, storage/type helpers, and an expanded test harness. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Core as CoreContract
participant Storage as Storage
participant Verifier as VerifierContract
participant Events as Events
Client->>Core: submit_proof(proof, public_signals)
Core->>Storage: get_verifier()
Core->>Storage: get_merkle_root()
Core->>Storage: has_commitment(public_signals.commitment)
alt verifier not set
Core-->>Client: Err(NotInitialized)
else commitment exists
Core-->>Client: Err(DuplicateCommitment)
else old_root mismatch
Core-->>Client: Err(RootMismatch)
else
Core->>Verifier: verify_proof(proof, public_signals)
alt verifier returns false
Core-->>Client: Err(InvalidProof)
else verifier returns true
Core->>Storage: store_commitment(public_signals.commitment)
Core->>Storage: set_merkle_root(public_signals.new_root)
Core->>Events: emit MerkleRootUpdated(old_root,new_root)
Core->>Events: emit UsernameRegistered(commitment)
Core-->>Client: Success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway-contract/contracts/core_contract/src/lib.rs`:
- Around line 47-67: submit_proof performs root and commitment checks before
calling out to the external verifier (VerifierContractClient::verify_proof)
which allows reentrancy and also doesn't map verifier errors; fix by either
adding a reentrancy guard around the verify_proof call or by re-reading and
re-checking current_merkle_root(&env) and storage::has_commitment(&env,
&public_signals.commitment) immediately after verify_proof returns, and ensure
the verifier invocation uses the fallible/try-style API (map the Result/err to a
ContractError like ContractError::VerifierError instead of letting raw panics
propagate). Update relevant symbols: submit_proof,
VerifierContractClient::verify_proof, storage::get_verifier,
current_merkle_root, storage::has_commitment, storage::store_commitment,
update_merkle_root.
- Around line 38-45: The init() entrypoint must require authorization before
mutating state: call env.require_auth(...) at the start of init to restrict who
can initialize (either require_auth(&env, &verifier) if the verifier is the
trusted deployer, or add an explicit deployer Address parameter and call
env.require_auth(&deployer)); keep the existing storage::is_initialized check
and ContractError::AlreadyInitialized panic, and only call storage::set_verifier
and storage::set_merkle_root after the require_auth check succeeds.
In `@gateway-contract/contracts/core_contract/src/storage.rs`:
- Around line 11-13: The current is_initialized function uses a logical AND so
it returns false if either DataKey::CurrentMerkleRoot or DataKey::Verifier is
missing, which allows init() to run when one of the two is present — change the
check in is_initialized (function name: is_initialized) to treat the presence of
either key as “already initialized” (use logical OR between
env.storage().persistent().has(&DataKey::CurrentMerkleRoot) and
env.storage().instance().has(&DataKey::Verifier)) so init() will fail-closed;
additionally consider moving the Verifier entry to persistent storage (make
DataKey::Verifier stored via env.storage().persistent()) so both items share
durability to avoid asymmetric deletion risks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 495b0612-1475-4948-a0bf-1748e1c9f050
📒 Files selected for processing (6)
gateway-contract/contracts/core_contract/src/errors.rsgateway-contract/contracts/core_contract/src/events.rsgateway-contract/contracts/core_contract/src/lib.rsgateway-contract/contracts/core_contract/src/storage.rsgateway-contract/contracts/core_contract/src/test.rsgateway-contract/contracts/core_contract/src/types.rs
| pub fn init(env: Env, verifier: Address, root: BytesN<32>) { | ||
| if storage::is_initialized(&env) { | ||
| panic_with_error!(&env, ContractError::AlreadyInitialized); | ||
| } | ||
|
|
||
| env.storage().persistent().set(&key, &data); | ||
| storage::set_verifier(&env, &verifier); | ||
| storage::set_merkle_root(&env, &root); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd gateway-contract && find . -type f -name "*.rs" | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 896
🏁 Script executed:
cd gateway-contract && rg "fn init" --type rust -B 2 -A 15Repository: Alien-Protocol/Alien-Gateway
Length of output: 2467
🏁 Script executed:
cd gateway-contract && rg "invoke_contract_check_host_approval|__check_auth" --type rustRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
cd gateway-contract && git log --oneline --all | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 1611
🏁 Script executed:
cd gateway-contract && find . -type f \( -name "*.toml" -o -name "*.md" \) | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 404
🏁 Script executed:
cd gateway-contract && sed -n '1,100p' contracts/core_contract/src/test.rs | grep -A 20 "fn setup"Repository: Alien-Protocol/Alien-Gateway
Length of output: 766
🏁 Script executed:
cd gateway-contract && rg "fn setup" contracts/core_contract/src/test.rs -A 25Repository: Alien-Protocol/Alien-Gateway
Length of output: 884
🏁 Script executed:
cd gateway-contract && cat -n contracts/core_contract/src/storage.rs | head -80Repository: Alien-Protocol/Alien-Gateway
Length of output: 1558
🏁 Script executed:
cd gateway-contract && rg "is_initialized\|initialization" contracts/core_contract/src/ -B 2 -A 3Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
cd gateway-contract && cat contracts/core_contract/Cargo.toml | grep -A 10 "soroban"Repository: Alien-Protocol/Alien-Gateway
Length of output: 187
🏁 Script executed:
cd gateway-contract && rg "contractimpl|contract_fn|soroban_sdk" contracts/core_contract/src/lib.rs | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 186
🏁 Script executed:
cd gateway-contract && find . -type f -name "*.rs" -exec grep -l "deploy\|register" {} \; | head -10Repository: Alien-Protocol/Alien-Gateway
Length of output: 195
🏁 Script executed:
cd gateway-contract && cat -n contracts/core_contract/src/lib.rs | sed -n '1,70p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 2839
🏁 Script executed:
cd gateway-contract && rg "require_auth\|require\|auth" contracts/core_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
cd gateway-contract && find . -type f -name "*.rs" -path "*/factory*" -o -name "*deploy*" | xargs cat 2>/dev/null | head -100Repository: Alien-Protocol/Alien-Gateway
Length of output: 207
🏁 Script executed:
cd gateway-contract && cat contracts/core_contract/core.md 2>/dev/null | head -150Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
cd gateway-contract && cat readme.md 2>/dev/null | grep -A 20 -i "init\|deploy\|setup"Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
cd gateway-contract && rg "fn init" contracts/ -B 5 -A 20 --type rust | grep -E "(init|pub|auth|require)" | head -30Repository: Alien-Protocol/Alien-Gateway
Length of output: 1143
🏁 Script executed:
cd gateway-contract && web_search "Soroban public contract function authorization require_auth"Repository: Alien-Protocol/Alien-Gateway
Length of output: 122
🏁 Script executed:
cd gateway-contract && find . -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l "init\|deploy" {} \;Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🌐 Web query:
Soroban contract require_auth authorization public functions
💡 Result:
In Soroban, any exported contract function (often called “public”) is callable by anyone unless your code explicitly enforces authorization. Soroban does not auto-restrict access based on “public/private”; you enforce permissions by calling require_auth (or require_auth_for_args) on the Address that must approve the action. [1][9]
How require_auth works (what it guarantees)
some_address.require_auth()tells the Soroban host: “this invocation must be authorized bysome_address”.- The host then handles signature verification + replay prevention (you don’t manually verify signatures in the contract). [9]
- If
some_addressis a contract address (account abstraction), the host calls that contract’s__check_authhook to validate the authorization. [1]
Pattern for “public” functions that must be protected
Call require_auth() at the start of the function for the party whose rights are being exercised:
pub fn withdraw(env: Env, from: Address, amount: i128) {
from.require_auth(); // must be authorized by `from`
// ... mutate balances / transfer tokens ...
}This is the core pattern used in Soroban auth examples. [2]
require_auth_for_args (more specific authorization)
Use require_auth_for_args(...) when you want the signature to be bound to specific arguments / intent, which enables safer delegation patterns (e.g., delegated signers / smart accounts authorizing particular calls). [3]
Cross-contract calls and “sub-invocation” authorization
If your contract calls another contract that does require_auth, Soroban’s auth framework supports authorizing those sub-contract calls as part of the same authorized invocation tree (you generally don’t need special wiring beyond using require_auth). [1]
Practical “when do I require auth?” rule of thumb
- Read-only access to a user’s data: usually no
require_authneeded. - Mutating a user’s state in a way that isn’t strictly beneficial (e.g., decreasing balance / transferring out): you should
require_auth. [1]
Sources: Stellar authorization docs and examples. [1][2][3][9]
🏁 Script executed:
cd gateway-contract && cat -n contracts/core_contract/src/lib.rs | sed -n '30,80p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 2224
🏁 Script executed:
cd gateway-contract && rg "TODO\|FIXME\|XXX\|HACK\|NOTE" contracts/core_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
cd gateway-contract && git log --all --oneline -- contracts/core_contract/src/lib.rs | head -10Repository: Alien-Protocol/Alien-Gateway
Length of output: 474
🏁 Script executed:
cd gateway-contract && git show d864384 | grep -A 5 -B 5 "init\|auth" | head -40Repository: Alien-Protocol/Alien-Gateway
Length of output: 2151
🏁 Script executed:
cd gateway-contract && git show d864384 | grep -A 50 "init\|Entry point" | head -80Repository: Alien-Protocol/Alien-Gateway
Length of output: 4103
🏁 Script executed:
cd gateway-contract && git show d864384 | grep -B 5 -A 5 "^| \`init\`"Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
cd gateway-contract && git show d864384 | grep -C 10 "core_contract" | grep -A 30 "Entry point"Repository: Alien-Protocol/Alien-Gateway
Length of output: 2951
🏁 Script executed:
cd gateway-contract && git show d864384:contracts/core_contract/src/lib.rs 2>/dev/null | head -100Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
cd gateway-contract && rg "fn init\|pub fn " contracts/core_contract/src/lib.rs | head -15Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
cd gateway-contract && git log --all --oneline --diff-filter=A -- contracts/core_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 183
🏁 Script executed:
cd gateway-contract && wc -l contracts/core_contract/src/lib.rs && grep -c "pub fn" contracts/core_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 111
🏁 Script executed:
cd gateway-contract && cat -n contracts/core_contract/src/lib.rs | grep "pub fn"Repository: Alien-Protocol/Alien-Gateway
Length of output: 430
Add require_auth() to gate initialization to a trusted deployer.
This entrypoint lacks authorization checks, so any caller can invoke init() first and permanently set both the verifier and initial merkle root. If deployment and initialization are separate transactions, a frontrunner can anchor arbitrary state before the intended owner. Call require_auth() on a trusted deployer address, or ensure initialization is atomic with contract deployment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/core_contract/src/lib.rs` around lines 38 - 45,
The init() entrypoint must require authorization before mutating state: call
env.require_auth(...) at the start of init to restrict who can initialize
(either require_auth(&env, &verifier) if the verifier is the trusted deployer,
or add an explicit deployer Address parameter and call
env.require_auth(&deployer)); keep the existing storage::is_initialized check
and ContractError::AlreadyInitialized panic, and only call storage::set_verifier
and storage::set_merkle_root after the require_auth check succeeds.
| pub fn submit_proof(env: Env, proof: Proof, public_signals: PublicSignals) { | ||
| let current_root = current_merkle_root(&env); | ||
|
|
||
| match env.storage().persistent().get::<DataKey, ResolveData>(&key) { | ||
| Some(data) => data, | ||
| None => panic_with_error!(&env, ResolverError::NotFound), | ||
| if current_root != public_signals.old_root.clone() { | ||
| panic_with_error!(&env, ContractError::RootMismatch); | ||
| } | ||
|
|
||
| if storage::has_commitment(&env, &public_signals.commitment) { | ||
| panic_with_error!(&env, ContractError::DuplicateCommitment); | ||
| } | ||
|
|
||
| let verifier = storage::get_verifier(&env) | ||
| .unwrap_or_else(|| panic_with_error!(&env, ContractError::NotInitialized)); | ||
| let verifier_client = VerifierContractClient::new(&env, &verifier); | ||
| let is_valid = verifier_client.verify_proof(&proof, &public_signals); | ||
| if !is_valid { | ||
| panic_with_error!(&env, ContractError::InvalidProof); | ||
| } | ||
|
|
||
| storage::store_commitment(&env, &public_signals.commitment); | ||
| update_merkle_root(&env, current_root, public_signals.new_root.clone()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure
find . -type f -name "*.rs" | grep -E "(lib|contract)" | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 1236
🏁 Script executed:
# Check the exact file and lines mentioned
if [ -f "gateway-contract/contracts/core_contract/src/lib.rs" ]; then
wc -l gateway-contract/contracts/core_contract/src/lib.rs
fiRepository: Alien-Protocol/Alien-Gateway
Length of output: 126
🏁 Script executed:
# Read the file around the specified lines
if [ -f "gateway-contract/contracts/core_contract/src/lib.rs" ]; then
head -80 gateway-contract/contracts/core_contract/src/lib.rs | tail -40
fiRepository: Alien-Protocol/Alien-Gateway
Length of output: 1463
🏁 Script executed:
# Search for reentrancy guard implementations in the codebase
rg "reentrancy|Reentrancy|re_entrant|ReEntrant" --type rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 98
🏁 Script executed:
# Look for any mutex or lock patterns that might be reentrancy guards
rg "Mutex|RwLock|atomic" gateway-contract/contracts/core_contract/src/ --type rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 98
🏁 Script executed:
# Check error handling in verifier calls - search for try_ patterns
rg "try_invoke|try_call|\.try_" gateway-contract/contracts/core_contract/src/ --type rs -A 3Repository: Alien-Protocol/Alien-Gateway
Length of output: 98
🏁 Script executed:
# Search for reentrancy patterns without type restriction
rg "reentrancy|Reentrancy|REENTRANCY" gateway-contract/Repository: Alien-Protocol/Alien-Gateway
Length of output: 206
🏁 Script executed:
# Look at the full submit_proof function and surrounding context
cat -n gateway-contract/contracts/core_contract/src/lib.rs | sed -n '40,75p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 1655
🏁 Script executed:
# Search for VerifierContractClient definition and available methods
rg "VerifierContractClient|verify_proof" gateway-contract/ -A 5 -B 2Repository: Alien-Protocol/Alien-Gateway
Length of output: 4213
🏁 Script executed:
# Check if there's a try_verify_proof method
rg "try_verify_proof|try_" gateway-contract/contracts/core_contract/src/lib.rs -B 2 -A 2Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Search for reentrancy guard patterns more broadly
rg "Mutex|RwLock|guard|locked|re_enter" gateway-contract/contracts/core_contract/ -iRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Check the storage module for any reentrancy guards on state
cat -n gateway-contract/contracts/core_contract/src/storage.rs | head -50Repository: Alien-Protocol/Alien-Gateway
Length of output: 1558
🏁 Script executed:
# Look for any comments or patterns related to reentrancy in the entire core_contract
grep -r "reenter\|re-enter\|reentr" gateway-contract/contracts/core_contract/ -iRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Check how panic_with_error is defined
rg "macro_rules! panic_with_error|fn panic_with_error" gateway-contract/ -A 5Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Look at the error types
cat -n gateway-contract/contracts/core_contract/src/errors.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 395
🏁 Script executed:
# Check if there are try_ methods in the contract
rg "try_submit_proof|pub fn" gateway-contract/contracts/core_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 395
🏁 Script executed:
# Look for try_ invocation patterns in Soroban
rg "try_invoke|try_call" gateway-contract/ -B 2 -A 2Repository: Alien-Protocol/Alien-Gateway
Length of output: 515
🏁 Script executed:
# Check the full context of how verifier_client is used - look for all invocations
rg "verifier_client\." gateway-contract/contracts/core_contract/src/ -B 1 -A 1Repository: Alien-Protocol/Alien-Gateway
Length of output: 980
🏁 Script executed:
# Let's check if there's a try_submit_proof method or any try_ variant methods
cat -n gateway-contract/contracts/core_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 3313
🏁 Script executed:
# Search for how other contracts handle verifier calls to see if there's a pattern
rg "verify_proof|verifier_client" gateway-contract/ -B 3 -A 3Repository: Alien-Protocol/Alien-Gateway
Length of output: 3686
🏁 Script executed:
# Check if there are any contract test cases that demonstrate reentrancy behavior
cat -n gateway-contract/contracts/core_contract/src/test.rs | grep -A 20 "reenter\|reentr" -iRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Look for the complete lib.rs to see all public functions and their signatures
cat -n gateway-contract/contracts/core_contract/src/lib.rs | grep -E "pub fn|pub trait"Repository: Alien-Protocol/Alien-Gateway
Length of output: 466
🏁 Script executed:
# Check if there are any reentrancy guard patterns in other contracts
cat gateway-contract/contracts/escrow_contract/src/lib.rs | head -100Repository: Alien-Protocol/Alien-Gateway
Length of output: 3159
🏁 Script executed:
# Let's check the test to understand the behavior better
cat -n gateway-contract/contracts/core_contract/src/test.rs | head -100Repository: Alien-Protocol/Alien-Gateway
Length of output: 3389
🏁 Script executed:
# Search for any context/mutex/lock patterns in the Soroban SDK usage
rg "env\.|storage\(\)" gateway-contract/contracts/core_contract/src/lib.rs | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
Add reentrancy protection or re-validate state after the verifier call.
The root and duplicate-commitment checks (lines 50–56) execute before the external verifier call (line 61), then their results are trusted without re-validation. A malicious verifier that reenters submit_proof can invalidate those checks. Additionally, the direct verify_proof() invocation on line 61 does not use try_ error handling, so verifier panics or errors will leak raw invoke errors instead of mapping to ContractError.
Either (1) re-read current_root and recheck has_commitment after the verifier returns, or (2) add a reentrancy guard to prevent reentry during the verification call. Explicitly map any verifier invocation errors to ContractError variants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/core_contract/src/lib.rs` around lines 47 - 67,
submit_proof performs root and commitment checks before calling out to the
external verifier (VerifierContractClient::verify_proof) which allows reentrancy
and also doesn't map verifier errors; fix by either adding a reentrancy guard
around the verify_proof call or by re-reading and re-checking
current_merkle_root(&env) and storage::has_commitment(&env,
&public_signals.commitment) immediately after verify_proof returns, and ensure
the verifier invocation uses the fallible/try-style API (map the Result/err to a
ContractError like ContractError::VerifierError instead of letting raw panics
propagate). Update relevant symbols: submit_proof,
VerifierContractClient::verify_proof, storage::get_verifier,
current_merkle_root, storage::has_commitment, storage::store_commitment,
update_merkle_root.
| pub fn is_initialized(env: &Env) -> bool { | ||
| env.storage().persistent().has(&DataKey::CurrentMerkleRoot) | ||
| && env.storage().instance().has(&DataKey::Verifier) |
There was a problem hiding this comment.
Fail closed on partial initialization state.
is_initialized() currently returns false unless both keys exist, but only one missing key is enough to make a second init() dangerous. If the verifier entry ever disappears while the persistent root remains, init() can overwrite the anchored root outside the proof flow. At minimum, treat either key as “already initialized”; ideally keep the verifier in the same durability class as the root.
🛠️ Minimal fail-closed fix
pub fn is_initialized(env: &Env) -> bool {
env.storage().persistent().has(&DataKey::CurrentMerkleRoot)
- && env.storage().instance().has(&DataKey::Verifier)
+ || env.storage().instance().has(&DataKey::Verifier)
}Also applies to: 26-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/core_contract/src/storage.rs` around lines 11 -
13, The current is_initialized function uses a logical AND so it returns false
if either DataKey::CurrentMerkleRoot or DataKey::Verifier is missing, which
allows init() to run when one of the two is present — change the check in
is_initialized (function name: is_initialized) to treat the presence of either
key as “already initialized” (use logical OR between
env.storage().persistent().has(&DataKey::CurrentMerkleRoot) and
env.storage().instance().has(&DataKey::Verifier)) so init() will fail-closed;
additionally consider moving the Verifier entry to persistent storage (make
DataKey::Verifier stored via env.storage().persistent()) so both items share
durability to avoid asymmetric deletion risks.
Obiajulu-gif
force-pushed
the
feat/contract-root-anchoring
branch
from
675bb76 to
1583021
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
gateway-contract/contracts/core_contract/src/storage.rs (1)
26-32:⚠️ Potential issue | 🟠 MajorVerifier uses instance storage while root uses persistent storage.
Instance storage and persistent storage have different archival/TTL behaviors. If the contract instance is archived and restored, or if TTLs differ, the verifier entry could be lost while the Merkle root remains—breaking
is_initialized()checks and potentially allowinginit()to be called again. Consider using persistent storage for both to ensure consistent durability.🛠️ Proposed fix to use persistent storage for verifier
pub fn get_verifier(env: &Env) -> Option<Address> { - env.storage().instance().get(&DataKey::Verifier) + env.storage().persistent().get(&DataKey::Verifier) } pub fn set_verifier(env: &Env, verifier: &Address) { - env.storage().instance().set(&DataKey::Verifier, verifier); + env.storage().persistent().set(&DataKey::Verifier, verifier); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/storage.rs` around lines 26 - 32, The verifier is stored in instance storage while the Merkle root uses persistent storage, causing durability mismatch; update get_verifier and set_verifier to use persistent storage (replace env.storage().instance() with env.storage().persistent()) when accessing DataKey::Verifier so the verifier entry shares the same persistence/TTL as the Merkle root and cannot be lost across archiving/restoration.
🧹 Nitpick comments (1)
gateway-contract/contracts/core_contract/src/storage.rs (1)
34-44: Consider extending TTL for persistent commitment entries.Persistent storage entries can expire if their TTL is not extended. For long-lived commitment tracking, you may want to call
extend_ttl()when storing or accessing commitments to prevent expiration of critical state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/storage.rs` around lines 34 - 44, When storing or checking commitments make sure to extend the persistent entry TTL so commitments don't expire: in store_commitment (and in has_commitment when a commitment is found) call the persistent storage's extend_ttl for the same DataKey::Commitment(commitment.clone()) after setting/reading it; add a clear TTL constant (e.g. COMMITMENT_TTL) and use that value for extend_ttl so the lifetime is explicit and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway-contract/contracts/core_contract/src/lib.rs`:
- Around line 16-20: Remove the duplicate module and import declarations that
cause E0428/E0252: delete the extra `pub mod events;` (duplicate of `mod
events;`) and remove the redundant `use soroban_sdk::{contract, contracterror,
contractimpl, contracttype, panic_with_error, Address, BytesN, Env};` duplicate
import so only the intended single `mod events;` and one `use
soroban_sdk::{...}` remain in the file; ensure no other duplicate `mod events`
or identical `use soroban_sdk` blocks remain.
---
Duplicate comments:
In `@gateway-contract/contracts/core_contract/src/storage.rs`:
- Around line 26-32: The verifier is stored in instance storage while the Merkle
root uses persistent storage, causing durability mismatch; update get_verifier
and set_verifier to use persistent storage (replace env.storage().instance()
with env.storage().persistent()) when accessing DataKey::Verifier so the
verifier entry shares the same persistence/TTL as the Merkle root and cannot be
lost across archiving/restoration.
---
Nitpick comments:
In `@gateway-contract/contracts/core_contract/src/storage.rs`:
- Around line 34-44: When storing or checking commitments make sure to extend
the persistent entry TTL so commitments don't expire: in store_commitment (and
in has_commitment when a commitment is found) call the persistent storage's
extend_ttl for the same DataKey::Commitment(commitment.clone()) after
setting/reading it; add a clear TTL constant (e.g. COMMITMENT_TTL) and use that
value for extend_ttl so the lifetime is explicit and maintainable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f99da82-83a3-40b1-a936-bcbc850148f0
📒 Files selected for processing (6)
gateway-contract/contracts/core_contract/src/errors.rsgateway-contract/contracts/core_contract/src/events.rsgateway-contract/contracts/core_contract/src/lib.rsgateway-contract/contracts/core_contract/src/storage.rsgateway-contract/contracts/core_contract/src/test.rsgateway-contract/contracts/core_contract/src/types.rs
✅ Files skipped from review due to trivial changes (1)
- gateway-contract/contracts/core_contract/src/errors.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- gateway-contract/contracts/core_contract/src/events.rs
- gateway-contract/contracts/core_contract/src/types.rs
- gateway-contract/contracts/core_contract/src/test.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gateway-contract/contracts/core_contract/src/lib.rs (2)
117-118:⚠️ Potential issue | 🔴 CriticalRemove duplicate test module declaration.
The test module is already declared at lines 8-9. This duplicate declaration at lines 117-118 will cause compilation error E0428 (module defined multiple times).
🐛 Proposed fix
} } - -#[cfg(test)] -mod test;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/lib.rs` around lines 117 - 118, There is a duplicate test module declaration `mod test;` in lib.rs causing duplicate definition error E0428; remove the redundant `mod test;` at the later location (the one around lines 117-118) so only the original `#[cfg(test)] mod test;` declaration remains (the earlier declaration at lines 8-9), ensuring the `test` module is defined only once.
80-102:⚠️ Potential issue | 🔴 CriticalCritical: Remove old resolver functions and fix
UsernameRegisteredstruct initialization.The
submit_prooffunction is broken by old code inserted between theUsernameRegisteredstruct initialization (lines 80-81) and its.publish(&env)call (line 101). Lines 82-100 contain oldset_memoandresolvefunctions that must be removed.🐛 Proposed fix to complete struct and remove old code
UsernameRegistered { commitment: public_signals.commitment, - pub fn set_memo(env: Env, commitment: BytesN<32>, memo_id: u64) { - let key = DataKey::Resolver(commitment); - let mut data = env - .storage() - .persistent() - .get::<DataKey, ResolveData>(&key) - .unwrap_or_else(|| panic_with_error!(&env, ResolverError::NotFound)); - - data.memo = Some(memo_id); - env.storage().persistent().set(&key, &data); - } - - pub fn resolve(env: Env, commitment: BytesN<32>) -> (Address, Option<u64>) { - let key = DataKey::Resolver(commitment); - - match env.storage().persistent().get::<DataKey, ResolveData>(&key) { - Some(data) => (data.wallet, data.memo), - None => panic_with_error!(&env, ResolverError::NotFound), - } + } .publish(&env); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/lib.rs` around lines 80 - 102, The UsernameRegistered struct initialization is being interrupted by two stale functions (set_memo and resolve) which were accidentally inserted between the struct literal that starts with UsernameRegistered { commitment: public_signals.commitment, and its .publish(&env) call; remove the entire set_memo and resolve function blocks (references: set_memo, resolve, DataKey::Resolver, ResolveData) so the struct literal is completed properly and the subsequent .publish(&env) is called on the UsernameRegistered instance, ensuring submit_proof compiles and behaves as intended.
♻️ Duplicate comments (3)
gateway-contract/contracts/core_contract/src/lib.rs (3)
49-56:⚠️ Potential issue | 🟠 MajorAdd
require_auth()to gate initialization to a trusted deployer.The
init()function lacks authorization checks, allowing any caller to invoke it first and permanently set both the verifier and initial Merkle root. If deployment and initialization are separate transactions, a frontrunner can anchor arbitrary state before the intended owner.Consider adding a deployer/admin parameter and requiring authorization:
pub fn init(env: Env, admin: Address, verifier: Address, root: BytesN<32>) { admin.require_auth(); // ... rest of initialization }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/lib.rs` around lines 49 - 56, The init function (init) currently has no authorization and can be called by anyone; update its signature to accept an admin/deployer Address, call admin.require_auth() at the start of init, and persist that admin (e.g., storage::set_admin) alongside verifier and merkle root so future protected ops can check authorized caller; ensure the existing storage helpers (storage::set_verifier, storage::set_merkle_root) remain but add storage::set_admin (and corresponding storage::is_admin or require_admin checks elsewhere) and return/handle ContractError::AlreadyInitialized as before.
16-22:⚠️ Potential issue | 🔴 CriticalCritical: Remove duplicate module declarations and imports blocking compilation.
Lines 16-22 contain old code that conflicts with the new declarations on lines 3-15:
pub mod events;andpub mod types;duplicate modules already declared at lines 4 and 6- The
use soroban_sdk::{...}block duplicates the import on line 11use types::ResolveData;references a type that no longer exists in the new implementationThese cause E0428 (duplicate module) and E0252 (duplicate import) compilation errors.
🐛 Proposed fix to remove duplicate definitions
pub use crate::types::{Proof, PublicSignals}; -pub mod events; -pub mod types; - -use soroban_sdk::{ - contract, contracterror, contractimpl, contracttype, panic_with_error, Address, BytesN, Env, -}; -use types::ResolveData; #[contract] pub struct CoreContract;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/lib.rs` around lines 16 - 22, Remove the duplicate module and import declarations that cause compilation errors: delete the redundant "pub mod events;" and "pub mod types;" lines that duplicate earlier declarations, remove the duplicated "use soroban_sdk::{contract, contracterror, contractimpl, contracttype, panic_with_error, Address, BytesN, Env};" import block, and remove the obsolete "use types::ResolveData;" reference (or replace it with the correct type/import used by the new implementation) so only the single, intended module and soroban_sdk imports remain.
69-78:⚠️ Potential issue | 🟠 MajorConsider reentrancy protection for external verifier call.
The root and duplicate-commitment checks (lines 61-67) execute before the external verifier call (line 72), then state is mutated after. A malicious verifier that reenters
submit_proofcould potentially bypass these checks since state updates haven't occurred yet.Consider either:
- Adding a reentrancy guard (lock flag in storage)
- Storing the commitment before the external call (checks-effects-interactions pattern)
- Re-validating state after the verifier returns
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/lib.rs` around lines 69 - 78, The external verifier call via VerifierContractClient::verify_proof is invoked before state changes, which permits reentrancy; fix by applying one of the suggested patterns: (A) implement a reentrancy guard in storage (e.g., a boolean lock flag set at start of submit_proof and cleared at the end) and check it in entry (update storage::get_verifier usage to set/clear the lock around the external call), or (B) follow checks-effects-interactions by calling storage::store_commitment and update_merkle_root before invoking VerifierContractClient::verify_proof, or (C) re-validate the critical invariants (current_root and duplicate-commitment checks) immediately after verify_proof returns before mutating state; reference the symbols storage::store_commitment, update_merkle_root, VerifierContractClient::verify_proof, and storage::get_verifier when applying the change.
🧹 Nitpick comments (1)
gateway-contract/contracts/core_contract/src/lib.rs (1)
104-110: Consider consistent error handling for getter functions.
get_merkle_rootpanics withContractError::NotInitializedwhen uninitialized, whileget_verifierreturnsOption<Address>. This inconsistency could confuse callers expecting uniform behavior. Consider either having both returnOptionor both panic when uninitialized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/lib.rs` around lines 104 - 110, The two getters are inconsistent: get_merkle_root currently panics via current_merkle_root (raising ContractError::NotInitialized) while get_verifier returns Option; make their behavior consistent by choosing one approach — e.g., have get_verifier also panic when uninitialized: change its signature to return Address and replace storage::get_verifier(&env) with a call that unwraps/expect() to raise ContractError::NotInitialized (or convert the Option into a Result and propagate the same ContractError), so both get_merkle_root (which calls current_merkle_root) and get_verifier exhibit the same error behavior; alternatively, if you prefer optional returns, change get_merkle_root to return Option<BytesN<32>> and delegate to a storage accessor that returns Option instead of current_merkle_root.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway-contract/contracts/core_contract/src/lib.rs`:
- Around line 37-45: The function update_merkle_root is missing its closing
brace and an old ResolverError enum was accidentally merged into its body; close
the update_merkle_root function after the MerkleRootUpdated.publish(env) call
(add the missing '}') and remove the outdated ResolverError enum block (the pub
enum ResolverError { ... }) so the function contains only the call to
storage::set_merkle_root and MerkleRootUpdated::publish; confirm symbols
referenced are update_merkle_root, storage::set_merkle_root, and
MerkleRootUpdated.
---
Outside diff comments:
In `@gateway-contract/contracts/core_contract/src/lib.rs`:
- Around line 117-118: There is a duplicate test module declaration `mod test;`
in lib.rs causing duplicate definition error E0428; remove the redundant `mod
test;` at the later location (the one around lines 117-118) so only the original
`#[cfg(test)] mod test;` declaration remains (the earlier declaration at lines
8-9), ensuring the `test` module is defined only once.
- Around line 80-102: The UsernameRegistered struct initialization is being
interrupted by two stale functions (set_memo and resolve) which were
accidentally inserted between the struct literal that starts with
UsernameRegistered { commitment: public_signals.commitment, and its
.publish(&env) call; remove the entire set_memo and resolve function blocks
(references: set_memo, resolve, DataKey::Resolver, ResolveData) so the struct
literal is completed properly and the subsequent .publish(&env) is called on the
UsernameRegistered instance, ensuring submit_proof compiles and behaves as
intended.
---
Duplicate comments:
In `@gateway-contract/contracts/core_contract/src/lib.rs`:
- Around line 49-56: The init function (init) currently has no authorization and
can be called by anyone; update its signature to accept an admin/deployer
Address, call admin.require_auth() at the start of init, and persist that admin
(e.g., storage::set_admin) alongside verifier and merkle root so future
protected ops can check authorized caller; ensure the existing storage helpers
(storage::set_verifier, storage::set_merkle_root) remain but add
storage::set_admin (and corresponding storage::is_admin or require_admin checks
elsewhere) and return/handle ContractError::AlreadyInitialized as before.
- Around line 16-22: Remove the duplicate module and import declarations that
cause compilation errors: delete the redundant "pub mod events;" and "pub mod
types;" lines that duplicate earlier declarations, remove the duplicated "use
soroban_sdk::{contract, contracterror, contractimpl, contracttype,
panic_with_error, Address, BytesN, Env};" import block, and remove the obsolete
"use types::ResolveData;" reference (or replace it with the correct type/import
used by the new implementation) so only the single, intended module and
soroban_sdk imports remain.
- Around line 69-78: The external verifier call via
VerifierContractClient::verify_proof is invoked before state changes, which
permits reentrancy; fix by applying one of the suggested patterns: (A) implement
a reentrancy guard in storage (e.g., a boolean lock flag set at start of
submit_proof and cleared at the end) and check it in entry (update
storage::get_verifier usage to set/clear the lock around the external call), or
(B) follow checks-effects-interactions by calling storage::store_commitment and
update_merkle_root before invoking VerifierContractClient::verify_proof, or (C)
re-validate the critical invariants (current_root and duplicate-commitment
checks) immediately after verify_proof returns before mutating state; reference
the symbols storage::store_commitment, update_merkle_root,
VerifierContractClient::verify_proof, and storage::get_verifier when applying
the change.
---
Nitpick comments:
In `@gateway-contract/contracts/core_contract/src/lib.rs`:
- Around line 104-110: The two getters are inconsistent: get_merkle_root
currently panics via current_merkle_root (raising ContractError::NotInitialized)
while get_verifier returns Option; make their behavior consistent by choosing
one approach — e.g., have get_verifier also panic when uninitialized: change its
signature to return Address and replace storage::get_verifier(&env) with a call
that unwraps/expect() to raise ContractError::NotInitialized (or convert the
Option into a Result and propagate the same ContractError), so both
get_merkle_root (which calls current_merkle_root) and get_verifier exhibit the
same error behavior; alternatively, if you prefer optional returns, change
get_merkle_root to return Option<BytesN<32>> and delegate to a storage accessor
that returns Option instead of current_merkle_root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 806ed1ff-9765-4c89-98a6-269b8b697adb
📒 Files selected for processing (3)
gateway-contract/contracts/core_contract/src/lib.rsgateway-contract/contracts/core_contract/src/test.rsgateway-contract/contracts/core_contract/src/types.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- gateway-contract/contracts/core_contract/src/types.rs
- gateway-contract/contracts/core_contract/src/test.rs
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
gateway-contract/contracts/core_contract/src/lib.rs (2)
3-14:⚠️ Potential issue | 🔴 CriticalDuplicate module declarations cause compilation errors E0428.
Private module declarations (lines 3-6) conflict with public module declarations (lines 7-14):
mod errorsvspub mod errorsmod eventsvspub mod eventsmod storagevspub mod storagemod typesvspub mod typesThis is merge conflict remnants. Decide on visibility (likely
pub modfor external access) and remove duplicates.🐛 Proposed fix to remove duplicate module declarations
#![no_std] -mod errors; -mod events; -mod storage; -mod types; pub mod address_manager; pub mod errors; pub mod events; pub mod registration; pub mod smt_root; pub mod storage; pub mod types; pub mod zk_verifier;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/lib.rs` around lines 3 - 14, The file contains duplicate private and public module declarations (mod errors/events/storage/types vs pub mod errors/events/storage/types) causing E0428; remove the private declarations and keep the public ones (retain pub mod address_manager, errors, events, registration, smt_root, storage, types, zk_verifier) so each module is declared only once and exported for external use.
19-29:⚠️ Potential issue | 🔴 CriticalDuplicate imports cause compilation errors E0252.
Line 19 and lines 28-29 both import from
soroban_sdkwith overlapping items. Thetypesmodule is also imported twice with different items.🐛 Proposed fix to consolidate imports
-use soroban_sdk::{contract, contractclient, contractimpl, panic_with_error, Address, BytesN, Env}; - pub use crate::errors::ContractError; pub use crate::events::{MerkleRootUpdated, UsernameRegistered}; pub use crate::types::{Proof, PublicSignals}; use address_manager::AddressManager; use errors::CoreError; use events::REGISTER_EVENT; use registration::Registration; -use soroban_sdk::{contract, contractimpl, panic_with_error, Address, Bytes, BytesN, Env}; -use types::{ChainType, PublicSignals, ResolveData}; +use soroban_sdk::{contract, contractclient, contractimpl, panic_with_error, Address, Bytes, BytesN, Env}; +use types::{ChainType, Proof, PublicSignals, ResolveData};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/lib.rs` around lines 19 - 29, There are duplicate imports from soroban_sdk and the types module causing E0252; consolidate them into a single use declaration (e.g., merge the two soroban_sdk lines into one that includes contract, contractclient (if needed), contractimpl, panic_with_error, Address, Bytes, BytesN, Env) and remove the redundant import; likewise unify the types imports so PublicSignals/Proof/ResolveData/ChainType come from the correct module (crate::types or types) in one use statement and remove the other conflicting use. Update the top-level imports (the duplicate soroban_sdk lines and duplicate types imports) so each symbol appears only once.gateway-contract/contracts/core_contract/src/test.rs (3)
263-291:⚠️ Potential issue | 🔴 CriticalTest
direct_root_override_entrypoint_is_rejectedhas merge conflict.Lines 263-291 show merge conflict remnants where the new test body (lines 263-274) is incorrectly merged with old test code (lines 275-291).
🐛 Proposed fix
#[test] fn direct_root_override_entrypoint_is_rejected() { let env = Env::default(); let (contract_id, client, _) = setup(&env); let result = env.try_invoke_contract::<(), InvokeError>( &contract_id, &Symbol::new(&env, "set_merkle_root"), vec![&env, bytes(&env, 9).into_val(&env)], ); assert_eq!(result, Err(Ok(InvokeError::Abort))); assert_eq!(client.get_merkle_root(), bytes(&env, 0)); - env.mock_all_auths(); - let (_, client, root) = setup_with_root(&env); - let caller = Address::generate(&env); - let hash = commitment(&env, 0); - let new_root = BytesN::from_array(&env, &[2u8; 32]); - - let signals = PublicSignals { - old_root: root, - new_root, - }; - client.register_resolver(&caller, &hash, &dummy_proof(&env), &signals); - client.set_memo(&hash, &4242u64); - - let (resolved_wallet, memo) = client.resolve(&hash); - assert_eq!(resolved_wallet, caller); - assert_eq!(memo, Some(4242u64)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/test.rs` around lines 263 - 291, The test direct_root_override_entrypoint_is_rejected contains leftover merge-conflict/duplicated code: remove the duplicated old test block and keep the intended single coherent test flow (the initial failed env.try_invoke_contract assertion, the check client.get_merkle_root, then env.mock_all_auths followed by the setup_with_root path). Edit the function body to eliminate the merged-old lines so only one sequence using setup, setup_with_root, caller, hash, new_root, PublicSignals, client.register_resolver, client.set_memo and client.resolve remains; ensure the assertions (Err(Ok(InvokeError::Abort)), root equality, resolved_wallet and memo checks) remain and the test compiles.
191-214:⚠️ Potential issue | 🔴 CriticalMissing closing brace and duplicate test code cause parse errors.
The test
root_progresses_across_multiple_registrations(lines 191-203) is missing its closing}. Lines 204-214 contain old imports and a duplicatesetupfunction that conflict with the new test infrastructure (lines 71-81).🐛 Proposed fix to close test and remove duplicate code
assert_eq!(client.get_merkle_root(), second_public_signals.new_root); assert!(client.has_commitment(&first_public_signals.commitment)); assert!(client.has_commitment(&second_public_signals.commitment)); +} -use crate::smt_root::SmtRoot; -use crate::types::{ChainType, PublicSignals}; -use crate::{Contract, ContractClient}; -use soroban_sdk::testutils::{Address as _, Events}; -use soroban_sdk::{Address, Bytes, BytesN, Env}; - -fn setup(env: &Env) -> (Address, ContractClient<'_>) { - let contract_id = env.register(Contract, ()); - let client = ContractClient::new(env, &contract_id); - (contract_id, client) -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/test.rs` around lines 191 - 214, The test function root_progresses_across_multiple_registrations is missing its closing brace and a duplicated block (imports and a second setup function) was accidentally inserted; close the test by adding the missing `}` immediately after the assertions in root_progresses_across_multiple_registrations, and remove the duplicated import block and duplicate setup function (the extra use crate::smt_root::SmtRoot / use crate::types::{ChainType, PublicSignals} / use crate::{Contract, ContractClient} / fn setup(...) block) so the file uses the original setup() and imports defined earlier and no duplicate symbols remain.
237-260:⚠️ Potential issue | 🔴 CriticalTest
root_cannot_be_overridden_by_reinitializinghas merge conflict.Lines 237-260 show merge conflict remnants where the new test body (lines 237-244) is incorrectly merged with old test code (lines 245-260). The old code references
setup_with_root,register_resolver,resolve,PublicSignals(old version), anddummy_proofwhich are incompatible with the new test structure.🐛 Proposed fix
#[test] fn root_cannot_be_overridden_by_reinitializing() { let env = Env::default(); let (_, client, verifier_id) = setup(&env); let result = client.try_init(&verifier_id, &bytes(&env, 9)); assert_eq!(result, Err(Ok(ContractError::AlreadyInitialized.into()))); assert_eq!(client.get_merkle_root(), bytes(&env, 0)); - env.mock_all_auths(); - let (_, client, root) = setup_with_root(&env); - let caller = Address::generate(&env); - let hash = commitment(&env, 0); - let new_root = BytesN::from_array(&env, &[2u8; 32]); - - let signals = PublicSignals { - old_root: root, - new_root, - }; - client.register_resolver(&caller, &hash, &dummy_proof(&env), &signals); - - let (resolved_wallet, memo) = client.resolve(&hash); - assert_eq!(resolved_wallet, caller); - assert_eq!(memo, None); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/test.rs` around lines 237 - 260, The test contains leftover merge-conflict/old-code fragments: remove the block that references setup_with_root, register_resolver, resolve, PublicSignals, and dummy_proof and keep only the new test flow that verifies reinitialization is rejected; specifically, in the function root_cannot_be_overridden_by_reinitializing() delete the old code after env.mock_all_auths() (all uses of setup_with_root, caller, hash, new_root, PublicSignals, register_resolver, resolve, and related assertions) so the test only uses setup(), client.try_init(), client.get_merkle_root(), and the existing assertions that assert AlreadyInitialized and the root remains bytes(&env, 0).gateway-contract/contracts/core_contract/src/types.rs (2)
12-37:⚠️ Potential issue | 🔴 CriticalDuplicate
PublicSignalsstruct definitions will cause compilation error E0428.Two
PublicSignalsstructs are defined:
- Lines 14-18: New version with
old_root,new_root,commitmentfields- Lines 34-37: Old version with only
old_root,new_rootfieldsThe old definition (lines 29-37) must be removed. Based on the PR objectives and test code, the new version with the
commitmentfield is the correct one.🐛 Proposed fix to remove duplicate definition
} -/// Public signals extracted from a Groth16 non-inclusion proof. -/// `old_root` must match the current on-chain SMT root. -/// `new_root` becomes the new SMT root after a successful registration. -#[contracttype] -#[derive(Clone)] -pub struct PublicSignals { - pub old_root: BytesN<32>, - pub new_root: BytesN<32>, -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/types.rs` around lines 12 - 37, Remove the duplicate PublicSignals struct declaration and keep the updated version that includes the commitment field; specifically, delete the second definition of PublicSignals (the one with only old_root and new_root) so there is a single #[contracttype] #[derive(...)] pub struct PublicSignals { pub old_root: BytesN<32>, pub new_root: BytesN<32>, pub commitment: BytesN<32>, } and ensure any associated doc comment about Groth16 non-inclusion is attached to that remaining struct and no conflicting derives/attributes (e.g., #[contracttype], #[derive(Clone)]) are duplicated elsewhere.
1-37:⚠️ Potential issue | 🔴 CriticalRestore missing
ResolveDatatype or remove all usages.
ResolveDatais imported and used inlib.rs(lines 29, 125, 144, 157) andtest.rs(line 502), but the type definition is absent fromtypes.rs. This will cause compilation failures.Restore the definition:
#[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct ResolveData { pub wallet: Address, pub memo: Option<u64>, }Or remove all usages in
lib.rsandtest.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/types.rs` around lines 1 - 37, The codebase references a missing contracttype struct ResolveData (used by lib.rs functions and tests), so restore its definition in types.rs by adding a #[contracttype] struct named ResolveData with fields wallet of type Address and memo of type Option<u64> (derive Clone, Debug, Eq, PartialEq), or alternatively remove all usages of ResolveData from lib.rs and test.rs (the symbols calling it are the ResolveData type in lib.rs and tests referenced around the earlier compile errors).gateway-contract/contracts/core_contract/src/storage.rs (1)
44-56:⚠️ Potential issue | 🔴 CriticalRemove duplicate
DataKeyenum definition and completestore_commitmentfunction.The file has two conflicting
DataKeyenum definitions (lines 5-9 and lines 49-56) causing compilation error E0428. Additionally, thestore_commitmentfunction at line 40 is incomplete and not properly closed.The old
DataKeyvariants (Resolver,SmtRoot,StellarAddress) are actively used inlib.rs,smt_root.rs, andtest.rs. Either:
- Merge both definitions into a single
DataKeyenum that includes all variants needed, or- Refactor all usage sites to use only the new variants (
CurrentMerkleRoot,Verifier,Commitment)Fix the
store_commitmentfunction closure before proceeding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/core_contract/src/storage.rs` around lines 44 - 56, Merge the duplicate DataKey enums into one consolidated enum named DataKey that contains all required variants used across the codebase (include Resolver(BytesN<32>), SmtRoot, StellarAddress(BytesN<32>) as well as any new variants like CurrentMerkleRoot, Verifier, Commitment if needed) so there is only a single definition; then fix and properly close the store_commitment function (ensure its braces complete and it returns/sets storage as expected) so compilation succeeds and usages in lib.rs, smt_root.rs, and test.rs resolve to the unified DataKey variants (refer to DataKey and store_commitment to locate the changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway-contract/contracts/core_contract/src/lib.rs`:
- Around line 31-32: There are duplicate/merged contract definitions: reconcile
and remove the orphan/old struct impl so only one contract type remains—ensure
all methods belong to the new CoreContract; specifically move the submit_proof
function into an impl CoreContract block (or rename CoreContract back to
Contract everywhere) and delete the extraneous impl Contract block and any
duplicate struct declaration so symbols CoreContract, Contract, submit_proof,
and impl Contract are consistent and compile.
- Around line 50-78: The impl CoreContract block is not closed because
submit_proof ends with an unfinished if !is_valid { branch; complete
submit_proof by handling the invalid-proof branch (e.g., call
panic_with_error!(&env, ContractError::InvalidProof); or any intended recovery),
perform the remaining valid-proof logic (store commitment via
storage::set_commitment, update merkle root with storage::set_merkle_root, emit
events, etc.), and then close the impl block with a matching } for impl
CoreContract; afterwards remove or reconcile the stray #[contractimpl] that
begins the old impl Contract so there are no overlapping/duplicate impl blocks.
In `@gateway-contract/contracts/core_contract/src/storage.rs`:
- Around line 40-44: The function store_commitment is missing its closing brace
and a stray import line was accidentally merged into the function body; close
the function by adding the missing "}" after the env.storage()...set(...)
statement and remove the duplicated import line `use soroban_sdk::{contracttype,
BytesN};` from inside the function so the import remains at module scope and the
function ends cleanly (refer to function name store_commitment and the
DataKey::Commitment usage to locate the block).
In `@gateway-contract/contracts/core_contract/src/types.rs`:
- Around line 1-2: Remove the duplicate use lines importing from soroban_sdk and
consolidate into a single use statement that lists all required symbols (e.g.,
contracttype, Address, BytesN, Symbol) so there is no overlapping import of
contracttype or BytesN; update the existing declarations (types.rs) to reference
that single use line and delete the redundant import to resolve compilation
error E0252.
---
Outside diff comments:
In `@gateway-contract/contracts/core_contract/src/lib.rs`:
- Around line 3-14: The file contains duplicate private and public module
declarations (mod errors/events/storage/types vs pub mod
errors/events/storage/types) causing E0428; remove the private declarations and
keep the public ones (retain pub mod address_manager, errors, events,
registration, smt_root, storage, types, zk_verifier) so each module is declared
only once and exported for external use.
- Around line 19-29: There are duplicate imports from soroban_sdk and the types
module causing E0252; consolidate them into a single use declaration (e.g.,
merge the two soroban_sdk lines into one that includes contract, contractclient
(if needed), contractimpl, panic_with_error, Address, Bytes, BytesN, Env) and
remove the redundant import; likewise unify the types imports so
PublicSignals/Proof/ResolveData/ChainType come from the correct module
(crate::types or types) in one use statement and remove the other conflicting
use. Update the top-level imports (the duplicate soroban_sdk lines and duplicate
types imports) so each symbol appears only once.
In `@gateway-contract/contracts/core_contract/src/storage.rs`:
- Around line 44-56: Merge the duplicate DataKey enums into one consolidated
enum named DataKey that contains all required variants used across the codebase
(include Resolver(BytesN<32>), SmtRoot, StellarAddress(BytesN<32>) as well as
any new variants like CurrentMerkleRoot, Verifier, Commitment if needed) so
there is only a single definition; then fix and properly close the
store_commitment function (ensure its braces complete and it returns/sets
storage as expected) so compilation succeeds and usages in lib.rs, smt_root.rs,
and test.rs resolve to the unified DataKey variants (refer to DataKey and
store_commitment to locate the changes).
In `@gateway-contract/contracts/core_contract/src/test.rs`:
- Around line 263-291: The test direct_root_override_entrypoint_is_rejected
contains leftover merge-conflict/duplicated code: remove the duplicated old test
block and keep the intended single coherent test flow (the initial failed
env.try_invoke_contract assertion, the check client.get_merkle_root, then
env.mock_all_auths followed by the setup_with_root path). Edit the function body
to eliminate the merged-old lines so only one sequence using setup,
setup_with_root, caller, hash, new_root, PublicSignals,
client.register_resolver, client.set_memo and client.resolve remains; ensure the
assertions (Err(Ok(InvokeError::Abort)), root equality, resolved_wallet and memo
checks) remain and the test compiles.
- Around line 191-214: The test function
root_progresses_across_multiple_registrations is missing its closing brace and a
duplicated block (imports and a second setup function) was accidentally
inserted; close the test by adding the missing `}` immediately after the
assertions in root_progresses_across_multiple_registrations, and remove the
duplicated import block and duplicate setup function (the extra use
crate::smt_root::SmtRoot / use crate::types::{ChainType, PublicSignals} / use
crate::{Contract, ContractClient} / fn setup(...) block) so the file uses the
original setup() and imports defined earlier and no duplicate symbols remain.
- Around line 237-260: The test contains leftover merge-conflict/old-code
fragments: remove the block that references setup_with_root, register_resolver,
resolve, PublicSignals, and dummy_proof and keep only the new test flow that
verifies reinitialization is rejected; specifically, in the function
root_cannot_be_overridden_by_reinitializing() delete the old code after
env.mock_all_auths() (all uses of setup_with_root, caller, hash, new_root,
PublicSignals, register_resolver, resolve, and related assertions) so the test
only uses setup(), client.try_init(), client.get_merkle_root(), and the existing
assertions that assert AlreadyInitialized and the root remains bytes(&env, 0).
In `@gateway-contract/contracts/core_contract/src/types.rs`:
- Around line 12-37: Remove the duplicate PublicSignals struct declaration and
keep the updated version that includes the commitment field; specifically,
delete the second definition of PublicSignals (the one with only old_root and
new_root) so there is a single #[contracttype] #[derive(...)] pub struct
PublicSignals { pub old_root: BytesN<32>, pub new_root: BytesN<32>, pub
commitment: BytesN<32>, } and ensure any associated doc comment about Groth16
non-inclusion is attached to that remaining struct and no conflicting
derives/attributes (e.g., #[contracttype], #[derive(Clone)]) are duplicated
elsewhere.
- Around line 1-37: The codebase references a missing contracttype struct
ResolveData (used by lib.rs functions and tests), so restore its definition in
types.rs by adding a #[contracttype] struct named ResolveData with fields wallet
of type Address and memo of type Option<u64> (derive Clone, Debug, Eq,
PartialEq), or alternatively remove all usages of ResolveData from lib.rs and
test.rs (the symbols calling it are the ResolveData type in lib.rs and tests
referenced around the earlier compile errors).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b95cd7b-2ca8-4486-952d-005ba55170ca
📒 Files selected for processing (5)
gateway-contract/contracts/core_contract/src/errors.rsgateway-contract/contracts/core_contract/src/lib.rsgateway-contract/contracts/core_contract/src/storage.rsgateway-contract/contracts/core_contract/src/test.rsgateway-contract/contracts/core_contract/src/types.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway-contract/contracts/core_contract/src/errors.rs
| pub fn store_commitment(env: &Env, commitment: &BytesN<32>) { | ||
| env.storage() | ||
| .persistent() | ||
| .set(&DataKey::Commitment(commitment.clone()), &true); | ||
| use soroban_sdk::{contracttype, BytesN}; |
There was a problem hiding this comment.
Missing closing brace for store_commitment function causes parse error.
The store_commitment function starting at line 40 is missing its closing }. Line 44 shows old code (use soroban_sdk::{contracttype, BytesN};) that was incorrectly merged into the function body.
🐛 Proposed fix to close function and remove duplicate code
pub fn store_commitment(env: &Env, commitment: &BytesN<32>) {
env.storage()
.persistent()
.set(&DataKey::Commitment(commitment.clone()), &true);
+}
-use soroban_sdk::{contracttype, BytesN};
-
-/// Storage keys for the Core contract's persistent and instance storage.
-#[contracttype]
-#[derive(Clone, Debug, Eq, PartialEq)]
-pub enum DataKey {
- /// Key for resolver data, indexed by commitment.
- Resolver(BytesN<32>),
- /// Key for the SMT root in instance storage.
- SmtRoot,
- /// Key for the primary Stellar address linked to a username hash.
- StellarAddress(BytesN<32>),
-}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/core_contract/src/storage.rs` around lines 40 -
44, The function store_commitment is missing its closing brace and a stray
import line was accidentally merged into the function body; close the function
by adding the missing "}" after the env.storage()...set(...) statement and
remove the duplicated import line `use soroban_sdk::{contracttype, BytesN};`
from inside the function so the import remains at module scope and the function
ends cleanly (refer to function name store_commitment and the
DataKey::Commitment usage to locate the block).
ryzen-xp
added
Blocked
Code Conflict
ryzen-xp
changed the title
2 participants
This pull request refactors and extends the
core_contractfor the gateway system to implement a Merkle root–based registration flow with proof verification, event emission, and robust storage management. The contract now supports zero-knowledge proof–based username registration, tracks commitments, manages Merkle roots, and emits events for state changes. Comprehensive tests are added to ensure correct behavior and error handling.Key changes:
Core Contract Redesign and Features
lib.rsto support initialization, proof submission with Merkle root and commitment checks, integration with an external verifier contract, and event emission on state changes. The contract now exposes methods for initialization, proof submission, root and verifier retrieval, and commitment existence checks.ContractErrorfor robust error handling (e.g., already initialized, not initialized, root mismatch, invalid proof, duplicate commitment).UsernameRegisteredandMerkleRootUpdatedfor better observability of contract state changes.Storage Layer Refactor
storage.rsmodule to encapsulate all persistent state management, including Merkle root, verifier address, and commitment tracking, using a typed key system.Type Definitions
ProofandPublicSignalsto model the data required for proof verification and registration.Comprehensive Testing
test.rsmodule with extensive test coverage for initialization, proof submission, error conditions (invalid proof, stale root, duplicate commitment), and event emission, using a mock verifier contract for simulation.Closes #56
Summary by CodeRabbit
New Features
Bug Fixes / Error Handling
Removed
Tests