feat: add admin and session safety controls#125
Merged
Luluameh merged 1 commit intoMar 29, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdded admin-controlled protocol governance with fee management, pause/unpause functionality, and expert reputation tracking. Enhanced session lifecycle with reputation-based gating, automatic expiry enforcement, and new refund mechanism. Refactored authorization checks and dispute resolution to use centralized admin validation, with type adjustments across dispute and upgrade structures. Changes
Sequence Diagram(s)sequenceDiagram
actor Seeker
participant Contract as SkillSphere Contract
participant Storage as Storage (Reputation)
participant Balance as Token Balance
participant Expert
Seeker->>Contract: start_session(expert, min_reputation, rate, amount)
activate Contract
Contract->>Contract: Check: protocol not paused
alt Protocol Paused
Contract-->>Seeker: ProtocolPaused Error
else Proceed
Contract->>Storage: Get expert reputation
activate Storage
Storage-->>Contract: expert_reputation
deactivate Storage
alt Reputation < min_reputation
Contract-->>Seeker: ReputationTooLow Error
else Reputation OK
Contract->>Balance: Lock amount tokens
activate Balance
Balance-->>Contract: Locked ✓
deactivate Balance
Contract->>Storage: Store session with expiry window
Contract-->>Seeker: session_id
end
end
deactivate Contract
rect rgb(100, 150, 200, 0.5)
note over Contract: Later: settle_session
Seeker->>Contract: settle_session(session_id)
activate Contract
Contract->>Contract: Check: protocol not paused
Contract->>Storage: Calculate claimable (with time bounds)
Contract->>Contract: Check: current_time > expiry?
alt Expired & Nothing Claimable
Contract->>Storage: Mark session Finished
Contract-->>Seeker: SessionExpired
else Claimable Funds
Contract->>Balance: Transfer claimable to Expert
Contract-->>Seeker: claimable_amount
end
deactivate Contract
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/src/lib.rs (3)
235-244:⚠️ Potential issue | 🟠 MajorSession is stored before token transfer, risking inconsistent state on transfer failure.
If
token_client.transfer(line 241) fails, the session is already persisted in storage (lines 235-237) withbalance: amount. This creates an orphan session with no actual locked funds.Consider reordering to transfer first:
🐛 Proposed fix: Transfer tokens before storing session
+ token_client.transfer(&seeker, &env.current_contract_address(), &amount); + env.storage() .persistent() .set(&DataKey::Session(session_id), &session); env.events() .publish((symbol_short!("started"),), session_id); - - token_client.transfer(&seeker, &env.current_contract_address(), &amount); Ok(session_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 235 - 244, The session is persisted (env.storage().persistent().set with DataKey::Session(session_id)) and published before attempting token_client.transfer, risking an orphan session if transfer fails; change the order so you call token_client.transfer(&seeker, &env.current_contract_address(), &amount) first and propagate any error, then on successful transfer write the session to storage and publish the (symbol_short!("started"),) event for session_id; ensure the function returns the transfer error if it fails so the storage/publish steps are not executed.
54-60:⚠️ Potential issue | 🟡 MinorInconsistent mapping between
Resolutionenum discriminants and storedresolutionvalues.The
Resolutionenum definesSeekerWins = 0,ExpertWins = 1,Refund = 2, butresolve_dispute(lines 442-446) stores1,2,3respectively. This creates confusion and makes the stored value unusable for reconstructing the original enum.Consider storing the enum discriminant directly:
🔧 Proposed fix to align stored values with enum
dispute.resolution = match resolution { - Resolution::SeekerWins => 1, - Resolution::ExpertWins => 2, - Resolution::Refund => 3, + Resolution::SeekerWins => 0, + Resolution::ExpertWins => 1, + Resolution::Refund => 2, };Or better, store the enum directly if
ResolutionderivesCopy(which it does):pub struct Dispute { // ... pub resolution: Option<Resolution>, // None when unresolved }Also applies to: 70-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 54 - 60, The stored numeric resolution values do not match the Resolution enum discriminants (Resolution: SeekerWins=0, ExpertWins=1, Refund=2 vs resolve_dispute storing 1,2,3), so update the contract to use a consistent representation: change the Dispute struct's resolution field to Option<Resolution> (so unresolved is None and resolved stores the enum directly) and update resolve_dispute to assign the appropriate Resolution variant (SeekerWins/ExpertWins/Refund) instead of hard-coded integers; alternatively, if you must store integers, change the values written in resolve_dispute to use the enum discriminants (0,1,2) to match Resolution. Ensure all code paths that read/write Dispute::resolution and any serialization/de/serialization use the new Option<Resolution> type or the corrected integer mapping to keep reconstruction correct.
282-300:⚠️ Potential issue | 🟡 MinorResuming session after expiry sets incorrect
last_settlement_timestamp.If a session is paused, then time passes beyond the expiry window, and then it's resumed, line 292 sets
last_settlement_timestampto the current unbounded time. This could create incorrect accounting since the session's funded window has already elapsed.Consider bounding the resume timestamp or preventing resume after expiry:
🔧 Proposed fix: Bound resume timestamp or prevent post-expiry resume
pub fn resume_session(env: Env, caller: Address, session_id: u64) -> Result<(), Error> { Self::ensure_protocol_active(&env)?; caller.require_auth(); let mut session = Self::get_session_or_error(&env, session_id)?; Self::require_participant(&session, &caller)?; if session.status != SessionStatus::Paused { return Err(Error::InvalidSessionState); } - session.last_settlement_timestamp = env.ledger().timestamp(); + let now = env.ledger().timestamp(); + let expiry = Self::expiry_timestamp_for_session(&session); + if now >= expiry { + return Err(Error::SessionExpired); + } + session.last_settlement_timestamp = now; session.status = SessionStatus::Active;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 282 - 300, In resume_session, avoid setting last_settlement_timestamp to an unbounded future value by bounding it to the session expiry (or failing resume if expired): fetch now = env.ledger().timestamp(), then if now > session.expiry either return Err(Error::SessionExpired) or set session.last_settlement_timestamp = min(now, session.expiry); keep the rest of resume_session (status change, save_session, publish) unchanged while using the existing session, resume_session, last_settlement_timestamp, and session.expiry identifiers to locate the change.
🧹 Nitpick comments (4)
contracts/src/lib.rs (4)
177-192: Consider adding bounds validation for reputation value.The
set_expert_reputationfunction accepts anyu32value without upper bounds. If reputation is intended to represent a percentage or score (0-100), unbounded values could cause confusion.💡 Optional: Add max reputation validation
+const MAX_REPUTATION: u32 = 100; + pub fn set_expert_reputation(env: Env, expert: Address, reputation: u32) -> Result<(), Error> { Self::require_admin(&env)?; + if reputation > MAX_REPUTATION { + return Err(Error::InvalidAmount); // or add InvalidReputation error + } env.storage() .persistent() .set(&DataKey::ExpertReputation(expert.clone()), &reputation);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 177 - 192, Add an upper-bound check in set_expert_reputation to validate the incoming reputation before writing to storage: ensure the u32 reputation is within the expected range (e.g. 0..=100) and return an Err variant (add or reuse an Error like Error::InvalidReputation) when out of bounds; keep the existing storage write to DataKey::ExpertReputation(expert.clone()) and the publish call (symbol_short!("setReput")) only after the value passes validation.
319-327: ReturningSessionExpirederror after successfully updating session state may confuse callers.Lines 320-324 update the session status to
Finishedand persist it, then returnErr(Error::SessionExpired). While technically correct (no tokens transferred), this pattern is unusual—callers might expect no state change on error.Consider either:
- Returning
Ok(0)with the state change (session naturally expired, no claim available)- Or documenting this behavior clearly
💡 Alternative: Return Ok(0) for natural expiry
if claimable <= 0 { if now > expiry { session.status = SessionStatus::Finished; session.last_settlement_timestamp = expiry; Self::save_session(&env, &session); - return Err(Error::SessionExpired); + env.events().publish((symbol_short!("expired"),), session_id); + return Ok(0); } return Ok(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 319 - 327, The code updates session.status to SessionStatus::Finished, sets session.last_settlement_timestamp, and calls Self::save_session(&env, &session) but then returns Err(Error::SessionExpired), which is surprising; change the return to Ok(0) after persisting the session (in the same branch where claimable <= 0 and now > expiry) so callers observe the state change but receive a successful zero-claim result (adjust any callers/tests if they expected Error::SessionExpired); update the branch handling claimable, now, expiry around variables/methods claimable, now, expiry, session.status, SessionStatus::Finished, session.last_settlement_timestamp, Self::save_session, and remove the Err(Error::SessionExpired) return.
675-686: Theoretical truncation risk in expiry calculation, but practically safe.The cast
funded_seconds as u64(line 681) could theoretically truncate ifbalance/rate_per_secondexceedsu64::MAX(~584 billion years). While practically impossible with realistic token amounts, consider usingtry_into()orsaturating_addfor defensive coding:💡 Optional: Defensive saturating conversion
- let funded_seconds = - ((session.balance + session.rate_per_second - 1) / session.rate_per_second) as u64; + let funded_seconds_i128 = + (session.balance + session.rate_per_second - 1) / session.rate_per_second; + let funded_seconds = if funded_seconds_i128 > u64::MAX as i128 { + u64::MAX + } else { + funded_seconds_i128 as u64 + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 675 - 686, The expiry_timestamp_for_session function casts funded_seconds to u64 which could theoretically overflow when converting from the intermediate integer type; update the conversion to be defensive by using try_into() and handling Err by returning u64::MAX (or use saturating conversion) before calling last_settlement_timestamp.saturating_add, e.g. perform let funded_seconds = ((session.balance + session.rate_per_second - 1) / session.rate_per_second).try_into().unwrap_or(u64::MAX) and then use last_settlement_timestamp.saturating_add(funded_seconds) so the Session overflow is handled safely; refer to expiry_timestamp_for_session, Session and funded_seconds in your change.
917-933: Test name promises to verify settlement blocking but doesn't test it.
test_protocol_pause_blocks_settlement_but_allows_refund_sessiononly tests thatrefund_sessionworks during pause. It doesn't verify thatsettle_sessionis blocked. Consider adding the settlement assertion:🧪 Add assertion for blocked settlement
#[test] +#[should_panic(expected = "Error(Contract, `#12`)")] +fn test_protocol_pause_blocks_settlement() { + let (env, client, _, _, seeker, expert, token, _) = setup(); + let session_id = client.start_session(&seeker, &expert, &token, &10, &500, &0); + + env.ledger().set_timestamp(1_010); + client.pause_protocol(); + + // This should panic with ProtocolPaused error + client.settle_session(&session_id); +} + +#[test] fn test_protocol_pause_blocks_settlement_but_allows_refund_session() {Or rename the existing test to
test_protocol_pause_allows_refund_session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 917 - 933, The test named test_protocol_pause_blocks_settlement_but_allows_refund_session currently only calls client.refund_session and asserts balances; add an explicit check that calling client.settle_session(&seeker, &session_id) while the protocol is paused is rejected: call settle_session after pause (and after refund if you keep the current flow) and assert it returns an error/non-zero code (or Err) and that token balances (token::Client::balance for expert and seeker) remain unchanged; reference client.pause_protocol(), client.settle_session(), client.refund_session(), token::Client::balance(), and client.get_session() to locate where to add the assertion.
🤖 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/src/lib.rs`:
- Around line 133-153: The platform fee is stored by set_fee/get_fee but never
applied: update settle_session, close_session, and resolve_dispute to read the
fee via get_fee (PlatformFeeBps), compute fee_amount = claimable * fee_bps /
MAX_BPS (integer math) and subtract it from amounts sent to the expert
(similarly apply to remaining where applicable), transfer the fee_amount to the
platform/admin account (use the same admin/platform address used elsewhere in
the contract) and transfer the net amount to the expert, and update any emitted
events to reflect both fee and net transfers; ensure you reference and reuse the
existing symbols PlatformFeeBps, get_fee, settle_session, close_session,
resolve_dispute, claimable, remaining, and MAX_BPS.
---
Outside diff comments:
In `@contracts/src/lib.rs`:
- Around line 235-244: The session is persisted (env.storage().persistent().set
with DataKey::Session(session_id)) and published before attempting
token_client.transfer, risking an orphan session if transfer fails; change the
order so you call token_client.transfer(&seeker,
&env.current_contract_address(), &amount) first and propagate any error, then on
successful transfer write the session to storage and publish the
(symbol_short!("started"),) event for session_id; ensure the function returns
the transfer error if it fails so the storage/publish steps are not executed.
- Around line 54-60: The stored numeric resolution values do not match the
Resolution enum discriminants (Resolution: SeekerWins=0, ExpertWins=1, Refund=2
vs resolve_dispute storing 1,2,3), so update the contract to use a consistent
representation: change the Dispute struct's resolution field to
Option<Resolution> (so unresolved is None and resolved stores the enum directly)
and update resolve_dispute to assign the appropriate Resolution variant
(SeekerWins/ExpertWins/Refund) instead of hard-coded integers; alternatively, if
you must store integers, change the values written in resolve_dispute to use the
enum discriminants (0,1,2) to match Resolution. Ensure all code paths that
read/write Dispute::resolution and any serialization/de/serialization use the
new Option<Resolution> type or the corrected integer mapping to keep
reconstruction correct.
- Around line 282-300: In resume_session, avoid setting
last_settlement_timestamp to an unbounded future value by bounding it to the
session expiry (or failing resume if expired): fetch now =
env.ledger().timestamp(), then if now > session.expiry either return
Err(Error::SessionExpired) or set session.last_settlement_timestamp = min(now,
session.expiry); keep the rest of resume_session (status change, save_session,
publish) unchanged while using the existing session, resume_session,
last_settlement_timestamp, and session.expiry identifiers to locate the change.
---
Nitpick comments:
In `@contracts/src/lib.rs`:
- Around line 177-192: Add an upper-bound check in set_expert_reputation to
validate the incoming reputation before writing to storage: ensure the u32
reputation is within the expected range (e.g. 0..=100) and return an Err variant
(add or reuse an Error like Error::InvalidReputation) when out of bounds; keep
the existing storage write to DataKey::ExpertReputation(expert.clone()) and the
publish call (symbol_short!("setReput")) only after the value passes validation.
- Around line 319-327: The code updates session.status to
SessionStatus::Finished, sets session.last_settlement_timestamp, and calls
Self::save_session(&env, &session) but then returns Err(Error::SessionExpired),
which is surprising; change the return to Ok(0) after persisting the session (in
the same branch where claimable <= 0 and now > expiry) so callers observe the
state change but receive a successful zero-claim result (adjust any
callers/tests if they expected Error::SessionExpired); update the branch
handling claimable, now, expiry around variables/methods claimable, now, expiry,
session.status, SessionStatus::Finished, session.last_settlement_timestamp,
Self::save_session, and remove the Err(Error::SessionExpired) return.
- Around line 675-686: The expiry_timestamp_for_session function casts
funded_seconds to u64 which could theoretically overflow when converting from
the intermediate integer type; update the conversion to be defensive by using
try_into() and handling Err by returning u64::MAX (or use saturating conversion)
before calling last_settlement_timestamp.saturating_add, e.g. perform let
funded_seconds = ((session.balance + session.rate_per_second - 1) /
session.rate_per_second).try_into().unwrap_or(u64::MAX) and then use
last_settlement_timestamp.saturating_add(funded_seconds) so the Session overflow
is handled safely; refer to expiry_timestamp_for_session, Session and
funded_seconds in your change.
- Around line 917-933: The test named
test_protocol_pause_blocks_settlement_but_allows_refund_session currently only
calls client.refund_session and asserts balances; add an explicit check that
calling client.settle_session(&seeker, &session_id) while the protocol is paused
is rejected: call settle_session after pause (and after refund if you keep the
current flow) and assert it returns an error/non-zero code (or Err) and that
token balances (token::Client::balance for expert and seeker) remain unchanged;
reference client.pause_protocol(), client.settle_session(),
client.refund_session(), token::Client::balance(), and client.get_session() to
locate where to add the assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+133
to
+153
| pub fn set_fee(env: Env, fee_bps: u32) -> Result<(), Error> { | ||
| Self::require_admin(&env)?; | ||
|
|
||
| if fee_bps > MAX_BPS { | ||
| return Err(Error::InvalidFeeBps); | ||
| } | ||
|
|
||
| env.storage() | ||
| .instance() | ||
| .set(&DataKey::PlatformFeeBps, &fee_bps); | ||
| env.events().publish((symbol_short!("setFee"),), fee_bps); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn get_fee(env: Env) -> u32 { | ||
| env.storage() | ||
| .instance() | ||
| .get(&DataKey::PlatformFeeBps) | ||
| .unwrap_or(0u32) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any usage of PlatformFeeBps or get_fee in settlement/transfer logic
rg -n 'PlatformFeeBps|get_fee' contracts/src/lib.rs | grep -v 'set_fee\|DataKey::PlatformFeeBps'Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 185
🏁 Script executed:
# Search for settle_session, close_session, and resolve_dispute function implementations
rg -n 'fn settle_session|fn close_session|fn resolve_dispute' contracts/src/lib.rs -A 20Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 2724
🏁 Script executed:
# Check if there are any arithmetic operations or divisions that might apply fees
rg -n 'divide|multiply|fee|percent' contracts/src/lib.rs -i | head -30Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 681
🏁 Script executed:
# Search for token transfers and where balances are actually transferred
rg -n 'token_client\|transfer\|balance.*=' contracts/src/lib.rs | head -40Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 56
🏁 Script executed:
# Get all references to PlatformFeeBps across the entire file
rg -n 'PlatformFeeBps' contracts/src/lib.rsRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 256
🏁 Script executed:
# Get the full settle_session and close_session implementations to see token transfers
sed -n '302,422p' contracts/src/lib.rsRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 3732
🏁 Script executed:
# Check close_session fully
sed -n '613,660p' contracts/src/lib.rsRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 1717
Platform fee is stored but never deducted from settlements.
The set_fee and get_fee functions exist, but PlatformFeeBps is never read in settle_session, close_session, or resolve_dispute. In settle_session (line 350), the full claimable amount is transferred to the expert without any fee deduction. Similarly, in close_session (lines 644, 649), both claimable and remaining are transferred in full. The fee has no effect on any token transfers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lib.rs` around lines 133 - 153, The platform fee is stored by
set_fee/get_fee but never applied: update settle_session, close_session, and
resolve_dispute to read the fee via get_fee (PlatformFeeBps), compute fee_amount
= claimable * fee_bps / MAX_BPS (integer math) and subtract it from amounts sent
to the expert (similarly apply to remaining where applicable), transfer the
fee_amount to the platform/admin account (use the same admin/platform address
used elsewhere in the contract) and transfer the net amount to the expert, and
update any emitted events to reflect both fee and net transfers; ensure you
reference and reuse the existing symbols PlatformFeeBps, get_fee,
settle_session, close_session, resolve_dispute, claimable, remaining, and
MAX_BPS.
7 tasks
This was referenced Apr 26, 2026
This was referenced May 28, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements the contract-side protocol safety controls requested across the admin, reputation, funding window, and emergency shutdown issues in one coordinated Soroban update.
Closes #116
Closes #117
Closes #118
Closes #119
Changes proposed
What were you told to do?
Implement multi-sig-ready admin controls for protocol parameters, enforce an expert reputation threshold during session creation, terminate sessions cleanly once their funded window is exhausted, and add an admin-controlled circuit breaker that blocks risky actions while still allowing refunds.
What did I do?
Admin and protocol controls
set_admin,get_admin,set_fee, andget_feeplus shared admin guard helpers.pause_protocol,unpause_protocol, andis_protocol_pausedto freeze sensitive flows.Reputation and session funding guards
set_expert_reputationandget_expert_reputation.start_sessionwith amin_reputationrequirement and rejects experts that do not meet the seeker threshold.calculate_expiry_timestampplus bounded settlement logic so sessions settle only within their funded window and auto-finish when fully drained.Refund and regression coverage
refund_sessionso seekers can recover unused funds even while the protocol is paused, while still paying experts for accrued work.Check List (Check all the applicable boxes)
Screenshots / Testing Evidence
Validated the Soroban contract with
cargo fmtandcargo testincontracts; all 15 contract tests passed, including new coverage for admin handoff and fee storage, expert reputation enforcement, funded-window expiry, protocol pause behavior, and refunds while paused.Summary by CodeRabbit
New Features
Bug Fixes