feat: implement core smart contract features#167
Merged
Luluameh merged 1 commit intoApr 27, 2026
Merged
Conversation
Add four essential smart contract functions for the expert consultation platform: - Partial Withdrawals (LightForgeHub#162): withdraw_accrued() allows experts to claim accrued funds mid-session without closing it, updating session state and tracking claimable amounts based on elapsed time and expert rate. - Staking Mechanism (LightForgeHub#163): stake_tokens() and unstake_tokens() enable experts to stake XLM/USDC for visibility boost, with events emitted for frontend indexer to update expert rankings based on stake tier. - Multi-Sig Arbitration (LightForgeHub#164): initialize_arbitration_committee() and propose_resolution() implement 2-of-3 multisig governance for dispute resolution, with committee member storage and proposal tracking. - Escrow Slashing (LightForgeHub#165): slash_expert() allows arbitration committee to penalize malicious experts by transferring staked tokens to treasury, with audit logging and balance tracking. All implementations include proper authorization checks, error handling, event emission for indexing, and state persistence via Soroban SDK storage. Refs LightForgeHub#162 LightForgeHub#163 LightForgeHub#164 LightForgeHub#165
📝 WalkthroughWalkthroughThis pull request introduces four new on-chain features to the smart contract: a mid-session withdrawal mechanism allowing experts to collect accrued proceeds while maintaining active sessions, token staking and unstaking capabilities for experts managed through the contract vault, a persistent three-member arbitration committee structure, and expert token slashing functionality that redirects slashed amounts to a treasury while tracking state changes. Changes
Sequence DiagramsequenceDiagram
participant Expert
participant Contract
participant Committee
participant Treasury
Expert->>Contract: stake_tokens(amount)
Contract->>Contract: Transfer tokens to vault
Contract->>Contract: Increment staked_balance
Note over Expert,Contract: Session Active
Expert->>Contract: withdraw_accrued(session_id)
Contract->>Contract: Calculate accrued amount
Contract->>Expert: Transfer accrued + newly_accrued
Contract->>Contract: Update last_settlement_timestamp
Note over Committee,Contract: If Dispute Occurs
Committee->>Contract: initialize_arbitration_committee(3 members)
Contract->>Contract: Store committee addresses
Committee->>Contract: propose_resolution(session_id, seeker_award_bps)
Contract->>Contract: Validate & emit proposal event
alt Expert Misbehavior
Committee->>Contract: slash_expert(expert_id, amount, reason)
Contract->>Treasury: Transfer slashed tokens
Contract->>Contract: Decrement staked_balance
Contract->>Contract: Emit audit event
end
Expert->>Contract: unstake_tokens(amount)
Contract->>Expert: Transfer remaining staked tokens
Contract->>Contract: Decrement staked_balance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: 5
🧹 Nitpick comments (2)
contracts/src/lib.rs (2)
1520-1522: Nit: preferis_empty()overlen() == 0.Idiomatic Rust and matches the style used elsewhere (e.g., Line 778:
if reason.is_empty()).- if reason.len() == 0 { + if reason.is_empty() { return Err(Error::EmptyDisputeReason); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 1520 - 1522, Replace the length check using reason.len() == 0 with the idiomatic is_empty() call to match project style: change the condition to if reason.is_empty() so the branch that returns Err(Error::EmptyDisputeReason) keeps the same behavior but uses the preferred Rust API for the variable reason.
1355-1563: Recommended: add tests for the four new features.The test module below already covers staking-tier fee math, treasury flows, sessions, and disputes, but there are zero tests for
withdraw_accrued,stake_tokens/unstake_tokens,initialize_arbitration_committee,propose_resolution, orslash_expert. Given the bugs flagged above (token misuse, storage-key divergence, missing multisig), any of these will silently regress without coverage. At minimum, add:
withdraw_accrued: happy path, non-expert caller (Unauthorized), inactive session (InvalidSessionState), insufficient balance, repeated calls update timestamp.stake_tokens/unstake_tokens: tier transitions verified viaget_expert_fee_bps, full unstake returns tokens, partial unstake retains tier.initialize_arbitration_committee: re-init rejected, non-admin rejected.slash_expert: stake decreases, treasury balance increases as observed byget_treasury_balance, non-admin rejected.I can scaffold these tests once the underlying bugs above are addressed — let me know.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 1355 - 1563, Add comprehensive unit tests for the new features: write tests for withdraw_accrued covering happy path, Unauthorized (non-expert caller), InvalidSessionState (inactive session), InsufficientBalance, and repeated calls updating last_settlement_timestamp; tests should exercise the withdraw_accrued function and inspect session fields (balance, accrued_amount, last_settlement_timestamp) stored under DataKey::Session. Add stake_tokens and unstake_tokens tests that assert token transfers occur and that get_expert_fee_bps reflects tier transitions for full/partial unstake, and that full unstake returns tokens to expert; reference stake_tokens, unstake_tokens, and get_expert_fee_bps. Add tests for initialize_arbitration_committee to assert admin-only access and that re-initialization is rejected and that committee is persisted under DataKey::ArbitrationCommittee. Add propose_resolution test to ensure a proposal event is emitted and invalid bps is rejected. Add slash_expert tests validating admin/committee authorization (non-admin rejected), staked_balance decreases for the expert, treasury balance increases as observed via get_treasury_balance or DataKey::TreasuryBalance, and that events are emitted; reference slash_expert, get_treasury_address, and DataKey::TreasuryBalance. Ensure tests set up token balances and use token::Client mocks or test ledger to verify transfers.
🤖 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 1484-1503: propose_resolution currently only logs an event and
must be converted into the first step of a 2-of-3 multisig: verify caller is a
member of ArbitrationCommittee (use the existing ArbitrationCommittee
storage/lookup), fetch the dispute via get_dispute and assert it is not already
resolved (rename the _dispute binding to dispute), persist a Proposal struct
containing session_id, seeker_award_bps and approvals: Vec<Address> (initially
containing the proposer), and do NOT call resolve_dispute_with_split yet;
implement a companion approve_resolution function that: validates caller is a
distinct ArbitrationCommittee member, loads the Proposal, adds the caller to
approvals if not already present, persists the updated Proposal, and when
approvals.len() >= 2 calls resolve_dispute_with_split(session_id,
seeker_award_bps) and clears the Proposal entry. Ensure uniqueness of approvers
and proper storage key for Proposal so both functions operate on the same
proposal record.
- Around line 1461-1481: initialize_arbitration_committee currently allows
silent re-initialization, accepts duplicate members, and emits no event; update
initialize_arbitration_committee to first check
env.storage().persistent().has(&DataKey::ArbitrationCommittee) and return an
error (reuse Error::Unauthorized or add Error::AlreadyInitialized) to prevent
re-initialization, validate that member1, member2, member3 are pairwise distinct
(return Error::InvalidInput or similar if duplicates), and write an on-chain
event/log after successful set so tooling (e.g., propose_resolution indexing)
can detect committee changes; alternatively split into a protected
rotate_committee function that requires existing committee approval if rotation
is desired.
- Around line 1355-1399: This function mixes u64/u32 timestamps, bypasses
fees/referrer/treasury logic, and omits the protocol-active guard—fix by (1)
calling Self::ensure_protocol_active(&env) at the top, (2) compute claimable
using the existing helper (e.g., call or mirror claimable_amount_for_session /
bounded_time so time_elapsed is correctly bounded) instead of manual math, (3)
reuse the internal_settle logic (or its sub-steps: calculate_platform_fee,
pay_referrer, route treasury) so platform fee/referrer/dust cleanup are applied
rather than transferring full gross amount, and (4) normalize types by casting
env.ledger().timestamp() to u32 where you store it (or cast
session.last_settlement_timestamp to u64 for math) and update
session.last_settlement_timestamp using the correct u32 cast; ensure all
references to session.last_settlement_timestamp and now use consistent types.
- Around line 1403-1457: stake_tokens and unstake_tokens use
env.current_contract_address() as the token client (wrong token) and update only
ExpertProfile.staked_balance while get_expert_fee_bps reads
DataKey::ExpertStakedBalance, causing state divergence and broken fee tiers; fix
by resolving the actual staking token source (use the existing
StakingContract/DataKey or the same token parameter pattern used in
start_session or the contract address stored by set_staking_contract) when
constructing token::Client (replace token = env.current_contract_address() with
the correct staking token reference), and atomically synchronize updates to both
storage locations (set DataKey::ExpertStakedBalance and
ExpertProfile.staked_balance together inside stake_tokens and unstake_tokens, or
consolidate to a single canonical storage key used by get_expert_fee_bps); also
change event topics to the existing symbol style (e.g., symbol_short!("staked")
/ symbol_short!("unstaked")) to match the rest of the contract.
- Around line 1507-1563: slash_expert has five bugs: use the staking token,
persist treasury accounting in the same storage, update the separate
expert-stake key, allow arbitration committee members, and return a clear error
when the treasury address is missing. Fix it by 1) using
Self::get_staking_contract() as the token address when constructing
token::Client and calling transfer (instead of env.current_contract_address());
2) update treasury bookkeeping through the same persistent storage and/or
existing helpers (get_treasury_balance, deposit_to_treasury, withdraw_treasury)
and write DataKey::TreasuryBalance into persistent storage (not instance
storage) so other methods see slashed funds; 3) decrement the expert stake both
on the ExpertProfile (profile.staked_balance) and the
DataKey::ExpertStakedBalance(expert_id) persistent key so
get_expert_staked_balance and fee-tier logic reflect the slash; 4) expand
authorization to allow either admin OR a member of the arbitration committee
stored by initialize_arbitration_committee (check the committee membership
storage/set before returning Unauthorized); and 5) add and return a clear error
variant (e.g., Error::TreasuryNotConfigured) when get_treasury_address(&env)
returns None, and use that instead of InsufficientTreasuryBalance.
---
Nitpick comments:
In `@contracts/src/lib.rs`:
- Around line 1520-1522: Replace the length check using reason.len() == 0 with
the idiomatic is_empty() call to match project style: change the condition to if
reason.is_empty() so the branch that returns Err(Error::EmptyDisputeReason)
keeps the same behavior but uses the preferred Rust API for the variable reason.
- Around line 1355-1563: Add comprehensive unit tests for the new features:
write tests for withdraw_accrued covering happy path, Unauthorized (non-expert
caller), InvalidSessionState (inactive session), InsufficientBalance, and
repeated calls updating last_settlement_timestamp; tests should exercise the
withdraw_accrued function and inspect session fields (balance, accrued_amount,
last_settlement_timestamp) stored under DataKey::Session. Add stake_tokens and
unstake_tokens tests that assert token transfers occur and that
get_expert_fee_bps reflects tier transitions for full/partial unstake, and that
full unstake returns tokens to expert; reference stake_tokens, unstake_tokens,
and get_expert_fee_bps. Add tests for initialize_arbitration_committee to assert
admin-only access and that re-initialization is rejected and that committee is
persisted under DataKey::ArbitrationCommittee. Add propose_resolution test to
ensure a proposal event is emitted and invalid bps is rejected. Add slash_expert
tests validating admin/committee authorization (non-admin rejected),
staked_balance decreases for the expert, treasury balance increases as observed
via get_treasury_balance or DataKey::TreasuryBalance, and that events are
emitted; reference slash_expert, get_treasury_address, and
DataKey::TreasuryBalance. Ensure tests set up token balances and use
token::Client mocks or test ledger to verify transfers.
🪄 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
+1355
to
+1399
| // ===== Issue #162: Partial Withdrawals for Long Sessions ===== | ||
| /// Allow experts to withdraw accrued funds mid-session without closing it | ||
| pub fn withdraw_accrued(env: Env, caller: Address, session_id: u64) -> Result<i128, Error> { | ||
| caller.require_auth(); | ||
| let mut session = Self::get_session_or_error(&env, session_id)?; | ||
|
|
||
| // Verify caller is the expert | ||
| if session.expert != caller { | ||
| return Err(Error::Unauthorized); | ||
| } | ||
|
|
||
| // Verify session is active | ||
| if session.status != SessionStatus::Active { | ||
| return Err(Error::InvalidSessionState); | ||
| } | ||
|
|
||
| // Calculate currently claimable amount based on time elapsed | ||
| let now = env.ledger().timestamp(); | ||
| let time_elapsed = now.saturating_sub(session.last_settlement_timestamp); | ||
| let newly_accrued = session.rate_per_second.saturating_mul(time_elapsed as i128); | ||
|
|
||
| // Total claimable is accrued + newly accrued | ||
| let total_claimable = session.accrued_amount.saturating_add(newly_accrued); | ||
|
|
||
| if total_claimable <= 0 { | ||
| return Err(Error::InvalidAmount); | ||
| } | ||
|
|
||
| // Verify session has sufficient balance | ||
| if session.balance < total_claimable { | ||
| return Err(Error::InsufficientBalance); | ||
| } | ||
|
|
||
| // Transfer tokens to expert | ||
| let token_client = token::Client::new(&env, &session.token); | ||
| token_client.transfer(&env.current_contract_address(), &caller, &total_claimable); | ||
|
|
||
| // Update session state | ||
| session.balance = session.balance.saturating_sub(total_claimable); | ||
| session.last_settlement_timestamp = now; | ||
| session.accrued_amount = 0; | ||
| env.storage().persistent().set(&DataKey::Session(session_id), &session); | ||
|
|
||
| Ok(total_claimable) | ||
| } |
There was a problem hiding this comment.
Critical: type mismatches and bypassed platform fees in withdraw_accrued.
Several blocking issues in this function:
- Won't compile (type mismatch).
session.last_settlement_timestampisu32, butnow = env.ledger().timestamp()isu64. Both Line 1373 (now.saturating_sub(session.last_settlement_timestamp)) and Line 1394 (session.last_settlement_timestamp = now;) mixu64andu32directly. The rest of the codebase consistently casts (e.g., Lines 580, 645). - Bypasses platform fee, referrer reward, and dust cleanup.
internal_settle(Lines 998–1084) deductscalculate_platform_fee, pays the referrer, and routes to treasury.withdraw_accruedtransfers the full claimable amount directly to the expert, opening an obvious fee-avoidance path: experts can callwithdraw_accruedrepeatedly instead ofsettle_session. Per Issue#162, mid-session withdrawals should still respect fee economics. - Missing protocol-pause guard. Other settlement-style operations (
resume_session,settle_session,batch_settle) callSelf::ensure_protocol_active(&env)first; this one does not, so withdrawals can drain the contract while the protocol is paused. - No expiry bounding. Compare with
bounded_time/claimable_amount_for_session. The localbalance < total_claimablecheck below saves you from over-paying, buttime_elapsedwill keep growing past expiry, masking the correct settlement timestamp on the session record.
🛠️ Suggested refactor — reuse settlement helpers
- pub fn withdraw_accrued(env: Env, caller: Address, session_id: u64) -> Result<i128, Error> {
- caller.require_auth();
- let mut session = Self::get_session_or_error(&env, session_id)?;
-
- // Verify caller is the expert
- if session.expert != caller {
- return Err(Error::Unauthorized);
- }
-
- // Verify session is active
- if session.status != SessionStatus::Active {
- return Err(Error::InvalidSessionState);
- }
-
- // Calculate currently claimable amount based on time elapsed
- let now = env.ledger().timestamp();
- let time_elapsed = now.saturating_sub(session.last_settlement_timestamp);
- let newly_accrued = session.rate_per_second.saturating_mul(time_elapsed as i128);
-
- // Total claimable is accrued + newly accrued
- let total_claimable = session.accrued_amount.saturating_add(newly_accrued);
-
- if total_claimable <= 0 {
- return Err(Error::InvalidAmount);
- }
-
- // Verify session has sufficient balance
- if session.balance < total_claimable {
- return Err(Error::InsufficientBalance);
- }
-
- // Transfer tokens to expert
- let token_client = token::Client::new(&env, &session.token);
- token_client.transfer(&env.current_contract_address(), &caller, &total_claimable);
-
- // Update session state
- session.balance = session.balance.saturating_sub(total_claimable);
- session.last_settlement_timestamp = now;
- session.accrued_amount = 0;
- env.storage().persistent().set(&DataKey::Session(session_id), &session);
-
- Ok(total_claimable)
- }
+ pub fn withdraw_accrued(env: Env, caller: Address, session_id: u64) -> Result<i128, Error> {
+ Self::ensure_protocol_active(&env)?;
+ caller.require_auth();
+ let session = Self::get_session_or_error(&env, session_id)?;
+ if session.expert != caller {
+ return Err(Error::Unauthorized);
+ }
+ if session.status != SessionStatus::Active {
+ return Err(Error::InvalidSessionState);
+ }
+ // Reuse the audited settlement path so platform fee, referrer reward,
+ // dust cleanup, and timestamp bounding all apply identically.
+ Self::internal_settle(&env, 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 1355 - 1399, This function mixes u64/u32
timestamps, bypasses fees/referrer/treasury logic, and omits the protocol-active
guard—fix by (1) calling Self::ensure_protocol_active(&env) at the top, (2)
compute claimable using the existing helper (e.g., call or mirror
claimable_amount_for_session / bounded_time so time_elapsed is correctly
bounded) instead of manual math, (3) reuse the internal_settle logic (or its
sub-steps: calculate_platform_fee, pay_referrer, route treasury) so platform
fee/referrer/dust cleanup are applied rather than transferring full gross
amount, and (4) normalize types by casting env.ledger().timestamp() to u32 where
you store it (or cast session.last_settlement_timestamp to u64 for math) and
update session.last_settlement_timestamp using the correct u32 cast; ensure all
references to session.last_settlement_timestamp and now use consistent types.
Comment on lines
+1403
to
+1457
| pub fn stake_tokens(env: Env, expert: Address, amount: i128) -> Result<(), Error> { | ||
| expert.require_auth(); | ||
|
|
||
| if amount <= 0 { | ||
| return Err(Error::InvalidAmount); | ||
| } | ||
|
|
||
| // Get expert profile | ||
| let mut profile = Self::expert_profile(&env, expert.clone()); | ||
|
|
||
| // Transfer tokens from expert to contract | ||
| let token = env.current_contract_address(); // Using contract address as staking vault | ||
| let token_client = token::Client::new(&env, &token); | ||
| token_client.transfer(&expert, &env.current_contract_address(), &amount); | ||
|
|
||
| // Update staked balance | ||
| profile.staked_balance = profile.staked_balance.saturating_add(amount); | ||
| env.storage().persistent().set(&DataKey::ExpertProfile(expert.clone()), &profile); | ||
|
|
||
| // Emit event for frontend indexer | ||
| env.events().publish(("expert", "staked"), (&expert, &amount)); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Allows experts to withdraw staked tokens | ||
| pub fn unstake_tokens(env: Env, expert: Address, amount: i128) -> Result<(), Error> { | ||
| expert.require_auth(); | ||
|
|
||
| if amount <= 0 { | ||
| return Err(Error::InvalidAmount); | ||
| } | ||
|
|
||
| // Get expert profile | ||
| let mut profile = Self::expert_profile(&env, expert.clone()); | ||
|
|
||
| // Verify expert has sufficient staked balance | ||
| if profile.staked_balance < amount { | ||
| return Err(Error::InsufficientBalance); | ||
| } | ||
|
|
||
| // Transfer tokens back to expert | ||
| let token = env.current_contract_address(); | ||
| let token_client = token::Client::new(&env, &token); | ||
| token_client.transfer(&env.current_contract_address(), &expert, &amount); | ||
|
|
||
| // Update staked balance | ||
| profile.staked_balance = profile.staked_balance.saturating_sub(amount); | ||
| env.storage().persistent().set(&DataKey::ExpertProfile(expert.clone()), &profile); | ||
|
|
||
| // Emit event for frontend indexer | ||
| env.events().publish(("expert", "unstaked"), (&expert, &amount)); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there is no separate staking-token configuration we missed,
# and that ExpertStakedBalance is the only key consulted for fee tiers.
rg -nP --type=rust -C2 '\bExpertStakedBalance\b'
rg -nP --type=rust -C2 '\bStakingContract\b'
rg -nP --type=rust -C2 'staked_balance'Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 10603
🏁 Script executed:
# Get the full implementation of stake_tokens and unstake_tokens, plus surrounding context
sed -n '1403,1457p' contracts/src/lib.rs | cat -nRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 2494
🏁 Script executed:
# Check if there's any code that synchronizes the two staked_balance locations
rg -n 'profile\.staked_balance' contracts/src/lib.rs -A2 -B2Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 1172
🏁 Script executed:
# Check if ExpertProfile.staked_balance is used anywhere for fee calculations or tier checking
rg -n 'profile\.staked_balance' contracts/src/lib.rs | head -30Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 429
🏁 Script executed:
# Check what token address is actually being used in the functions (search for the exact lines)
sed -n '1408,1420p' contracts/src/lib.rs
sed -n '1441,1453p' contracts/src/lib.rsRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 1229
🏁 Script executed:
# Check all places that write to ExpertStakedBalance to confirm only set_expert_staked_balance does it
rg -n 'ExpertStakedBalance' contracts/src/lib.rs | grep -v '//'Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 234
🏁 Script executed:
# Verify the token parameter usage in other functions like start_session (mentioned in review)
sed -n '570,590p' contracts/src/lib.rs | cat -nRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 995
🏁 Script executed:
# Check the ExpertProfile struct definition
sed -n '110,125p' contracts/src/lib.rs | cat -nRepository: LightForgeHub/SkillSphere-Dapp
Length of output: 615
Critical: staking is broken — wrong token address and separate staked balance tracking.
Two independent bugs prevent these functions from working correctly:
-
env.current_contract_address()is not a token. Lines 1412 and 1443 setlet token = env.current_contract_address();and constructtoken::Client::new(&env, &token). This dispatches SAC calls to this contract, which does not implement the token transfer interface. The staking token must come from a parameter (as instart_sessionat line 573), aStakingTokenDataKey, or theStakingContractconfigured byset_staking_contract(line 298). -
State divergence: stake updates do not affect fee tiers.
stake_tokensmutatesprofile.staked_balancein theExpertProfilestorage key (line 1419), butget_expert_fee_bps(line 339) reads fromDataKey::ExpertStakedBalance, which is only populated byset_expert_staked_balance. An expert usingstake_tokenswill not receive Tier 1/2/3 fee reductions. Either consolidate to a single storage source or synchronize both atomically. Same applies tounstake_tokens.
Consider using a separate "staking" event topic (e.g., symbol_short!("staked")) for consistency with the rest of the file rather than the string-tuple form.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lib.rs` around lines 1403 - 1457, stake_tokens and
unstake_tokens use env.current_contract_address() as the token client (wrong
token) and update only ExpertProfile.staked_balance while get_expert_fee_bps
reads DataKey::ExpertStakedBalance, causing state divergence and broken fee
tiers; fix by resolving the actual staking token source (use the existing
StakingContract/DataKey or the same token parameter pattern used in
start_session or the contract address stored by set_staking_contract) when
constructing token::Client (replace token = env.current_contract_address() with
the correct staking token reference), and atomically synchronize updates to both
storage locations (set DataKey::ExpertStakedBalance and
ExpertProfile.staked_balance together inside stake_tokens and unstake_tokens, or
consolidate to a single canonical storage key used by get_expert_fee_bps); also
change event topics to the existing symbol style (e.g., symbol_short!("staked")
/ symbol_short!("unstaked")) to match the rest of the contract.
Comment on lines
+1461
to
+1481
| pub fn initialize_arbitration_committee( | ||
| env: Env, | ||
| member1: Address, | ||
| member2: Address, | ||
| member3: Address, | ||
| ) -> Result<(), Error> { | ||
| // Only admin can initialize | ||
| let admin = Self::get_admin(&env)?; | ||
| admin.require_auth(); | ||
|
|
||
| // Store committee members in persistent state | ||
| // Using a vector to store the committee members | ||
| let mut committee: Vec<Address> = Vec::new(&env); | ||
| committee.push_back(member1); | ||
| committee.push_back(member2); | ||
| committee.push_back(member3); | ||
|
|
||
| env.storage().persistent().set(&DataKey::ArbitrationCommittee, &committee); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Major: committee can be silently re-initialized; no duplicate-member or event guard.
- No "already initialized" check. Compare with
initialize(Lines 152–154). An admin (or compromised admin) can replace the committee at any time, defeating the multisig safety property. Addif env.storage().persistent().has(&DataKey::ArbitrationCommittee) { return Err(Error::Unauthorized); }(or a dedicated error variant) and/or expose a separaterotate_committeeflow that requires existing committee approval. - No check that
member1,member2,member3are distinct. If two (or three) addresses are equal, "2-of-3" collapses to "1-of-1" / "1-of-2". - No event emitted;
propose_resolutionand downstream tooling can't index committee changes.
🛠️ Suggested fix
pub fn initialize_arbitration_committee(
env: Env,
member1: Address,
member2: Address,
member3: Address,
) -> Result<(), Error> {
- // Only admin can initialize
let admin = Self::get_admin(&env)?;
admin.require_auth();
+
+ if env.storage().persistent().has(&DataKey::ArbitrationCommittee) {
+ return Err(Error::InvalidSessionState); // or a dedicated error
+ }
+ if member1 == member2 || member1 == member3 || member2 == member3 {
+ return Err(Error::InvalidReferrer); // or a dedicated error
+ }
- // Store committee members in persistent state
- // Using a vector to store the committee members
let mut committee: Vec<Address> = Vec::new(&env);
committee.push_back(member1);
committee.push_back(member2);
committee.push_back(member3);
env.storage().persistent().set(&DataKey::ArbitrationCommittee, &committee);
+ env.events().publish((symbol_short!("arbInit"),), committee);
Ok(())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lib.rs` around lines 1461 - 1481,
initialize_arbitration_committee currently allows silent re-initialization,
accepts duplicate members, and emits no event; update
initialize_arbitration_committee to first check
env.storage().persistent().has(&DataKey::ArbitrationCommittee) and return an
error (reuse Error::Unauthorized or add Error::AlreadyInitialized) to prevent
re-initialization, validate that member1, member2, member3 are pairwise distinct
(return Error::InvalidInput or similar if duplicates), and write an on-chain
event/log after successful set so tooling (e.g., propose_resolution indexing)
can detect committee changes; alternatively split into a protected
rotate_committee function that requires existing committee approval if rotation
is desired.
Comment on lines
+1484
to
+1503
| pub fn propose_resolution( | ||
| env: Env, | ||
| caller: Address, | ||
| session_id: u64, | ||
| seeker_award_bps: u32, | ||
| ) -> Result<(), Error> { | ||
| caller.require_auth(); | ||
|
|
||
| if seeker_award_bps > MAX_BPS { | ||
| return Err(Error::InvalidSplitBps); | ||
| } | ||
|
|
||
| // Verify dispute exists | ||
| let _dispute = Self::get_dispute(&env, session_id)?; | ||
|
|
||
| // In a real implementation, this would store the proposal for other members to approve | ||
| env.events().publish(("resolution", "proposed"), (&session_id, &seeker_award_bps)); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Critical: propose_resolution does not implement the 2-of-3 multisig described in #164.
The function only authenticates the caller and emits an event — there is no committee membership check, no proposal record persisted, and no companion approve_resolution flow that counts approvals and triggers resolve_dispute_with_split. As written, any address can call this and nothing changes on-chain. Per the PR objectives this should: (1) verify caller is in ArbitrationCommittee, (2) persist a Proposal { session_id, seeker_award_bps, approvals: Vec<Address> }, and (3) once a second distinct committee member calls approve_resolution, invoke the existing resolve_dispute_with_split helper.
Also: the _dispute binding on Line 1497 is fetched only to validate existence; consider renaming or asserting !dispute.resolved to avoid acting on already-resolved disputes.
Want me to draft the full proposal/approval state machine (storage key, propose_resolution, approve_resolution, integration with resolve_dispute_with_split)? Happy to open it as a follow-up issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lib.rs` around lines 1484 - 1503, propose_resolution currently
only logs an event and must be converted into the first step of a 2-of-3
multisig: verify caller is a member of ArbitrationCommittee (use the existing
ArbitrationCommittee storage/lookup), fetch the dispute via get_dispute and
assert it is not already resolved (rename the _dispute binding to dispute),
persist a Proposal struct containing session_id, seeker_award_bps and approvals:
Vec<Address> (initially containing the proposer), and do NOT call
resolve_dispute_with_split yet; implement a companion approve_resolution
function that: validates caller is a distinct ArbitrationCommittee member, loads
the Proposal, adds the caller to approvals if not already present, persists the
updated Proposal, and when approvals.len() >= 2 calls
resolve_dispute_with_split(session_id, seeker_award_bps) and clears the Proposal
entry. Ensure uniqueness of approvers and proper storage key for Proposal so
both functions operate on the same proposal record.
Comment on lines
+1507
to
+1563
| pub fn slash_expert( | ||
| env: Env, | ||
| caller: Address, | ||
| expert_id: Address, | ||
| amount: i128, | ||
| reason: String, | ||
| ) -> Result<(), Error> { | ||
| caller.require_auth(); | ||
|
|
||
| if amount <= 0 { | ||
| return Err(Error::InvalidAmount); | ||
| } | ||
|
|
||
| if reason.len() == 0 { | ||
| return Err(Error::EmptyDisputeReason); | ||
| } | ||
|
|
||
| // Verify caller is admin or arbitration committee member | ||
| let admin = Self::get_admin(&env)?; | ||
| if caller != admin { | ||
| return Err(Error::Unauthorized); | ||
| } | ||
|
|
||
| // Get expert profile | ||
| let mut profile = Self::expert_profile(&env, expert_id.clone()); | ||
|
|
||
| // Verify expert has sufficient staked balance | ||
| if profile.staked_balance < amount { | ||
| return Err(Error::InsufficientBalance); | ||
| } | ||
|
|
||
| // Get treasury address | ||
| let treasury = Self::get_treasury_address(&env) | ||
| .ok_or(Error::InsufficientTreasuryBalance)?; | ||
|
|
||
| // Transfer slashed tokens to treasury | ||
| let token = env.current_contract_address(); | ||
| let token_client = token::Client::new(&env, &token); | ||
| token_client.transfer(&env.current_contract_address(), &treasury, &amount); | ||
|
|
||
| // Deduct from expert's staked balance | ||
| profile.staked_balance = profile.staked_balance.saturating_sub(amount); | ||
| env.storage().persistent().set(&DataKey::ExpertProfile(expert_id.clone()), &profile); | ||
|
|
||
| // Update treasury balance tracking | ||
| let treasury_key = DataKey::TreasuryBalance(token); | ||
| let mut treasury_balance: i128 = env.storage().instance() | ||
| .get(&treasury_key) | ||
| .unwrap_or(0); | ||
| treasury_balance = treasury_balance.saturating_add(amount); | ||
| env.storage().instance().set(&treasury_key, &treasury_balance); | ||
|
|
||
| // Emit event for auditing | ||
| env.events().publish(("expert", "slashed"), (&expert_id, &amount, &reason)); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify treasury read path uses persistent storage exclusively,
# so the instance-storage write in slash_expert is provably invisible.
rg -nP --type=rust -C2 'DataKey::TreasuryBalance'Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 1560
🏁 Script executed:
# Check what the actual staking token contract should be
rg -nP --type=rust -C3 'staking.*token|stake.*token' | head -60Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 1406
🏁 Script executed:
# Verify the ExpertStakedBalance key usage and fee-tier calculation
rg -nP --type=rust 'ExpertStakedBalance|get_expert_staked_balance' -A2 -B2Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 2415
🏁 Script executed:
# Check admin authorization logic and see if committee is implemented
rg -nP --type=rust 'arbitration|committee|admin.*auth' -C3 | head -80Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 4125
🏁 Script executed:
# Examine the get_treasury_address function and error handling
rg -nP --type=rust 'get_treasury_address|TreasuryNotConfigured' -A5 -B2Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 2147
🏁 Script executed:
# Find Error enum definition
rg -nP --type=rust 'enum Error' -A30Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 1587
🏁 Script executed:
# Check how staking token should be initialized or stored
rg -nP --type=rust 'StakingContract|staking.*contract|STAKING' -B2 -A2Repository: LightForgeHub/SkillSphere-Dapp
Length of output: 2191
Critical: slash_expert has five serious bugs.
-
Wrong token (line 1543). Uses
env.current_contract_address()instead ofget_staking_contract(). The transfer won't debit the actual staking token. -
Treasury accounting in the wrong storage (lines 1552–1557). Updates
DataKey::TreasuryBalancein instance storage, butget_treasury_balance,deposit_to_treasury, andwithdraw_treasuryread/write persistent storage. Slashed funds are silently invisible to the rest of the contract. -
Stake bookkeeping divergence (line 1548). Decrements
profile.staked_balance, but fee-tier calculation andget_expert_staked_balance(lines 330–334) read the separateDataKey::ExpertStakedBalance(addr)key. Slashed experts retain their fee-tier discount. -
Authorization incomplete (lines 1524–1527). Comment claims "admin or arbitration committee member", but only the admin check is implemented. Committee members are never authorized despite
initialize_arbitration_committeestoring them (lines 1461–1481). -
Wrong error type (lines 1539–1540). Returns
InsufficientTreasuryBalancewhen the treasury address is unset. That error semantically means balance is low, not that the treasury isn't configured. NoTreasuryNotConfiguredvariant exists, but the current usage is misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lib.rs` around lines 1507 - 1563, slash_expert has five bugs:
use the staking token, persist treasury accounting in the same storage, update
the separate expert-stake key, allow arbitration committee members, and return a
clear error when the treasury address is missing. Fix it by 1) using
Self::get_staking_contract() as the token address when constructing
token::Client and calling transfer (instead of env.current_contract_address());
2) update treasury bookkeeping through the same persistent storage and/or
existing helpers (get_treasury_balance, deposit_to_treasury, withdraw_treasury)
and write DataKey::TreasuryBalance into persistent storage (not instance
storage) so other methods see slashed funds; 3) decrement the expert stake both
on the ExpertProfile (profile.staked_balance) and the
DataKey::ExpertStakedBalance(expert_id) persistent key so
get_expert_staked_balance and fee-tier logic reflect the slash; 4) expand
authorization to allow either admin OR a member of the arbitration committee
stored by initialize_arbitration_committee (check the committee membership
storage/set before returning Unauthorized); and 5) add and return a clear error
variant (e.g., Error::TreasuryNotConfigured) when get_treasury_address(&env)
returns None, and use that instead of InsufficientTreasuryBalance.
This was referenced Apr 28, 2026
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.
Summary
Implement four critical smart contract features for the SkillSphere expert consultation platform built on Soroban. These implementations enable partial withdrawals for long sessions, expert staking for visibility, multi-signature arbitration governance, and penalty mechanisms for malicious experts.
Changes Implemented
1. Partial Withdrawals for Long Sessions (#162)
withdraw_accrued(session_id)last_settlement_timestampto track payment intervals2. Staking Mechanism for Top Experts (#163)
stake_tokens(amount),unstake_tokens(amount)3. Multi-Signature Arbitration Panel (#164)
initialize_arbitration_committee(member1, member2, member3),propose_resolution(session_id, seeker_award_bps)4. Escrow Slashing for Malicious Experts (#165)
slash_expert(expert_id, amount, reason)Technical Details
All implementations include:
Data Structure Updates:
ArbitrationCommitteetoDataKeyenumSession,ExpertProfiletypesSecurity Considerations
Integration Points
token::Clientfor XLM/USDC transfersTesting Recommendations
Next Steps
Closes #162
Closes #163
Closes #164
Closes #165
Summary by CodeRabbit