refactor: normalize CoreContract error handling and structured events#449
Conversation
- Replace panic_with_error! with explicit Result returns in admin, registration, resolver, transfer, and address_manager modules - Update top-level Contract entrypoints to expose Result-based APIs for fallible operations - Standardize event emission using structured events with consistent naming - Add helper functions for emitting events in events.rs - Update tests to work with new event symbols
|
@anoncon 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! 🚀 |
📝 WalkthroughWalkthroughThis PR refactors the CoreContract to replace panic-based error handling with explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 7
🧹 Nitpick comments (3)
onchain/contracts/core_contract/src/events.rs (3)
25-121: Parameter nameenvis inconsistent with the rest of the PR'serename.The PR summary calls out renaming
env→ethroughout core contract modules, and the call sites inadmin.rs/registration.rsuse&e. This file still usesenv: &Enveverywhere. Purely cosmetic and works fine because of the borrow at call sites, but renaming here would close out the consistency goal of the PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/events.rs` around lines 25 - 121, The functions in this file (e.g., emit_init, emit_transfer, emit_register, emit_root_updated, emit_admin_set, emit_operator_set, emit_addr_added, emit_chain_added, emit_chain_removed, emit_role_granted, emit_role_revoked, emit_privacy_set, emit_shielded_add, emit_username_reg, emit_stellar_add, emit_stellar_rem, emit_delegate_granted, emit_delegate_revoked) still use the parameter name env: &Env while the rest of the PR renamed it to e; change each function signature to use e: &Env and update every use of env inside each function to e (keeping the same calls to e.events().publish and preserving attributes like #[allow(deprecated)]), so the file matches the PR-wide naming convention.
90-93: Inconsistent payload handling vs. the otheremit_*helpers.Every other helper clones the payload (
x.clone()) before passing it topublish;emit_username_reginstead passes the borrowcommitment(an&BytesN<32>) through. It compiles viaIntoValfor refs, but the inconsistency is a footgun for future edits and complicates grep-based audits of the event surface.♻️ Make it consistent
pub fn emit_username_reg(env: &Env, commitment: &BytesN<32>) { #[allow(deprecated)] - env.events().publish((EVENT_USERNAME,), commitment); + env.events().publish((EVENT_USERNAME,), commitment.clone()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/events.rs` around lines 90 - 93, The helper emit_username_reg is passing a borrowed &BytesN<32> to env.events().publish which is inconsistent with other emit_* helpers that pass an owned clone; change the call to publish to pass an owned value (clone the commitment) so the signature and payload handling match others (i.e., use commitment.clone() as the payload when calling env.events().publish in emit_username_reg) and keep the same event tuple (EVENT_USERNAME,) and env/events usage.
25-121: Migrate to#[contractevent]structured events to align with soroban-sdk v23.Every
emit_*function here uses the deprecatedenv.events().publish(...)API, suppressing warnings with#[allow(deprecated)]. Soroban SDK v23 officially replacesEvents::publishwith#[contractevent]for structured event definition and emission. This approach provides type-safe payloads, generates contract specifications, and eliminates deprecation suppression — directly matching the PR's stated goal of "structured event emission."Minor inconsistencies in the current approach:
- Line 92 (
emit_username_reg): passescommitmentdirectly without cloning, while all other functions clone their arguments.- Topic naming: "RVN" (revoked) is non-standard; conventional abbreviations would be "RVK" or "REV". If these topic strings must be preserved for off-chain consumer compatibility, document the rationale explicitly.
If you intend to keep the legacy
EVENT_*constants and helper functions for backward compatibility, clarify that decision; otherwise, migrating to#[contractevent]would be the cleaner path forward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/core_contract/src/events.rs` around lines 25 - 121, All emit_* helpers use the deprecated env.events().publish API (suppressed with #[allow(deprecated)]) and one function emit_username_reg fails to clone its BytesN argument; migrate to Soroban v23 structured events by defining #[contractevent] structs for each event payload and replacing calls like emit_init, emit_transfer, emit_register, emit_root_updated, emit_admin_set, emit_operator_set, emit_addr_added, emit_chain_added, emit_chain_removed, emit_role_granted, emit_role_revoked, emit_privacy_set, emit_shielded_add, emit_username_reg, emit_stellar_add, emit_stellar_rem, emit_delegate_granted, and emit_delegate_revoked with contract event emissions (remove #[allow(deprecated)]), ensure all event payloads clone or take owned values consistently (fix emit_username_reg to clone the commitment), and keep or document any legacy EVENT_* topic constants only if backward compatibility is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@onchain/contracts/core_contract/src/address_manager.rs`:
- Line 4: The import line for crate::events (containing emit_chain_added,
emit_chain_removed, emit_shielded_add, emit_stellar_add, emit_stellar_rem,
EVENT_CHAIN_ADD, EVENT_CHAIN_REM, EVENT_SHIELD_ADD, EVENT_STELLAR_ADD,
EVENT_STELLAR_REM) is too long and failing rustfmt; reformat the use
crate::events::{...} into a multi-line import or simply run cargo fmt --all to
apply rustfmt's wrapping and commit the resulting changes so the import is
wrapped across lines per rustfmt rules.
In `@onchain/contracts/core_contract/src/admin.rs`:
- Around line 22-32: The three getters get_contract_owner, get_admin, and
get_operator currently call storage::get_*(&e).unwrap(), which causes opaque
panics; change them to return Result<Address, CoreError> (preferred) mirroring
the module (update callers in lib.rs accordingly) or keep the current signature
but replace .unwrap() with .unwrap_or_else(|_| panic_with_error!(e,
CoreError::NotFound)) so failures surface as structured CoreError::NotFound;
apply the same change to the other getter group mentioned (lines 50–52).
- Line 4: The imports EVENT_ADMIN_SET and EVENT_OPER_SET are unused in admin.rs;
either remove these two constants from the use statement or update the
role-emission calls (currently using emit_role_granted with the role symbols
"admin" and "operator") to call the dedicated emit_admin_set / emit_operator_set
functions instead; locate the use line that lists emit_init, emit_role_granted,
emit_root_updated, EVENT_ADMIN_SET, EVENT_OPER_SET, EVENT_ROOT_UPD and remove
the two unused constants or replace the emit_role_granted calls with the
specific emit_admin_set/emit_operator_set calls so the imports are actually
used.
In `@onchain/contracts/core_contract/src/events.rs`:
- Line 16: The event topic constants EVENT_ROLE_RVN and EVENT_DLG_RVN use an
unclear abbreviation; rename them to a clear revocation token (e.g.,
EVENT_ROLE_RVK and EVENT_DLG_RVK) and update all references: change the Symbol
definitions (symbol_short!("ROLE_RVN") / symbol_short!("DLG_RVN")) to the new
strings, update any emitters (emit_role_revoked, emit_delegate_revoked) to use
the new constants, and update tests and any off-chain indexer strings that
expect the old topics so the wire-visible topic names remain consistent; ensure
you update all usages of EVENT_ROLE_RVN and EVENT_DLG_RVN across the codebase.
In `@onchain/contracts/core_contract/src/lib.rs`:
- Line 51: The wrappers register_resolver and set_privacy_mode currently discard
the Result from Resolver::register_resolver and Resolver::set_privacy_mode (they
end the call with `;` and always return Ok(())), so any Err is swallowed and
tests will fail; fix by propagating the inner Result instead of ignoring it —
replace the call expressions in the functions register_resolver(e, c, h, p, s)
and set_privacy_mode(e, c, m) so they return the Resolver::* result (or use the
`?` operator) and then return Ok(()) only if appropriate, ensuring any
Err(CoreError) from Resolver methods bubbles up to the caller.
In `@onchain/contracts/core_contract/src/test.rs`:
- Around line 1119-1121: The test_transfer_succeeds test is marked #[ignore],
leaving the Contract::transfer success path and emit_transfer unverified; remove
the #[ignore] and rewrite the test to call the real Transfer::transfer (which
now returns Result) instead of the in-contract stub, propagate/handle the Result
(expect Ok), and assert the ROOT_UPD and TRANSFER events plus the post-transfer
state; locate the assertions around test_transfer_succeeds, replace the
in-contract invocation with Transfer::transfer(...) and update assertions to
match emitted events and final contract state.
- Around line 728-729: The assertion fails because
SmtRoot::get_root(env.clone()) returns Option<BytesN<32>> while signals.old_root
is BytesN<32>; restore a consistent comparison by unwrapping or wrapping
appropriately: either call get_root(...).expect or unwrap_or_else to produce
BytesN<32> before comparing to signals.old_root, or compare by wrapping
signals.old_root in Some(...) and assert_eq!(Some(signals.old_root), current);
update the assertion in the test that references SmtRoot::get_root and
signals.old_root accordingly.
---
Nitpick comments:
In `@onchain/contracts/core_contract/src/events.rs`:
- Around line 25-121: The functions in this file (e.g., emit_init,
emit_transfer, emit_register, emit_root_updated, emit_admin_set,
emit_operator_set, emit_addr_added, emit_chain_added, emit_chain_removed,
emit_role_granted, emit_role_revoked, emit_privacy_set, emit_shielded_add,
emit_username_reg, emit_stellar_add, emit_stellar_rem, emit_delegate_granted,
emit_delegate_revoked) still use the parameter name env: &Env while the rest of
the PR renamed it to e; change each function signature to use e: &Env and update
every use of env inside each function to e (keeping the same calls to
e.events().publish and preserving attributes like #[allow(deprecated)]), so the
file matches the PR-wide naming convention.
- Around line 90-93: The helper emit_username_reg is passing a borrowed
&BytesN<32> to env.events().publish which is inconsistent with other emit_*
helpers that pass an owned clone; change the call to publish to pass an owned
value (clone the commitment) so the signature and payload handling match others
(i.e., use commitment.clone() as the payload when calling env.events().publish
in emit_username_reg) and keep the same event tuple (EVENT_USERNAME,) and
env/events usage.
- Around line 25-121: All emit_* helpers use the deprecated env.events().publish
API (suppressed with #[allow(deprecated)]) and one function emit_username_reg
fails to clone its BytesN argument; migrate to Soroban v23 structured events by
defining #[contractevent] structs for each event payload and replacing calls
like emit_init, emit_transfer, emit_register, emit_root_updated, emit_admin_set,
emit_operator_set, emit_addr_added, emit_chain_added, emit_chain_removed,
emit_role_granted, emit_role_revoked, emit_privacy_set, emit_shielded_add,
emit_username_reg, emit_stellar_add, emit_stellar_rem, emit_delegate_granted,
and emit_delegate_revoked with contract event emissions (remove
#[allow(deprecated)]), ensure all event payloads clone or take owned values
consistently (fix emit_username_reg to clone the commitment), and keep or
document any legacy EVENT_* topic constants only if backward compatibility is
required.
🪄 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
Run ID: e757db42-b046-4900-87ec-ab79f2730ea8
📒 Files selected for processing (9)
onchain/contracts/core_contract/src/address_manager.rsonchain/contracts/core_contract/src/admin.rsonchain/contracts/core_contract/src/events.rsonchain/contracts/core_contract/src/lib.rsonchain/contracts/core_contract/src/registration.rsonchain/contracts/core_contract/src/resolver.rsonchain/contracts/core_contract/src/smt_root.rsonchain/contracts/core_contract/src/test.rsonchain/contracts/core_contract/src/transfer.rs
|
|
||
| use crate::errors::{ChainAddressError, CoreError}; | ||
| use crate::events::{shielded_add_event, stellar_rem_event, ADDR_ADD, CHAIN_ADD, CHAIN_REM}; | ||
| use crate::events::{emit_chain_added, emit_chain_removed, emit_shielded_add, emit_stellar_add, emit_stellar_rem, EVENT_CHAIN_ADD, EVENT_CHAIN_REM, EVENT_SHIELD_ADD, EVENT_STELLAR_ADD, EVENT_STELLAR_REM}; |
There was a problem hiding this comment.
cargo fmt --all --check is failing — likely the long single-line events import.
The Smart Contracts CI is red on cargo fmt. The 10-symbol single-line import on line 4 is over the default max_width and will be rewrapped by rustfmt. Run cargo fmt --all locally and commit the result.
♻️ Likely rustfmt output
-use crate::events::{emit_chain_added, emit_chain_removed, emit_shielded_add, emit_stellar_add, emit_stellar_rem, EVENT_CHAIN_ADD, EVENT_CHAIN_REM, EVENT_SHIELD_ADD, EVENT_STELLAR_ADD, EVENT_STELLAR_REM};
+use crate::events::{
+ emit_chain_added, emit_chain_removed, emit_shielded_add, emit_stellar_add, emit_stellar_rem,
+ EVENT_CHAIN_ADD, EVENT_CHAIN_REM, EVENT_SHIELD_ADD, EVENT_STELLAR_ADD, EVENT_STELLAR_REM,
+};📝 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.
| use crate::events::{emit_chain_added, emit_chain_removed, emit_shielded_add, emit_stellar_add, emit_stellar_rem, EVENT_CHAIN_ADD, EVENT_CHAIN_REM, EVENT_SHIELD_ADD, EVENT_STELLAR_ADD, EVENT_STELLAR_REM}; | |
| use crate::events::{ | |
| emit_chain_added, emit_chain_removed, emit_shielded_add, emit_stellar_add, emit_stellar_rem, | |
| EVENT_CHAIN_ADD, EVENT_CHAIN_REM, EVENT_SHIELD_ADD, EVENT_STELLAR_ADD, EVENT_STELLAR_REM, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/core_contract/src/address_manager.rs` at line 4, The import
line for crate::events (containing emit_chain_added, emit_chain_removed,
emit_shielded_add, emit_stellar_add, emit_stellar_rem, EVENT_CHAIN_ADD,
EVENT_CHAIN_REM, EVENT_SHIELD_ADD, EVENT_STELLAR_ADD, EVENT_STELLAR_REM) is too
long and failing rustfmt; reformat the use crate::events::{...} into a
multi-line import or simply run cargo fmt --all to apply rustfmt's wrapping and
commit the resulting changes so the import is wrapped across lines per rustfmt
rules.
|
|
||
| use crate::errors::CoreError; | ||
| use crate::events::{INIT_EVENT, ROLE_GRANTED}; | ||
| use crate::events::{emit_init, emit_role_granted, emit_root_updated, EVENT_ADMIN_SET, EVENT_OPER_SET, EVENT_ROOT_UPD}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm EVENT_ADMIN_SET / EVENT_OPER_SET are not emitted anywhere in the workspace.
rg -nP '\bEVENT_(ADMIN_SET|OPER_SET)\b' -C2
# Confirm any subscribers/tests still reference the old topics.
rg -nP '"(ADMIN_SET|OPER_SET)"' -C2Repository: Alien-Protocol/Alien-Protocol
Length of output: 2613
🏁 Script executed:
# Check what's in admin.rs around the affected lines
head -60 onchain/contracts/core_contract/src/admin.rs | cat -n
# Search for emit_admin_set and emit_operator_set calls in the entire workspace
rg 'emit_admin_set|emit_operator_set|emit_role_granted' -n
# Check if EVENT_ADMIN_SET/OPER_SET are actually used in admin.rs (not just imported)
rg 'EVENT_(ADMIN_SET|OPER_SET)' onchain/contracts/core_contract/src/admin.rsRepository: Alien-Protocol/Alien-Protocol
Length of output: 3303
Remove unused imports EVENT_ADMIN_SET and EVENT_OPER_SET from admin.rs line 4.
The module imports these constants but never uses them. The code emits role changes via emit_role_granted with role symbols ("admin", "operator") at lines 38 and 46, rather than using the dedicated emit_admin_set/emit_operator_set functions. Either remove these unused imports or refactor to use the dedicated emit functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/core_contract/src/admin.rs` at line 4, The imports
EVENT_ADMIN_SET and EVENT_OPER_SET are unused in admin.rs; either remove these
two constants from the use statement or update the role-emission calls
(currently using emit_role_granted with the role symbols "admin" and "operator")
to call the dedicated emit_admin_set / emit_operator_set functions instead;
locate the use line that lists emit_init, emit_role_granted, emit_root_updated,
EVENT_ADMIN_SET, EVENT_OPER_SET, EVENT_ROOT_UPD and remove the two unused
constants or replace the emit_role_granted calls with the specific
emit_admin_set/emit_operator_set calls so the imports are actually used.
| pub fn get_contract_owner(e: Env) -> Address { | ||
| storage::get_owner(&e).unwrap() | ||
| } | ||
|
|
||
| /// Returns the current admin address. | ||
| pub fn get_admin(env: Env) -> Address { | ||
| storage::get_admin(&env).unwrap_or_else(|| panic_with_error!(&env, CoreError::NotFound)) | ||
| pub fn get_admin(e: Env) -> Address { | ||
| storage::get_admin(&e).unwrap() | ||
| } | ||
|
|
||
| /// Returns the current operator address. | ||
| pub fn get_operator(env: Env) -> Address { | ||
| storage::get_operator(&env).unwrap_or_else(|| panic_with_error!(&env, CoreError::NotFound)) | ||
| pub fn get_operator(e: Env) -> Address { | ||
| storage::get_operator(&e).unwrap() | ||
| } |
There was a problem hiding this comment.
Getters now panic with an opaque unwrap() instead of a structured CoreError.
Replacing panic_with_error!(env, CoreError::NotFound) with .unwrap() regresses the PR's normalization goal: callers that previously received Error::from_contract_error(CoreError::NotFound as u32) will now see an unstructured unwrap() panic without a contract error code. The test test_require_owner_panics_if_uninitialized (lines 916–931) explicitly asserts CoreError::NotFound for the update_smt_root path; doing the same for get_contract_owner/get_admin/get_operator/get_smt_root keeps the surface consistent.
🛡️ Proposed fix — surface a structured error
Either keep these as Result<_, CoreError> (preferred, mirrors the rest of the module), or use unwrap_or_else with panic_with_error! to retain the contract error code:
- pub fn get_contract_owner(e: Env) -> Address {
- storage::get_owner(&e).unwrap()
- }
-
- pub fn get_admin(e: Env) -> Address {
- storage::get_admin(&e).unwrap()
- }
-
- pub fn get_operator(e: Env) -> Address {
- storage::get_operator(&e).unwrap()
- }
+ pub fn get_contract_owner(e: Env) -> Result<Address, CoreError> {
+ storage::get_owner(&e).ok_or(CoreError::NotFound)
+ }
+
+ pub fn get_admin(e: Env) -> Result<Address, CoreError> {
+ storage::get_admin(&e).ok_or(CoreError::NotFound)
+ }
+
+ pub fn get_operator(e: Env) -> Result<Address, CoreError> {
+ storage::get_operator(&e).ok_or(CoreError::NotFound)
+ }
@@
- pub fn get_smt_root(e: Env) -> BytesN<32> {
- smt_root::SmtRoot::get_root(e.clone()).unwrap()
- }
+ pub fn get_smt_root(e: Env) -> Result<BytesN<32>, CoreError> {
+ smt_root::SmtRoot::get_root(e.clone()).ok_or(CoreError::RootNotSet)
+ }Note: changing return types here will require corresponding updates in lib.rs (lines 35–45) and any tests asserting infallible getters.
Also applies to: 50-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/core_contract/src/admin.rs` around lines 22 - 32, The three
getters get_contract_owner, get_admin, and get_operator currently call
storage::get_*(&e).unwrap(), which causes opaque panics; change them to return
Result<Address, CoreError> (preferred) mirroring the module (update callers in
lib.rs accordingly) or keep the current signature but replace .unwrap() with
.unwrap_or_else(|_| panic_with_error!(e, CoreError::NotFound)) so failures
surface as structured CoreError::NotFound; apply the same change to the other
getter group mentioned (lines 50–52).
| pub const EVENT_CHAIN_ADD: Symbol = symbol_short!("CHAIN_ADD"); | ||
| pub const EVENT_CHAIN_REM: Symbol = symbol_short!("CHAIN_REM"); | ||
| pub const EVENT_ROLE_GNT: Symbol = symbol_short!("ROLE_GNT"); | ||
| pub const EVENT_ROLE_RVN: Symbol = symbol_short!("ROLE_RVN"); |
There was a problem hiding this comment.
RVN is an unusual abbreviation for "revoked" — likely should be RVK.
EVENT_ROLE_RVN / EVENT_DLG_RVN use RVN, which doesn't naturally map to any of "revoke/revoked/revocation" (the conventional short forms are RVK or REV). The matching grant constants use GNT (granted), so the asymmetry is also visible. These topic strings are wire-visible to off-chain indexers, so picking the right name now avoids a follow-up breaking change.
📝 Proposed rename
-pub const EVENT_ROLE_RVN: Symbol = symbol_short!("ROLE_RVN");
+pub const EVENT_ROLE_RVK: Symbol = symbol_short!("ROLE_RVK");
@@
-pub const EVENT_DLG_RVN: Symbol = symbol_short!("DLG_RVN");
+pub const EVENT_DLG_RVK: Symbol = symbol_short!("DLG_RVK");Update emit_role_revoked / emit_delegate_revoked and any tests/indexers accordingly.
Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/core_contract/src/events.rs` at line 16, The event topic
constants EVENT_ROLE_RVN and EVENT_DLG_RVN use an unclear abbreviation; rename
them to a clear revocation token (e.g., EVENT_ROLE_RVK and EVENT_DLG_RVK) and
update all references: change the Symbol definitions (symbol_short!("ROLE_RVN")
/ symbol_short!("DLG_RVN")) to the new strings, update any emitters
(emit_role_revoked, emit_delegate_revoked) to use the new constants, and update
tests and any off-chain indexer strings that expect the old topics so the
wire-visible topic names remain consistent; ensure you update all usages of
EVENT_ROLE_RVN and EVENT_DLG_RVN across the codebase.
| pub fn submit_proof(e: Env, c: Address, p: Proof, s: PublicSignals) -> Result<(), errors::CoreError> { Registration::submit_proof(e, c, p, s) } | ||
|
|
||
| pub fn register_resolver(e: Env, c: Address, h: BytesN<32>, p: Proof, s: PublicSignals) { Resolver::register_resolver(e, c, h, p, s); } | ||
| pub fn register_resolver(e: Env, c: Address, h: BytesN<32>, p: Proof, s: PublicSignals) -> Result<(), errors::CoreError> { Resolver::register_resolver(e, c, h, p, s); Ok(()) } |
There was a problem hiding this comment.
Critical: inner Result is discarded — errors are silently swallowed.
Both Resolver::register_resolver and Resolver::set_privacy_mode return Result<(), CoreError>, but the wrappers terminate the call expression with ; and unconditionally return Ok(()). Any Err(...) from the resolver (DuplicateCommitment, StaleRoot, InvalidProof, Unauthorized, NotFound) is dropped, the transaction commits as success, and existing tests that assert these contract errors will break, e.g.:
test_register_resolver_duplicate_commitment_fails—#[should_panic(expected = "Error(Contract,#4003)")]test_register_resolver_stale_root_fails—#[should_panic(expected = "Error(Contract,#4004)")]
This also produces an unused_must_use warning at compile time.
🐛 Proposed fix — propagate the inner Result
- pub fn register_resolver(e: Env, c: Address, h: BytesN<32>, p: Proof, s: PublicSignals) -> Result<(), errors::CoreError> { Resolver::register_resolver(e, c, h, p, s); Ok(()) }
+ pub fn register_resolver(e: Env, c: Address, h: BytesN<32>, p: Proof, s: PublicSignals) -> Result<(), errors::CoreError> { Resolver::register_resolver(e, c, h, p, s) }
@@
- pub fn set_privacy_mode(e: Env, c: Address, h: BytesN<32>, m: PrivacyMode) -> Result<(), errors::CoreError> { Resolver::set_privacy_mode(e, c, h, m); Ok(()) }
+ pub fn set_privacy_mode(e: Env, c: Address, h: BytesN<32>, m: PrivacyMode) -> Result<(), errors::CoreError> { Resolver::set_privacy_mode(e, c, h, m) }Also applies to: 55-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/core_contract/src/lib.rs` at line 51, The wrappers
register_resolver and set_privacy_mode currently discard the Result from
Resolver::register_resolver and Resolver::set_privacy_mode (they end the call
with `;` and always return Ok(())), so any Err is swallowed and tests will fail;
fix by propagating the inner Result instead of ignoring it — replace the call
expressions in the functions register_resolver(e, c, h, p, s) and
set_privacy_mode(e, c, m) so they return the Resolver::* result (or use the `?`
operator) and then return Ok(()) only if appropriate, ensuring any
Err(CoreError) from Resolver methods bubbles up to the caller.
| let current = SmtRoot::get_root(env.clone()); | ||
| assert_eq!(signals.old_root, current); |
There was a problem hiding this comment.
Type mismatch — assert_eq! will not compile.
SmtRoot::get_root returns Option<BytesN<32>>, but signals.old_root is BytesN<32>. There is no PartialEq between these two types, so this assertion fails to compile. Replacing the previous unwrap_or_else(...) with the bare call dropped the unwrapping.
🐛 Proposed fix
- let current = SmtRoot::get_root(env.clone());
- assert_eq!(signals.old_root, current);
+ let current = SmtRoot::get_root(env.clone()).expect("smt root should be set");
+ assert_eq!(signals.old_root, current);📝 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.
| let current = SmtRoot::get_root(env.clone()); | |
| assert_eq!(signals.old_root, current); | |
| let current = SmtRoot::get_root(env.clone()).expect("smt root should be set"); | |
| assert_eq!(signals.old_root, current); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/core_contract/src/test.rs` around lines 728 - 729, The
assertion fails because SmtRoot::get_root(env.clone()) returns
Option<BytesN<32>> while signals.old_root is BytesN<32>; restore a consistent
comparison by unwrapping or wrapping appropriately: either call
get_root(...).expect or unwrap_or_else to produce BytesN<32> before comparing to
signals.old_root, or compare by wrapping signals.old_root in Some(...) and
assert_eq!(Some(signals.old_root), current); update the assertion in the test
that references SmtRoot::get_root and signals.old_root accordingly.
| #[test] | ||
| #[ignore] | ||
| fn test_transfer_succeeds() { |
There was a problem hiding this comment.
#[ignore] on test_transfer_succeeds weakens coverage of the success path.
This was the only end-to-end test asserting both the ROOT_UPD and TRANSFER events plus the post-transfer state. Marking it #[ignore] leaves the happy path of Contract::transfer (incl. emit_transfer) untested, which conflicts with the PR's acceptance criteria of "updated tests covering success/error paths and events". Consider porting the in-contract block to drive the real Transfer::transfer (now returning Result) and unignore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/core_contract/src/test.rs` around lines 1119 - 1121, The
test_transfer_succeeds test is marked #[ignore], leaving the Contract::transfer
success path and emit_transfer unverified; remove the #[ignore] and rewrite the
test to call the real Transfer::transfer (which now returns Result) instead of
the in-contract stub, propagate/handle the Result (expect Ok), and assert the
ROOT_UPD and TRANSFER events plus the post-transfer state; locate the assertions
around test_transfer_succeeds, replace the in-contract invocation with
Transfer::transfer(...) and update assertions to match emitted events and final
contract state.
3 participants
closes #385
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor