Feat/contract optimizations#151
Conversation
📝 WalkthroughWalkthroughDocumentation updated to describe SEP-10-based authorization using wallet-signed challenges and Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
contracts/README.md (1)
62-108:⚠️ Potential issue | 🟡 MinorStruct definitions in README contradict the new lib.rs types.
This PR changes
Session.start_timestamp,Session.last_settlement_timestamp,Dispute.created_at,UpgradeTimelock.initiated_at, andUpgradeTimelock.execute_aftertou32incontracts/src/lib.rs, but the README still documents them asu64. Since the README is being touched for the SEP-10 section in this same PR, please update these struct snippets so on-chain users/integrators don't rely on the wrong field widths.Note: there are additional pre-existing drifts in this section worth fixing while you're here (e.g.
Disputenow hasevidence_cid/seeker_award_bps/expert_award_bps/auto_resolvedinstead ofipfs_metadata_hash/resolved/resolution;resolve_disputenow takes a BPS split, not an enum;start_session's documented signature on line 117 is missingmin_reputationandmetadata_cidand listsrate_per_secondwhich is no longer a parameter).📝 Proposed timestamp-only fix (the changes directly required by this PR)
pub struct Session { pub id: u64, pub seeker: Address, pub expert: Address, pub token: Address, pub rate_per_second: i128, - pub start_timestamp: u64, - pub last_settlement_timestamp: u64, + pub start_timestamp: u32, + pub last_settlement_timestamp: u32, pub status: SessionStatus, pub balance: i128, pub accrued_amount: i128, }pub struct Dispute { pub session_id: u64, pub reason: String, pub ipfs_metadata_hash: String, - pub created_at: u64, + pub created_at: u32, pub resolved: bool, pub resolution: u32, // 0=unresolved, 1=SeekerWins, 2=ExpertWins, 3=Refund }pub struct UpgradeTimelock { pub new_wasm_hash: BytesN<32>, - pub initiated_at: u64, - pub execute_after: u64, + pub initiated_at: u32, + pub execute_after: u32, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/README.md` around lines 62 - 108, Update the README struct snippets to match the new on-chain types in contracts/src/lib.rs: change Session.start_timestamp and Session.last_settlement_timestamp to u32, Dispute.created_at to u32, and UpgradeTimelock.initiated_at and execute_after to u32; also reconcile the Dispute fields (replace ipfs_metadata_hash/resolved/resolution with evidence_cid/seeker_award_bps/expert_award_bps/auto_resolved) and update the documented resolve_dispute and start_session signatures to reflect the current function parameters (resolve_dispute now accepts a BPS split, and start_session includes min_reputation and metadata_cid and no longer accepts rate_per_second).contracts/src/lib.rs (3)
963-991:⚠️ Potential issue | 🔴 CriticalSilent truncation when persisting
expiry/effective_timeback intou32.
expiry_timestamp_for_sessionreturns au64produced via(last_settlement_timestamp as u64).saturating_add(funded_seconds), wherefunded_seconds = balance / rate_per_secondand can become very large for big balances or low rates. Casting that result back withexpiry as u32(line 971) andeffective_time as u32(line 990) silently truncates whenever the value exceedsu32::MAX(~year 2106 in absolute terms, but reachable much sooner forexpirybecause it addsfunded_secondsto "now"). A truncatedlast_settlement_timestampwill appear as a past time, making subsequentstreamed_amount_sincecalculations stream the entire balance at once and corrupt accounting.The same pattern exists at line 1038 (
close_session) and at everyenv.ledger().timestamp() as u32site (lines 566, 652, 775, 849).🛠 Suggested helper to make the conversion explicit and safe
// Place near other helpers fn to_u32_timestamp(env: &Env, t: u64) -> u32 { if t > u32::MAX as u64 { panic_with_error!(env, Error::SessionExpired); } t as u32 }Then replace
expiry as u32/effective_time as u32/env.ledger().timestamp() as u32withSelf::to_u32_timestamp(env, x). At a minimum, clamp with.min(u32::MAX as u64) as u32so a malicious or malformed session cannot wrap the stored timestamp into the past.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 963 - 991, The code silently truncates u64 timestamps to u32 (e.g. expiry as u32 and effective_time as u32), which can wrap stored timestamps into the past and corrupt accounting; add a helper like Self::to_u32_timestamp(env: &Env, t: u64) -> u32 that explicitly checks if t > u32::MAX and either panics via panic_with_error!(env, Error::SessionExpired) or clamps to u32::MAX, then replace all raw casts (expiry as u32, effective_time as u32, and direct env.ledger().timestamp() as u32 uses) with Self::to_u32_timestamp(env, t) in methods such as expiry_timestamp_for_session usage sites, the settlement block (where session.last_settlement_timestamp is set), close_session, and other timestamp reads to ensure no silent truncation.
865-877:⚠️ Potential issue | 🔴 CriticalFix compile-time type mismatch in
execute_upgradecomparison.At line 875,
env.ledger().timestamp()(returnsu64) cannot be directly compared withtimelock.execute_after(u32from line 119). Rust does not implicitly coerce between integer types. This will fail to compile.Fix
- let now = env.ledger().timestamp(); - if now < timelock.execute_after { + let now = env.ledger().timestamp(); + if now < timelock.execute_after as u64 { return Err(Error::TimelockNotExpired);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 865 - 877, In execute_upgrade, the comparison between now (env.ledger().timestamp() -> u64) and timelock.execute_after (u32) causes a type-mismatch; fix by converting one side to the other's type (e.g., cast timelock.execute_after to u64) before comparing so the if now < ... check compiles; update the comparison in the execute_upgrade function referencing the UpgradeTimelock.execute_after field accordingly.
88-93:⚠️ Potential issue | 🟠 MajorStorage layout change (u64 → u32 timestamps) is a breaking, non-backward-compatible migration.
Switching
created_atfromu64tou32in theDisputestruct (line 88) changes the SCVal layout of persisted entries. The same applies toSession(last_settlement_timestampandstart_timestampat lines 130–131) andUpgradeTimelock(initiated_atandexecute_afterat lines 118–119). Any pre-existing entries written with a previous version will fail to decode after upgrade.No migration or state backfill code exists in the codebase. Before deploying:
- Confirm this is a greenfield deployment with no existing state, or
- Implement a migration step that rewrites all existing
Dispute,Session, andUpgradeTimelockentries to the new layout as part of the upgrade.Note: The README.md (lines 103–105) documents
initiated_at: u64but the code hasu32—documentation is out of sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 88 - 93, The change from u64 to u32 for timestamp fields is a storage-layout breaking change; either revert the types back to u64 on the Dispute struct (created_at), Session (last_settlement_timestamp, start_timestamp) and UpgradeTimelock (initiated_at, execute_after) to preserve SCVal compatibility, or add an on-upgrade migration that iterates existing keys and rewrites stored Dispute, Session, and UpgradeTimelock entries from the old layout to the new layout before accepting the new contract code; also update README.md to reflect whichever type you choose (ensure initiated_at: u64/u32 is consistent).
🧹 Nitpick comments (4)
contracts/src/lib.rs (3)
1078-1095: Repeatedas u64casts on the hot streaming path — extract a helper.
streamed_amount_sinceandexpiry_timestamp_for_sessionboth reconstructlast_settlement_timestamp as u64inline. Extracting a tiny accessor (e.g.fn last_settlement_u64(s: &Session) -> u64 { s.last_settlement_timestamp as u64 }) makes the intent explicit and centralizes the widening, so a future change to the timestamp type only needs to touch one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 1078 - 1095, Both streamed_amount_since and expiry_timestamp_for_session repeatedly cast session.last_settlement_timestamp to u64; add a tiny helper like fn last_settlement_u64(s: &Session) -> u64 { s.last_settlement_timestamp as u64 } and replace each inline cast with calls to last_settlement_u64(&session) (or &s where applicable) so the widening is centralized; update references in streamed_amount_since and expiry_timestamp_for_session to use this helper and keep behavior unchanged.
9-10: Constant types stillu64while the timestamps they're added to are nowu32.
TIMELOCK_DURATIONandDISPUTE_EXPIRY_WINDOWremainu64constants. At line 853 you castTIMELOCK_DURATION as u32per use; at line 1242 you castdispute.created_at as u64to addDISPUTE_EXPIRY_WINDOW. Pinning these constants to the type they're actually used with reduces casting noise and prevents accidental truncation if either constant grows in the future.♻️ Optional cleanup
-const TIMELOCK_DURATION: u64 = 48 * 60 * 60; -const DISPUTE_EXPIRY_WINDOW: u64 = 30 * 24 * 60 * 60; +const TIMELOCK_DURATION: u32 = 48 * 60 * 60; +const DISPUTE_EXPIRY_WINDOW: u64 = 30 * 24 * 60 * 60; // kept u64: added to a widened u64 timestamp- execute_after: now.saturating_add(TIMELOCK_DURATION as u32), + execute_after: now.saturating_add(TIMELOCK_DURATION),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 9 - 10, The constants TIMELOCK_DURATION and DISPUTE_EXPIRY_WINDOW are declared as u64 but are added to u32 timestamps in code (you currently cast them at use sites), so change their types to u32 to match usage, update any dependent arithmetic to u32, and remove the explicit casts (e.g., the TIMELOCK_DURATION as u32 and the cast of dispute.created_at as u64) so the additions use matching types and avoid truncation or unnecessary casting.
1272-1303: Add tests covering the u32 timestamp boundaries.The optimization touches every timestamp write site, but the test suite only exercises timestamps in the low thousands (around
1_000–1_060). Two cases worth covering explicitly:
- A session whose
expiry_timestamp_for_sessionwould exceedu32::MAX— verify the contract either errors cleanly or saturates instead of silently truncating into a past timestamp.execute_upgradeinvoked withenv.ledger().timestamp()set above the timelock — this currently doesn't compile (see comment on lines 865-877), and a test would have caught it.Want me to draft these tests?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 1272 - 1303, Add unit tests that exercise u32 timestamp boundary behavior: (1) create a session via client.start_session with a rate and deposit such that expiry_timestamp_for_session would exceed u32::MAX and assert the contract either returns an explicit error or saturates (e.g., compare client.get_current_earnings or start_session result against expected saturated value/error) to ensure no silent wraparound; (2) add a test that advances env.ledger().set_timestamp above the timelock and then calls execute_upgrade (or the public wrapper used in tests) to ensure the call compiles and behaves correctly when ledger timestamp > timelock, asserting the expected success/failure path. Use existing helpers like setup(), register_and_avail(), test_cid(), and token::StellarAssetClient::mint() to construct the scenarios so the new tests mirror test_1_second_session and test_1_year_session_overflow_check structure.contracts/README.md (1)
252-266: Clarify SEP-10 vs. Sorobanrequire_auth()distinction in the Authorization & SEP-10 section.The signer mappings are correct—
start_session(seeker),settle_session(expert), andflag_dispute(seeker) all userequire_auth()as documented. However, the section conflates two different authorization mechanisms. SEP-10 is an off-chain web authentication protocol that issues JWTs for wallet-server sessions, while Sorobanrequire_auth()is an on-chain mechanism that validates pre-signed authorization entries in transactions. The wording "By leveragingrequire_auth(), the contract ensures that only the actual owner of a Stellar address can perform sensitive operations" is accurate, but preceding context about SEP-10 could mislead readers into thinking the SEP-10 challenge itself is whatrequire_auth()validates. Consider clarifying that SEP-10 handles frontend/backend authentication, while Soroban auth entries (also wallet-signed) are whatrequire_auth()enforces on-chain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/README.md` around lines 252 - 266, Revise the "Authorization & SEP-10" section to clearly separate SEP-10 (off-chain web auth that issues JWTs and validates wallet-server sessions) from Soroban's on-chain authorization (require_auth() and require_auth_for_args()); explicitly state that SEP-10 is used by the frontend/backend for login and challenge signing, while require_auth() enforces wallet-signed auth entries in on-chain transactions, and update the wording around start_session, settle_session, and flag_dispute to note those functions call address.require_auth() on-chain (not that SEP-10 itself is validated by require_auth()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/README.md`:
- Around line 62-108: Update the README struct snippets to match the new
on-chain types in contracts/src/lib.rs: change Session.start_timestamp and
Session.last_settlement_timestamp to u32, Dispute.created_at to u32, and
UpgradeTimelock.initiated_at and execute_after to u32; also reconcile the
Dispute fields (replace ipfs_metadata_hash/resolved/resolution with
evidence_cid/seeker_award_bps/expert_award_bps/auto_resolved) and update the
documented resolve_dispute and start_session signatures to reflect the current
function parameters (resolve_dispute now accepts a BPS split, and start_session
includes min_reputation and metadata_cid and no longer accepts rate_per_second).
In `@contracts/src/lib.rs`:
- Around line 963-991: The code silently truncates u64 timestamps to u32 (e.g.
expiry as u32 and effective_time as u32), which can wrap stored timestamps into
the past and corrupt accounting; add a helper like Self::to_u32_timestamp(env:
&Env, t: u64) -> u32 that explicitly checks if t > u32::MAX and either panics
via panic_with_error!(env, Error::SessionExpired) or clamps to u32::MAX, then
replace all raw casts (expiry as u32, effective_time as u32, and direct
env.ledger().timestamp() as u32 uses) with Self::to_u32_timestamp(env, t) in
methods such as expiry_timestamp_for_session usage sites, the settlement block
(where session.last_settlement_timestamp is set), close_session, and other
timestamp reads to ensure no silent truncation.
- Around line 865-877: In execute_upgrade, the comparison between now
(env.ledger().timestamp() -> u64) and timelock.execute_after (u32) causes a
type-mismatch; fix by converting one side to the other's type (e.g., cast
timelock.execute_after to u64) before comparing so the if now < ... check
compiles; update the comparison in the execute_upgrade function referencing the
UpgradeTimelock.execute_after field accordingly.
- Around line 88-93: The change from u64 to u32 for timestamp fields is a
storage-layout breaking change; either revert the types back to u64 on the
Dispute struct (created_at), Session (last_settlement_timestamp,
start_timestamp) and UpgradeTimelock (initiated_at, execute_after) to preserve
SCVal compatibility, or add an on-upgrade migration that iterates existing keys
and rewrites stored Dispute, Session, and UpgradeTimelock entries from the old
layout to the new layout before accepting the new contract code; also update
README.md to reflect whichever type you choose (ensure initiated_at: u64/u32 is
consistent).
---
Nitpick comments:
In `@contracts/README.md`:
- Around line 252-266: Revise the "Authorization & SEP-10" section to clearly
separate SEP-10 (off-chain web auth that issues JWTs and validates wallet-server
sessions) from Soroban's on-chain authorization (require_auth() and
require_auth_for_args()); explicitly state that SEP-10 is used by the
frontend/backend for login and challenge signing, while require_auth() enforces
wallet-signed auth entries in on-chain transactions, and update the wording
around start_session, settle_session, and flag_dispute to note those functions
call address.require_auth() on-chain (not that SEP-10 itself is validated by
require_auth()).
In `@contracts/src/lib.rs`:
- Around line 1078-1095: Both streamed_amount_since and
expiry_timestamp_for_session repeatedly cast session.last_settlement_timestamp
to u64; add a tiny helper like fn last_settlement_u64(s: &Session) -> u64 {
s.last_settlement_timestamp as u64 } and replace each inline cast with calls to
last_settlement_u64(&session) (or &s where applicable) so the widening is
centralized; update references in streamed_amount_since and
expiry_timestamp_for_session to use this helper and keep behavior unchanged.
- Around line 9-10: The constants TIMELOCK_DURATION and DISPUTE_EXPIRY_WINDOW
are declared as u64 but are added to u32 timestamps in code (you currently cast
them at use sites), so change their types to u32 to match usage, update any
dependent arithmetic to u32, and remove the explicit casts (e.g., the
TIMELOCK_DURATION as u32 and the cast of dispute.created_at as u64) so the
additions use matching types and avoid truncation or unnecessary casting.
- Around line 1272-1303: Add unit tests that exercise u32 timestamp boundary
behavior: (1) create a session via client.start_session with a rate and deposit
such that expiry_timestamp_for_session would exceed u32::MAX and assert the
contract either returns an explicit error or saturates (e.g., compare
client.get_current_earnings or start_session result against expected saturated
value/error) to ensure no silent wraparound; (2) add a test that advances
env.ledger().set_timestamp above the timelock and then calls execute_upgrade (or
the public wrapper used in tests) to ensure the call compiles and behaves
correctly when ledger timestamp > timelock, asserting the expected
success/failure path. Use existing helpers like setup(), register_and_avail(),
test_cid(), and token::StellarAssetClient::mint() to construct the scenarios so
the new tests mirror test_1_second_session and
test_1_year_session_overflow_check structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 791f3663-78bc-4f23-bae5-ba8a769f195c
📒 Files selected for processing (2)
contracts/README.mdcontracts/src/lib.rs
2 participants
PR Summary: SkillSphere Contract Optimizations
This PR focuses on gas optimization and security integration for the SkillSphere smart contract.
Key Changes
1. Gas-Optimized Data Packing (#148)
Session,Dispute, andUpgradeTimelockstructures to useu32for timestamps instead ofu64.2. Integration with Stellar SEP-10 for Auth (#147)
require_auth()usage to ensure all entry points are compatible with SEP-10 web auth.Closes
Summary by CodeRabbit
Documentation
Refactor