Implement Expert Registry, Encrypted Notes, and Active Status#150
Conversation
📝 WalkthroughWalkthroughThe pull request extends the expert profile system with additional state fields (rate, metadata CID, staked balance, reputation, availability), introduces three new contract entrypoints for expert registration and session management ( Changes
Sequence DiagramsequenceDiagram
participant User as User/Expert
participant Contract as SkillSphere Contract
participant Storage as Contract Storage
User->>Contract: register_expert(expert, rate, metadata_cid)
Contract->>Storage: Store ExpertProfile with rate & metadata
Storage-->>Contract: Success
Contract-->>User: Confirmed
User->>Contract: set_availability(expert, true)
Contract->>Storage: Update ExpertProfile availability_status
Storage-->>Contract: Success
Contract-->>User: Confirmed
User->>Contract: start_session(seeker, expert, token, amount, min_reputation, metadata_cid)
Contract->>Storage: Read ExpertProfile
Storage-->>Contract: Return ExpertProfile (rate, reputation, availability)
Contract->>Contract: Validate: registered, available, reputation ≥ min_reputation
Contract->>Storage: Create Session with derived rate_per_second, encrypted_notes_hash=None
Storage-->>Contract: Session ID returned
Contract-->>User: Return u64 session_id
User->>Contract: update_session_notes(caller, session_id, notes_hash)
Contract->>Storage: Verify caller is seeker or expert for session
Contract->>Storage: Update Session.encrypted_notes_hash
Storage-->>Contract: Success
Contract-->>User: Ok(())
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/src/lib.rs (2)
1-1:⚠️ Potential issue | 🟠 MajorPR description does not match the actual code changes.
The PR title/body describe fixes for
#173,#175,#176,#177(verification reset,VerificationHistorycap, redundant storage checks, TTL hygiene) acrossonboarding.rs,lib.rs,test.rs, andenhanced_features_test.rs. The diff in this file instead implements the commit-message features (#143–#146): expert registry, encrypted notes, and active status — and the listed companion files don't appear in the diff at all. Please align the PR description (and ideally split unrelated work into separate PRs) so reviewers and auditors can map changes to issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` at line 1, The PR description and title do not match the actual changes (the diff adds/adjusts crate-level attributes like #![no_std] and implements expert registry/encrypted notes/active status rather than fixes for issues `#173`–#177), so update the PR title/body to accurately describe the implemented features (mention `#143`–#146, expert registry, encrypted notes, active status and the actual modified files) or split this work into separate PRs that each map to the correct issue; ensure commit messages and the PR changelog reference the correct issue numbers and files (e.g., onboarding.rs, lib.rs, test.rs, enhanced_features_test.rs) so reviewers can map changes to issues.
525-601:⚠️ Potential issue | 🟠 MajorUpdate documentation to reflect
start_sessionAPI changes — the README is out of sync with the implementation.The breaking changes are confirmed and internal callers have been updated:
- Parameter
rate_per_secondremoved (now derived fromExpertProfile)- Return type changed to direct
u64with errors surfaced viapanic_with_error!However, the README (lines 117 and 206) still documents the old signature with
rate_per_secondas a parameter, creating documentation drift that will mislead users.Also note: this entrypoint now panics on errors while neighboring methods (
pause_session,settle_session,set_admin) returnResult. Consider standardizing error handling across the public API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 525 - 601, The README docs are out of sync with the new start_session signature: update the documented API entries that mention start_session (previously listing a rate_per_second parameter and a Result return) to reflect the current implementation of start_session(seeker: Address, expert: Address, token: Address, amount: i128, min_reputation: u32, metadata_cid: String) -> u64 (i.e., remove rate_per_second parameter and show it derives from ExpertProfile, and change the documented return to u64), and add a note that this entrypoint surfaces errors via panic_with_error! (unlike pause_session/settle_session/set_admin which return Result) so callers should expect panics; ensure examples and parameter lists for the start_session symbol are updated accordingly.
🧹 Nitpick comments (2)
contracts/src/lib.rs (2)
535-548: Sentinelrate_per_second == 0for "not registered" is fragile.Using
0as the registration sentinel meansregister_expertcan never legitimately offer free sessions, and any future code path that constructs anExpertProfilewithout going throughregister_expert(defaults, partial migration,set_expert_referreron a brand-new expert at line 351, etc.) will look "unregistered" even when other fields were populated.Consider an explicit boolean (e.g.,
is_registered: bool) onExpertProfile, set only byregister_expert. This also eliminates the need to validaterate > 0purely to avoid sentinel collisions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` around lines 535 - 548, The current check using profile.rate_per_second == 0 to detect registration is brittle; add an explicit is_registered: bool field to the ExpertProfile struct, set is_registered = true only inside register_expert, and change the registration check here to use that flag (replace the rate_per_second==0 sentinel with !profile.is_registered and keep the ExpertNotRegistered panic). Update any places that construct or mutate ExpertProfile (e.g., register_expert, set_expert_referrer, any default constructors or migrations) to initialize is_registered correctly; keep rate_per_second validation independent (allow zero rates if you want free sessions) or validate separately during register_expert if zero should be disallowed.
62-62: Remove the unusedExpertReputationvariant from theDataKeyenum.Nothing reads or writes
DataKey::ExpertReputationanymore. Be aware that removing or reordering variants in acontracttypeenum changes the on-wire discriminant for the remaining variants—either remove this variant last in the list or handle migration for any pre-existing storage under otherDataKeyvalues explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lib.rs` at line 62, Remove the unused ExpertReputation variant from the DataKey enum (contracttype) by deleting the `ExpertReputation` entry in the enum declaration; ensure you do this as the last change in the enum sequence or implement a storage migration/compatibility layer to preserve on-wire discriminants for existing persisted data (handle conversion of any pre-existing storage keyed under the old discriminant to the new key mapping). Update any pattern matches or imports that reference DataKey::ExpertReputation (search for the symbol) and run tests to verify no remaining references.
🤖 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 105-112: ExpertProfile.staked_balance must be removed to avoid
stale/duplicated state: delete the staked_balance field from the ExpertProfile
struct and remove any use of ExpertProfile.staked_balance; then update
get_expert_profile (and any code that constructs or returns an ExpertProfile) to
either omit that field or explicitly fetch the canonical value via
get_expert_staked_balance/DataKey::ExpertStakedBalance when a caller needs the
balance; ensure serialization/types are updated to match the new struct shape
and remove any writes/reads that tried to maintain two separate sources of
truth.
- Around line 180-187: set_availability currently creates an "available" profile
for unregistered experts; update the function to load the profile via
Self::expert_profile and reject if the expert isn't registered (detect
registration using the sentinel rate_per_second == 0 or the contract's
registration flag) by returning/throwing the ExpertNotRegistered error before
updating availability, then only set profile.availability_status and write back
to storage (DataKey::ExpertProfile(expert)) when registered; reference the
set_availability, register_expert, expert_profile, ExpertNotRegistered and
DataKey::ExpertProfile symbols when making the change.
- Around line 170-178: register_expert allows negative/zero rates and arbitrary
metadata CIDs; fix it by validating inputs before mutating storage: in
register_expert (and before using DataKey::ExpertProfile/expert_profile),
require rate > 0 (reject zero and negative rates, return/abort on invalid) and
optionally bound rate to a sane max if desired, and validate metadata_cid with
Self::is_valid_ipfs_cid(&metadata_cid) (abort if invalid) so it matches the same
CID checks used by start_session and flag_dispute; keep the existing
expert.require_auth() and only write the profile after both checks pass.
- Around line 189-200: update_session_notes must (1) early-check protocol is
active (call the same ensure_protocol_active/ protocol_paused check used by
resume_session/settle_session/batch_settle), (2) refuse updates when the session
is in terminal states (disallow when session.state is Finished, Disputed, or
Resolved — mirror the same state-guarding logic other mutators use), and (3)
validate notes_hash with is_valid_ipfs_cid(¬es_hash) and return the
appropriate Error variant if invalid; perform these checks before assigning
session.encrypted_notes_hash and persisting via DataKey::Session(session_id)
after fetching the session with get_session_or_error.
---
Outside diff comments:
In `@contracts/src/lib.rs`:
- Line 1: The PR description and title do not match the actual changes (the diff
adds/adjusts crate-level attributes like #![no_std] and implements expert
registry/encrypted notes/active status rather than fixes for issues `#173`–#177),
so update the PR title/body to accurately describe the implemented features
(mention `#143`–#146, expert registry, encrypted notes, active status and the
actual modified files) or split this work into separate PRs that each map to the
correct issue; ensure commit messages and the PR changelog reference the correct
issue numbers and files (e.g., onboarding.rs, lib.rs, test.rs,
enhanced_features_test.rs) so reviewers can map changes to issues.
- Around line 525-601: The README docs are out of sync with the new
start_session signature: update the documented API entries that mention
start_session (previously listing a rate_per_second parameter and a Result
return) to reflect the current implementation of start_session(seeker: Address,
expert: Address, token: Address, amount: i128, min_reputation: u32,
metadata_cid: String) -> u64 (i.e., remove rate_per_second parameter and show it
derives from ExpertProfile, and change the documented return to u64), and add a
note that this entrypoint surfaces errors via panic_with_error! (unlike
pause_session/settle_session/set_admin which return Result) so callers should
expect panics; ensure examples and parameter lists for the start_session symbol
are updated accordingly.
---
Nitpick comments:
In `@contracts/src/lib.rs`:
- Around line 535-548: The current check using profile.rate_per_second == 0 to
detect registration is brittle; add an explicit is_registered: bool field to the
ExpertProfile struct, set is_registered = true only inside register_expert, and
change the registration check here to use that flag (replace the
rate_per_second==0 sentinel with !profile.is_registered and keep the
ExpertNotRegistered panic). Update any places that construct or mutate
ExpertProfile (e.g., register_expert, set_expert_referrer, any default
constructors or migrations) to initialize is_registered correctly; keep
rate_per_second validation independent (allow zero rates if you want free
sessions) or validate separately during register_expert if zero should be
disallowed.
- Line 62: Remove the unused ExpertReputation variant from the DataKey enum
(contracttype) by deleting the `ExpertReputation` entry in the enum declaration;
ensure you do this as the last change in the enum sequence or implement a
storage migration/compatibility layer to preserve on-wire discriminants for
existing persisted data (handle conversion of any pre-existing storage keyed
under the old discriminant to the new key mapping). Update any pattern matches
or imports that reference DataKey::ExpertReputation (search for the symbol) and
run tests to verify no remaining references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| pub struct ExpertProfile { | ||
| pub rate_per_second: i128, | ||
| pub metadata_cid: String, | ||
| pub referrer: Option<Address>, | ||
| pub staked_balance: i128, | ||
| pub reputation: u32, | ||
| pub availability_status: bool, | ||
| } |
There was a problem hiding this comment.
ExpertProfile.staked_balance is unmaintained — it will always read 0.
set_expert_staked_balance (line 302) only writes to DataKey::ExpertStakedBalance(Address), and get_expert_staked_balance only reads from that key. Nothing in the new flow updates ExpertProfile.staked_balance, so this field will be permanently 0 for everyone except possibly through future migrations. Anyone reading it via get_expert_profile will see misleading data.
Either remove the field from ExpertProfile (and keep the dedicated DataKey::ExpertStakedBalance), or unify storage by removing DataKey::ExpertStakedBalance and routing all reads/writes through ExpertProfile. Don't keep both.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lib.rs` around lines 105 - 112, ExpertProfile.staked_balance
must be removed to avoid stale/duplicated state: delete the staked_balance field
from the ExpertProfile struct and remove any use of
ExpertProfile.staked_balance; then update get_expert_profile (and any code that
constructs or returns an ExpertProfile) to either omit that field or explicitly
fetch the canonical value via
get_expert_staked_balance/DataKey::ExpertStakedBalance when a caller needs the
balance; ensure serialization/types are updated to match the new struct shape
and remove any writes/reads that tried to maintain two separate sources of
truth.
| pub fn register_expert(env: Env, expert: Address, rate: i128, metadata_cid: String) { | ||
| expert.require_auth(); | ||
| let mut profile = Self::expert_profile(&env, expert.clone()); | ||
| profile.rate_per_second = rate; | ||
| profile.metadata_cid = metadata_cid; | ||
| env.storage() | ||
| .persistent() | ||
| .set(&DataKey::ExpertProfile(expert), &profile); | ||
| } |
There was a problem hiding this comment.
register_expert is missing critical input validation.
Two correctness/safety gaps:
rateis unvalidated. A negativerate_per_secondwould makestreamed_amount_sincereturn negative values during streaming, causingclaimableto become negative andsession.balance -= claimableto increase the balance — i.e., the expert can be paid out far more than was deposited (or refunds inflate).rate == 0is also silently accepted but is later overloaded as the "not registered" sentinel instart_session, which is a fragile coupling.metadata_cidis unvalidated. Every other CID written to storage in this contract is gated bySelf::is_valid_ipfs_cid(&cid)(seestart_session,flag_dispute). This entrypoint lets arbitrary garbage be stored and surfaced viaget_expert_profile.
🛡️ Proposed fix
- pub fn register_expert(env: Env, expert: Address, rate: i128, metadata_cid: String) {
- expert.require_auth();
- let mut profile = Self::expert_profile(&env, expert.clone());
- profile.rate_per_second = rate;
- profile.metadata_cid = metadata_cid;
- env.storage()
- .persistent()
- .set(&DataKey::ExpertProfile(expert), &profile);
- }
+ pub fn register_expert(
+ env: Env,
+ expert: Address,
+ rate: i128,
+ metadata_cid: String,
+ ) -> Result<(), Error> {
+ expert.require_auth();
+ Self::ensure_protocol_active(&env)?;
+ if rate <= 0 {
+ return Err(Error::InvalidAmount);
+ }
+ if !Self::is_valid_ipfs_cid(&metadata_cid) {
+ return Err(Error::InvalidCid);
+ }
+ let mut profile = Self::expert_profile(&env, expert.clone());
+ profile.rate_per_second = rate;
+ profile.metadata_cid = metadata_cid;
+ env.storage()
+ .persistent()
+ .set(&DataKey::ExpertProfile(expert), &profile);
+ 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 170 - 178, register_expert allows
negative/zero rates and arbitrary metadata CIDs; fix it by validating inputs
before mutating storage: in register_expert (and before using
DataKey::ExpertProfile/expert_profile), require rate > 0 (reject zero and
negative rates, return/abort on invalid) and optionally bound rate to a sane max
if desired, and validate metadata_cid with
Self::is_valid_ipfs_cid(&metadata_cid) (abort if invalid) so it matches the same
CID checks used by start_session and flag_dispute; keep the existing
expert.require_auth() and only write the profile after both checks pass.
| pub fn set_availability(env: Env, expert: Address, status: bool) { | ||
| expert.require_auth(); | ||
| let mut profile = Self::expert_profile(&env, expert.clone()); | ||
| profile.availability_status = status; | ||
| env.storage() | ||
| .persistent() | ||
| .set(&DataKey::ExpertProfile(expert), &profile); | ||
| } |
There was a problem hiding this comment.
set_availability should require prior registration.
Calling set_availability before register_expert materializes a profile with rate_per_second = 0 (the "not registered" sentinel) and availability_status = true. start_session will still reject it via ExpertNotRegistered, but off-chain readers of get_expert_profile see an inconsistent record (available but unregistered). Reject if the expert isn't registered.
🛠 Proposed fix
- pub fn set_availability(env: Env, expert: Address, status: bool) {
- expert.require_auth();
- let mut profile = Self::expert_profile(&env, expert.clone());
- profile.availability_status = status;
- env.storage()
- .persistent()
- .set(&DataKey::ExpertProfile(expert), &profile);
- }
+ pub fn set_availability(env: Env, expert: Address, status: bool) -> Result<(), Error> {
+ expert.require_auth();
+ let mut profile = Self::expert_profile(&env, expert.clone());
+ if profile.rate_per_second == 0 {
+ return Err(Error::ExpertNotRegistered);
+ }
+ profile.availability_status = status;
+ env.storage()
+ .persistent()
+ .set(&DataKey::ExpertProfile(expert), &profile);
+ Ok(())
+ }📝 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 set_availability(env: Env, expert: Address, status: bool) { | |
| expert.require_auth(); | |
| let mut profile = Self::expert_profile(&env, expert.clone()); | |
| profile.availability_status = status; | |
| env.storage() | |
| .persistent() | |
| .set(&DataKey::ExpertProfile(expert), &profile); | |
| } | |
| pub fn set_availability(env: Env, expert: Address, status: bool) -> Result<(), Error> { | |
| expert.require_auth(); | |
| let mut profile = Self::expert_profile(&env, expert.clone()); | |
| if profile.rate_per_second == 0 { | |
| return Err(Error::ExpertNotRegistered); | |
| } | |
| profile.availability_status = status; | |
| env.storage() | |
| .persistent() | |
| .set(&DataKey::ExpertProfile(expert), &profile); | |
| 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 180 - 187, set_availability currently
creates an "available" profile for unregistered experts; update the function to
load the profile via Self::expert_profile and reject if the expert isn't
registered (detect registration using the sentinel rate_per_second == 0 or the
contract's registration flag) by returning/throwing the ExpertNotRegistered
error before updating availability, then only set profile.availability_status
and write back to storage (DataKey::ExpertProfile(expert)) when registered;
reference the set_availability, register_expert, expert_profile,
ExpertNotRegistered and DataKey::ExpertProfile symbols when making the change.
| pub fn update_session_notes(env: Env, caller: Address, session_id: u64, notes_hash: String) -> Result<(), Error> { | ||
| caller.require_auth(); | ||
| let mut session = Self::get_session_or_error(&env, session_id)?; | ||
| if caller != session.seeker && caller != session.expert { | ||
| return Err(Error::Unauthorized); | ||
| } | ||
| session.encrypted_notes_hash = Some(notes_hash); | ||
| env.storage() | ||
| .persistent() | ||
| .set(&DataKey::Session(session_id), &session); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
update_session_notes is missing several guards.
Three issues that should be addressed before this ships:
- No session-state check. A seeker or expert can overwrite
encrypted_notes_hashonFinished,Disputed, orResolvedsessions. After dispute resolution this lets either party rewrite the off-chain evidence/notes pointer post-mortem, undermining the audit trail. notes_hashisn't validated as an IPFS CID. Other CID-bearing fields in this contract (metadata_cid,evidence_cid) go throughis_valid_ipfs_cid. Garbage in this field weakens the on-chain↔off-chain integrity contract that the "encrypted notes hash" is supposed to provide.- No
ensure_protocol_activecheck. Other state-mutating session operations (resume_session,settle_session,batch_settle) gate onprotocol_paused; this one doesn't.
🛡️ Proposed fix
- pub fn update_session_notes(env: Env, caller: Address, session_id: u64, notes_hash: String) -> Result<(), Error> {
- caller.require_auth();
- let mut session = Self::get_session_or_error(&env, session_id)?;
- if caller != session.seeker && caller != session.expert {
- return Err(Error::Unauthorized);
- }
- session.encrypted_notes_hash = Some(notes_hash);
- env.storage()
- .persistent()
- .set(&DataKey::Session(session_id), &session);
- Ok(())
- }
+ pub fn update_session_notes(
+ env: Env,
+ caller: Address,
+ session_id: u64,
+ notes_hash: String,
+ ) -> Result<(), Error> {
+ Self::ensure_protocol_active(&env)?;
+ caller.require_auth();
+ if !Self::is_valid_ipfs_cid(¬es_hash) {
+ return Err(Error::InvalidCid);
+ }
+ let mut session = Self::get_session_or_error(&env, session_id)?;
+ Self::require_participant(&session, &caller)?;
+ if !matches!(session.status, SessionStatus::Active | SessionStatus::Paused) {
+ return Err(Error::InvalidSessionState);
+ }
+ session.encrypted_notes_hash = Some(notes_hash);
+ Self::save_session(&env, &session);
+ 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 189 - 200, update_session_notes must (1)
early-check protocol is active (call the same ensure_protocol_active/
protocol_paused check used by resume_session/settle_session/batch_settle), (2)
refuse updates when the session is in terminal states (disallow when
session.state is Finished, Disputed, or Resolved — mirror the same
state-guarding logic other mutators use), and (3) validate notes_hash with
is_valid_ipfs_cid(¬es_hash) and return the appropriate Error variant if
invalid; perform these checks before assigning session.encrypted_notes_hash and
persisting via DataKey::Session(session_id) after fetching the session with
get_session_or_error.
SkillSphere Core Refactor: Registry, Privacy & Math Robustness
Overview
This PR introduces a major refactor to the expert onboarding and session management logic. By shifting from ad-hoc session creation to an on-chain Registry model, we ensure that only registered and available experts can provide services, while also significantly hardening the streaming math against edge cases.
Key Feature Implementations
1. Expert Profile & Registry (#146, #145)
register_expertwhich allows experts to define theirrate_per_secondandmetadata_cid(portfolio/bio) on-chain.availability_status(Boolean) and aset_availabilitymethod.start_sessionnow strictly validates that an expert is both registered and "On-Call" before allowing a session to begin.start_sessionto pull the rate directly from the registry rather than relying on caller-provided arguments, preventing rate manipulation.2. Encrypted Session Notes (#144)
encrypted_notes_hashfield to theSessionstruct.update_session_notesto store the IPFS hash of encrypted consultation notes, ensuring professional records remain private and tamper-proof.3. Streaming Math Robustness & Testing (#143)
Technical Changes
ExpertProfileandSessionstorage structuresstart_sessionsignature (removedrate_per_secondas an input)reputationstorage with the centralizedExpertProfilestructpanic_with_error!for clearer diagnostic debugging in testsCloses: #143
Closes: #144
Closes: #145
Closes: #146