feat(benchmark): add gas and footprint profiling for critical admin operations#277
feat(benchmark): add gas and footprint profiling for critical admin operations#277OlufunbiIK merged 2 commits intoOlufunbiIK:mainfrom
Conversation
|
@lishmanTech is attempting to deploy a commit to the olufunbiik's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughIntroduces an access control module for admin management and a benchmark module for gas profiling. The access control module provides functions to retrieve and transfer admin privileges with authentication. The benchmark module captures and computes CPU instruction and memory usage deltas to measure operation costs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
🧹 Nitpick comments (3)
contracts/benchmark/mod.rs (1)
6-15: Consider returning the closure result alongside the profile.The current signature
F: FnOnce()discards any return value from the profiled operation. This limits usability when callers need both the benchmark metrics and the operation result.♻️ Proposed enhancement to return closure result
-/// Measure gas/footprint for a closure -pub fn profile<F>(env: &Env, f: F) -> GasProfile +/// Measure gas/footprint for a closure, returning both the result and profile +pub fn profile<F, R>(env: &Env, f: F) -> (R, GasProfile) where - F: FnOnce(), + F: FnOnce() -> R, { let start = GasProfile::new(env); - f(); + let result = f(); let end = GasProfile::new(env); - start.delta(&end) + (result, start.delta(&end)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/benchmark/mod.rs` around lines 6 - 15, The profile function currently drops the closure return value because it bounds F as FnOnce() and returns only a GasProfile; change the signature to generic over the closure return type (e.g., F: FnOnce() -> R) and capture the closure result when calling f(), then return both the gas profile delta and the closure result (for example as (GasProfile, R) or a small struct). Update the function body around profile, GasProfile::new, f(), and start.delta(&end) to compute the delta and return it together with the captured result.contracts/access_control/admin.rs (1)
19-21: Consider usingResultinstead ofpanic!for recoverable errors.Using
panic!("New admin must be different")aborts the contract. Sibling contracts in this repo (e.g.,staking,multisig) returnResult<(), Error>for recoverable error conditions, allowing callers to handle failures gracefully. Consider defining an error enum and returningErr(Error::SameAdmin)instead.♻️ Suggested pattern
#[derive(Debug, Clone, PartialEq)] #[contracttype] pub enum AccessControlError { SameAdmin, AdminNotInitialized, } pub fn transfer_admin(env: &Env, new_admin: Address) -> Result<(), AccessControlError> { let current_admin = require_admin(env)?; if current_admin == new_admin { return Err(AccessControlError::SameAdmin); } new_admin.require_auth(); super::storage::set_admin(env, &new_admin); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/access_control/admin.rs` around lines 19 - 21, The code uses panic!("New admin must be different") inside transfer_admin which aborts the contract; change transfer_admin to return Result<(), AccessControlError> (define an AccessControlError enum with SameAdmin, AdminNotInitialized, etc.), replace the panic with returning Err(AccessControlError::SameAdmin) when current_admin == new_admin, keep calling require_admin() to obtain current_admin, ensure new_admin.require_auth() and super::storage::set_admin(env, &new_admin) are performed before returning Ok(()), and update any callers/tests to handle the Result return type.contracts/access_control/storage.rs (1)
9-13:get_adminpanics when admin is uninitialized—consider a fallible alternative.Using
.expect("Admin not initialized")will abort the contract if no admin has been set. While this may be intentional, it makes it impossible for callers to gracefully handle the uninitialized state (e.g., during setup or migration). Consider adding anOption-returning variant likeget_admin_optalongside this function.💡 Suggested addition
pub fn get_admin_opt(env: &Env) -> Option<Address> { env.storage().instance().get(&ADMIN_KEY) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/access_control/storage.rs` around lines 9 - 13, The current get_admin(env: &Env) -> Address panics if ADMIN_KEY is missing; add a non-panicking, Option-returning accessor get_admin_opt(env: &Env) -> Option<Address> that returns env.storage().instance().get(&ADMIN_KEY) and keep get_admin for callers that want to assert presence (or refactor callers to call get_admin_opt and handle None); update any callers that should handle uninitialized state to use get_admin_opt instead of relying on expect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/access_control/admin.rs`:
- Around line 15-23: The transfer_admin function currently only checks the
caller via require_admin(env) and sets the new admin without confirming the
recipient can authenticate; update transfer_admin to also call
new_admin.require_auth() (or equivalent) before calling
super::storage::set_admin to enforce two-sided authorization so the new admin
consents/controls the address (keep existing require_admin(env) check and the
self-transfer panic).
In `@contracts/benchmark/profiler.rs`:
- Line 1: The import list includes an unused symbol BytesN which should be
removed to clean up warnings: in the module's use declaration (currently "use
soroban_sdk::{Env, BytesN};") delete BytesN so only Env is imported; ensure no
other references to BytesN (e.g., variables or functions named BytesN) remain in
profiler.rs before committing.
- Around line 15-20: The delta method in GasProfile uses plain u64 subtraction
and can panic or wrap on underflow; update GasProfile::delta to use
saturating_sub for cpu_insns and mem_bytes so any negative delta clamps to zero
(e.g., replace after.cpu_insns - self.cpu_insns and after.mem_bytes -
self.mem_bytes with saturating_sub calls on those fields), keeping the function
signature and returned GasProfile unchanged.
---
Nitpick comments:
In `@contracts/access_control/admin.rs`:
- Around line 19-21: The code uses panic!("New admin must be different") inside
transfer_admin which aborts the contract; change transfer_admin to return
Result<(), AccessControlError> (define an AccessControlError enum with
SameAdmin, AdminNotInitialized, etc.), replace the panic with returning
Err(AccessControlError::SameAdmin) when current_admin == new_admin, keep calling
require_admin() to obtain current_admin, ensure new_admin.require_auth() and
super::storage::set_admin(env, &new_admin) are performed before returning
Ok(()), and update any callers/tests to handle the Result return type.
In `@contracts/access_control/storage.rs`:
- Around line 9-13: The current get_admin(env: &Env) -> Address panics if
ADMIN_KEY is missing; add a non-panicking, Option-returning accessor
get_admin_opt(env: &Env) -> Option<Address> that returns
env.storage().instance().get(&ADMIN_KEY) and keep get_admin for callers that
want to assert presence (or refactor callers to call get_admin_opt and handle
None); update any callers that should handle uninitialized state to use
get_admin_opt instead of relying on expect.
In `@contracts/benchmark/mod.rs`:
- Around line 6-15: The profile function currently drops the closure return
value because it bounds F as FnOnce() and returns only a GasProfile; change the
signature to generic over the closure return type (e.g., F: FnOnce() -> R) and
capture the closure result when calling f(), then return both the gas profile
delta and the closure result (for example as (GasProfile, R) or a small struct).
Update the function body around profile, GasProfile::new, f(), and
start.delta(&end) to compute the delta and return it together with the captured
result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4a96606-8a5d-40e1-947b-ecb0590853aa
📒 Files selected for processing (5)
contracts/access_control/admin.rscontracts/access_control/mod.rscontracts/access_control/storage.rscontracts/benchmark/mod.rscontracts/benchmark/profiler.rs
| pub fn transfer_admin(env: &Env, new_admin: Address) { | ||
| let current_admin = require_admin(env); | ||
|
|
||
| // Prevent accidental self-transfer (optional safety) | ||
| if current_admin == new_admin { | ||
| panic!("New admin must be different"); | ||
| } | ||
|
|
||
| super::storage::set_admin(env, &new_admin); |
There was a problem hiding this comment.
Consider requiring new_admin.require_auth() for two-sided authorization.
The transfer_admin function only authenticates the current admin, not the new admin. This allows transferring ownership to any address without the recipient's consent, which could lead to:
- Accidental transfer to an incorrect or non-existent address
- No confirmation that the new admin can actually control that address
Per the relevant code snippets, sibling contracts (staking, multisig) use a two-sided pattern where the admin authenticates themselves during initialization. Consider requiring new_admin.require_auth() to ensure the recipient can control the address before accepting ownership.
🔒 Proposed fix for two-sided authorization
pub fn transfer_admin(env: &Env, new_admin: Address) {
let current_admin = require_admin(env);
// Prevent accidental self-transfer (optional safety)
if current_admin == new_admin {
panic!("New admin must be different");
}
+ // Require new admin to consent to ownership transfer
+ new_admin.require_auth();
+
super::storage::set_admin(env, &new_admin);
}📝 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.
| pub fn transfer_admin(env: &Env, new_admin: Address) { | |
| let current_admin = require_admin(env); | |
| // Prevent accidental self-transfer (optional safety) | |
| if current_admin == new_admin { | |
| panic!("New admin must be different"); | |
| } | |
| super::storage::set_admin(env, &new_admin); | |
| pub fn transfer_admin(env: &Env, new_admin: Address) { | |
| let current_admin = require_admin(env); | |
| // Prevent accidental self-transfer (optional safety) | |
| if current_admin == new_admin { | |
| panic!("New admin must be different"); | |
| } | |
| // Require new admin to consent to ownership transfer | |
| new_admin.require_auth(); | |
| super::storage::set_admin(env, &new_admin); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/access_control/admin.rs` around lines 15 - 23, The transfer_admin
function currently only checks the caller via require_admin(env) and sets the
new admin without confirming the recipient can authenticate; update
transfer_admin to also call new_admin.require_auth() (or equivalent) before
calling super::storage::set_admin to enforce two-sided authorization so the new
admin consents/controls the address (keep existing require_admin(env) check and
the self-transfer panic).
| @@ -0,0 +1,21 @@ | |||
| use soroban_sdk::{Env, BytesN}; | |||
There was a problem hiding this comment.
Remove unused import BytesN.
BytesN is imported but never used in this file.
🧹 Proposed fix
-use soroban_sdk::{Env, BytesN};
+use soroban_sdk::Env;📝 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 soroban_sdk::{Env, BytesN}; | |
| use soroban_sdk::Env; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/benchmark/profiler.rs` at line 1, The import list includes an
unused symbol BytesN which should be removed to clean up warnings: in the
module's use declaration (currently "use soroban_sdk::{Env, BytesN};") delete
BytesN so only Env is imported; ensure no other references to BytesN (e.g.,
variables or functions named BytesN) remain in profiler.rs before committing.
| pub fn delta(&self, after: &GasProfile) -> GasProfile { | ||
| GasProfile { | ||
| cpu_insns: after.cpu_insns - self.cpu_insns, | ||
| mem_bytes: after.mem_bytes - self.mem_bytes, | ||
| } | ||
| } |
There was a problem hiding this comment.
Unchecked subtraction can panic or silently wrap on underflow.
The delta() method uses unchecked u64 subtraction. If after values are unexpectedly less than self values (e.g., due to budget reset, test misconfiguration, or SDK behavior changes), this will panic in debug builds or silently wrap around in release builds, producing misleading benchmark results.
Consider using saturating_sub() for safer arithmetic that clamps to zero instead of panicking/wrapping.
🛡️ Proposed fix using saturating subtraction
pub fn delta(&self, after: &GasProfile) -> GasProfile {
GasProfile {
- cpu_insns: after.cpu_insns - self.cpu_insns,
- mem_bytes: after.mem_bytes - self.mem_bytes,
+ cpu_insns: after.cpu_insns.saturating_sub(self.cpu_insns),
+ mem_bytes: after.mem_bytes.saturating_sub(self.mem_bytes),
}
}📝 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.
| pub fn delta(&self, after: &GasProfile) -> GasProfile { | |
| GasProfile { | |
| cpu_insns: after.cpu_insns - self.cpu_insns, | |
| mem_bytes: after.mem_bytes - self.mem_bytes, | |
| } | |
| } | |
| pub fn delta(&self, after: &GasProfile) -> GasProfile { | |
| GasProfile { | |
| cpu_insns: after.cpu_insns.saturating_sub(self.cpu_insns), | |
| mem_bytes: after.mem_bytes.saturating_sub(self.mem_bytes), | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/benchmark/profiler.rs` around lines 15 - 20, The delta method in
GasProfile uses plain u64 subtraction and can panic or wrap on underflow; update
GasProfile::delta to use saturating_sub for cpu_insns and mem_bytes so any
negative delta clamps to zero (e.g., replace after.cpu_insns - self.cpu_insns
and after.mem_bytes - self.mem_bytes with saturating_sub calls on those fields),
keeping the function signature and returned GasProfile unchanged.
|
LGTM |
2 participants
closes #275
Summary by CodeRabbit