feat: submit review logic - #61
Conversation
📝 WalkthroughWalkthroughThe reputation contract adds cross-contract review submission against completed vault bookings, persists review records and expert statistics, exposes review queries, defines related errors and events, and adds mock-vault integration tests. ChangesReputation review flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Reviewer
participant ReputationScoringContract
participant PaymentVault
participant PersistentStorage
Reviewer->>ReputationScoringContract: submit_review(booking_id, score)
ReputationScoringContract->>PaymentVault: get_booking(booking_id)
PaymentVault-->>ReputationScoringContract: BookingRecord
ReputationScoringContract->>PersistentStorage: store ReviewRecord and ExpertStats
ReputationScoringContract-->>Reviewer: emit review event
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/identity-registry-contract/src/contract.rs (1)
27-35:⚠️ Potential issue | 🟠 MajorPreserve the stored category on batch re-verification.
Line 34 is hit for any non-verified expert, including previously banned ones. That means a re-verify via
batch_add_expertsnow resets an existingcategory_idto0, even though the single-item ban/unban flows preserve it. At minimum, carry forward the stored category when a record already exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/identity-registry-contract/src/contract.rs` around lines 27 - 35, When batch re-verifying experts in the loop, preserve any existing category_id instead of unconditionally passing 0 to storage::set_expert_record; call the storage getter (e.g. storage::get_expert_record or storage::get_expert_category / storage::get_expert_status variant) to read the current category for the expert and use that value when invoking storage::set_expert_record(&env, &expert, ExpertStatus::Verified, empty_uri, category_id) so existing categories (e.g. for previously banned experts) are retained; if no record exists fall back to 0.
🧹 Nitpick comments (1)
contracts/identity-registry-contract/src/lib.rs (1)
89-99: Makecategory_idround-trippable through the public API.
update_profilenow requires callers to resendcategory_id, but the contract still only exposesget_status. A client doing a URI-only edit has no supported way to fetch the current value first, so it's easy to overwrite it with a stale or default category. Consider adding a getter for the stored record/category, or keepingupdate_profileURI-only and preserving the stored category internally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/identity-registry-contract/src/lib.rs` around lines 89 - 99, The public API requires callers to resend category_id in update_profile which can lead to overwrites; either add a getter that returns the stored profile record (including category) or change update_profile to be URI-only and preserve the existing category internally. Locate the contract functions update_profile and batch_update_profiles and implement one of two fixes: (A) add a new public getter (e.g., get_profile or get_category) that returns the stored URI and category for an Address so clients can read the current category before calling update_profile, or (B) modify contract::update_profile (and contract::batch_update_profiles handling) to accept only a new_uri and read/retain the existing category from storage when updating so callers need not supply category_id. Ensure you update public API signatures accordingly and keep get_status behavior consistent.
🤖 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/reputation-scoring-contract/src/contract.rs`:
- Around line 82-86: The call to the vault's get_booking currently deserializes
into a bare BookingRecord causing a trap when no booking exists; change the
invoke_contract deserialization to Option<BookingRecord> (e.g., let booking_opt:
Option<BookingRecord> = env.invoke_contract(...get_booking...)) and then .ok_or
or unwrap_or_else with a clear panic/error to handle missing booking; also
update MockVault in test.rs (the mock get_booking implementation and its return
type) to return Option<BookingRecord> so tests match the real ABI.
In `@contracts/reputation-scoring-contract/src/error.rs`:
- Around line 10-13: The error-code assignments in the ReputationError enum are
incorrect: change the variant named BookingNotComplete back to
InvalidBookingState with value 5, assign AlreadyReviewed value 7, and assign
NotBookingUser value 6 so codes match Issue `#54`; update the enum variant
names/values (InvalidScore = 4, InvalidBookingState = 5, NotBookingUser = 6,
AlreadyReviewed = 7) in error.rs (so callers decoding contract errors get the
correct reasons and you have a slot for a missing-booking error when the vault
lookup is fixed).
In `@contracts/reputation-scoring-contract/src/lib.rs`:
- Around line 40-47: The exported function submit_review currently only accepts
reviewer, booking_id, and score but must also accept expert and review_uri per
issue `#54`; update the public signature of submit_review (and its Env wrapper) to
include expert: Address and review_uri: String (or Bytes if URI is binary) and
pass those new args through to contract::submit_review (i.e., change the call to
contract::submit_review(&env, &reviewer, &expert, booking_id, score,
&review_uri) or the appropriate ordering), and then update the internal
contract::submit_review function signature and any call sites/ABI export so the
new parameters are accepted and propagated.
In `@contracts/reputation-scoring-contract/src/types.rs`:
- Around line 17-20: The ExpertStats struct's field types don't match the ABI in
issue `#54`: change ExpertStats (used by get_expert_stats) from { total_score:
u64, review_count: u32 } to { total_score: u64, total_reviews: u64 } so the
on-chain shape uses a u64 counter; update the struct declaration name and field
(review_count -> total_reviews, type u32 -> u64) and adjust any code, tests or
serialization/deserialization that reference ExpertStats or review_count to use
total_reviews accordingly.
---
Outside diff comments:
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 27-35: When batch re-verifying experts in the loop, preserve any
existing category_id instead of unconditionally passing 0 to
storage::set_expert_record; call the storage getter (e.g.
storage::get_expert_record or storage::get_expert_category /
storage::get_expert_status variant) to read the current category for the expert
and use that value when invoking storage::set_expert_record(&env, &expert,
ExpertStatus::Verified, empty_uri, category_id) so existing categories (e.g. for
previously banned experts) are retained; if no record exists fall back to 0.
---
Nitpick comments:
In `@contracts/identity-registry-contract/src/lib.rs`:
- Around line 89-99: The public API requires callers to resend category_id in
update_profile which can lead to overwrites; either add a getter that returns
the stored profile record (including category) or change update_profile to be
URI-only and preserve the existing category internally. Locate the contract
functions update_profile and batch_update_profiles and implement one of two
fixes: (A) add a new public getter (e.g., get_profile or get_category) that
returns the stored URI and category for an Address so clients can read the
current category before calling update_profile, or (B) modify
contract::update_profile (and contract::batch_update_profiles handling) to
accept only a new_uri and read/retain the existing category from storage when
updating so callers need not supply category_id. Ensure you update public API
signatures accordingly and keep get_status behavior consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c345b875-1cfd-4e79-aa48-c8f571e71046
📒 Files selected for processing (12)
contracts/identity-registry-contract/src/contract.rscontracts/identity-registry-contract/src/lib.rscontracts/identity-registry-contract/src/storage.rscontracts/identity-registry-contract/src/test.rscontracts/identity-registry-contract/src/types.rscontracts/reputation-scoring-contract/src/contract.rscontracts/reputation-scoring-contract/src/error.rscontracts/reputation-scoring-contract/src/events.rscontracts/reputation-scoring-contract/src/lib.rscontracts/reputation-scoring-contract/src/storage.rscontracts/reputation-scoring-contract/src/test.rscontracts/reputation-scoring-contract/src/types.rs
|
@arandomogg pls address the major and critical code rabbit comments and also remove changes to identity-registry-contract. Thanks |
Add submit_review to reputation-scoring contract with: - Score validation (1-5 range) - Cross-contract vault call to verify booking is Complete - Reviewer must be the booking user - Duplicate review prevention per booking_id - ExpertStats accumulation (total_score, review_count) - review_submitted event emission - ReviewRecord and ExpertStats types with getters - 10 new tests covering all paths via MockVault Rebased onto main; resolved conflicts with the merged dispute-penalty work (LightForgeHub#55) by keeping penalize_expert and submit_review side by side. Closes LightForgeHub#54 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@Bosun-Josh121 conflicts resolved, please merge |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
contracts/reputation-scoring-contract/src/error.rs (1)
6-15: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winError code assignments still don't match the previously-agreed fix.
This was previously raised as major: the ordering (
BookingNotComplete=5, AlreadyReviewed=6, NotBookingUser=7) doesn't align with issue#54's expected mapping, and callers decoding numeric contract errors will get the wrong reason. The author's earlier reply confirmed the fix would land, but the enum is unchanged from what was originally flagged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/reputation-scoring-contract/src/error.rs` around lines 6 - 15, Update the ReputationError enum’s numeric assignments to match the mapping agreed in issue `#54`, specifically correcting the ordering and values for BookingNotComplete, AlreadyReviewed, and NotBookingUser. Preserve the remaining error variants and their established codes so callers decoding contract errors receive the expected reasons.contracts/reputation-scoring-contract/src/contract.rs (1)
108-116: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winCross-contract ABI mismatch with the real vault's
get_bookingremains unfixed.This was previously flagged as critical: the production vault exports
get_bookingreturningOption<BookingRecord>, but this call still deserializes into a bareBookingRecord. If the booking doesn't exist, this will trap instead of allowing a proper error response — an external-call hazard that can abort the whole transaction unexpectedly rather than returning a typedReputationError.🐛 Proposed fix
- let booking: BookingRecord = env.invoke_contract( + let booking: Option<BookingRecord> = env.invoke_contract( &vault_address, &Symbol::new(env, "get_booking"), soroban_sdk::vec![env, booking_id.into_val(env)], ); + let booking = booking.ok_or(ReputationError::BookingNotComplete)?;Note the
MockVaulttest helper'sget_bookingalso returns a bareBookingRecord, so the test suite won't catch this mismatch against the real vault.#!/bin/bash rg -n -B2 -A6 'pub fn get_booking' contracts/payment-vault-contract/src/lib.rs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/reputation-scoring-contract/src/contract.rs` around lines 108 - 116, Update the cross-contract call in the reputation contract’s booking lookup to deserialize the vault’s get_booking result as Option<BookingRecord>, then convert None into the appropriate typed ReputationError instead of allowing deserialization to trap. Keep the existing vault-address and booking-id arguments unchanged, and update MockVault::get_booking or its callers so tests exercise the same optional-return ABI as the production vault.
🧹 Nitpick comments (1)
contracts/reputation-scoring-contract/src/test.rs (1)
3-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMockVault's hardcoded booking id forces full setup duplication in two tests.
set_bookingalways writes to key1u64(Line 26, Line 42), andsetup_with_vault()doesn't expose aMockVaultClientfor reuse. As a result,test_submit_review_booking_not_complete(Lines 274-296) andtest_expert_stats_accumulate(Lines 316-361) each re-implement the full env/registration/admin/init boilerplate from scratch instead of building onsetup_with_vault().♻️ Suggested refactor: parameterize booking id and expose the vault client
- pub fn set_booking(env: Env, user: Address, expert: Address, status: u32) { + pub fn set_booking(env: Env, booking_id: u64, user: Address, expert: Address, status: u32) { use crate::types::BookingRecord; let booking = BookingRecord { - id: 1, + id: booking_id, user, expert, ... }; - env.storage().persistent().set(&1u64, &booking); + env.storage().persistent().set(&booking_id, &booking); }Then
setup_with_vault()can returnvault_client(or tests can rebuildMockVaultClient::new(&env, &vault_id)from the already-returnedvault_id) and callvault_client.set_booking(&1u64, &user, &expert, &0u32)to override status, or&2u64for a second booking — removing the need for the manualenv.as_contractblock and duplicated env setup in both tests.Also applies to: 66-99, 274-296, 316-361
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/reputation-scoring-contract/src/test.rs` around lines 3 - 54, Refactor MockVault::set_booking and its callers to accept a booking_id instead of always storing under 1u64, while preserving get_booking’s keyed lookup. Update setup_with_vault to expose or allow reconstruction of the MockVaultClient, then rewrite test_submit_review_booking_not_complete and test_expert_stats_accumulate to reuse the shared setup and set booking status or additional bookings through the client, removing duplicated environment, registration, admin, and initialization boilerplate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/reputation-scoring-contract/src/lib.rs`:
- Around line 48-55: Update the submit_review entrypoint and its
contract::submit_review flow to accept both the supplied user/reviewer and
expert plus review_uri, then validate that the supplied parties match the
booking before recording the review. Extend types.rs ReviewRecord with
review_uri and update events.rs review_submitted to include the URI, propagating
the new data through storage and event emission consistently.
In `@contracts/reputation-scoring-contract/src/storage.rs`:
- Around line 53-79: Update get_expert_score, set_expert_score,
get_expert_reviews, and set_expert_reviews to use env.storage().persistent()
instead of instance() for the per-expert DataKey::ExpertScore and
DataKey::ExpertReviews entries, matching the storage approach used by Review and
ExpertStats.
In `@contracts/reputation-scoring-contract/src/test.rs`:
- Around line 216-234: The ExpertStats field name review_count conflicts with
the issue `#54` specification. Rename the field to total_reviews across the
ExpertStats definition and all related storage initialization, review submission
logic, getters, and tests, including test_submit_review_success, while
preserving the existing counter behavior.
In `@contracts/reputation-scoring-contract/src/types.rs`:
- Around line 14-20: Update the ExpertStats schema to rename review_count to
total_reviews and change its type from u32 to u64. Propagate this contract
across storage.rs get_expert_stats defaults, contract.rs increment logic, and
lib.rs's public get_expert_stats return path, ensuring all references use
total_reviews with u64-compatible values.
- Around line 22-47: Update the BookingRecord struct to mirror
PaymentVault::BookingRecord exactly by adding the dispute_user_refund,
dispute_expert_pay, and dispute_remainder_recovered fields with matching types
and ordering. Keep the existing fields unchanged so cross-contract
deserialization consumes the full get_booking tuple and submit_review can
perform its normal validation.
---
Duplicate comments:
In `@contracts/reputation-scoring-contract/src/contract.rs`:
- Around line 108-116: Update the cross-contract call in the reputation
contract’s booking lookup to deserialize the vault’s get_booking result as
Option<BookingRecord>, then convert None into the appropriate typed
ReputationError instead of allowing deserialization to trap. Keep the existing
vault-address and booking-id arguments unchanged, and update
MockVault::get_booking or its callers so tests exercise the same optional-return
ABI as the production vault.
In `@contracts/reputation-scoring-contract/src/error.rs`:
- Around line 6-15: Update the ReputationError enum’s numeric assignments to
match the mapping agreed in issue `#54`, specifically correcting the ordering and
values for BookingNotComplete, AlreadyReviewed, and NotBookingUser. Preserve the
remaining error variants and their established codes so callers decoding
contract errors receive the expected reasons.
---
Nitpick comments:
In `@contracts/reputation-scoring-contract/src/test.rs`:
- Around line 3-54: Refactor MockVault::set_booking and its callers to accept a
booking_id instead of always storing under 1u64, while preserving get_booking’s
keyed lookup. Update setup_with_vault to expose or allow reconstruction of the
MockVaultClient, then rewrite test_submit_review_booking_not_complete and
test_expert_stats_accumulate to reuse the shared setup and set booking status or
additional bookings through the client, removing duplicated environment,
registration, admin, and initialization boilerplate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10f55a6b-68ee-46a8-aff8-c39b0e0b930b
📒 Files selected for processing (7)
contracts/reputation-scoring-contract/src/contract.rscontracts/reputation-scoring-contract/src/error.rscontracts/reputation-scoring-contract/src/events.rscontracts/reputation-scoring-contract/src/lib.rscontracts/reputation-scoring-contract/src/storage.rscontracts/reputation-scoring-contract/src/test.rscontracts/reputation-scoring-contract/src/types.rs
| pub fn submit_review( | ||
| env: Env, | ||
| reviewer: Address, | ||
| booking_id: u64, | ||
| score: u32, | ||
| ) -> Result<(), ReputationError> { | ||
| contract::submit_review(&env, &reviewer, booking_id, score) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
submit_review still doesn't accept expert/review_uri; previously-agreed fix wasn't applied.
Issue #54 requires validating that the "supplied user and expert match the booking" and submitting a review URI, but this entrypoint only accepts reviewer, booking_id, score. The author previously confirmed this would be updated across lib.rs, contract.rs, types.rs (ReviewRecord.review_uri), and events.rs (review_submitted payload), but none of those changes are present in the current diff.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/reputation-scoring-contract/src/lib.rs` around lines 48 - 55,
Update the submit_review entrypoint and its contract::submit_review flow to
accept both the supplied user/reviewer and expert plus review_uri, then validate
that the supplied parties match the booking before recording the review. Extend
types.rs ReviewRecord with review_uri and update events.rs review_submitted to
include the URI, propagating the new data through storage and event emission
consistently.
| @@ -63,6 +78,40 @@ pub fn set_expert_reviews(env: &Env, expert: &Address, count: u64) { | |||
| .set(&DataKey::ExpertReviews(expert.clone()), &count); | |||
| } | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate storage.rs"
fd -a 'storage\.rs$' . | sed 's#^\./##'
echo
echo "## Outline storage.rs"
ast-grep outline contracts/reputation-scoring-contract/src/storage.rs --view compact || true
echo
echo "## Relevant section"
sed -n '1,130p' contracts/reputation-scoring-contract/src/storage.rs
echo
echo "## DataKey definitions"
rg -n "enum DataKey|ExpertScore|ExpertReviews|Review|ExpertStats|instance|persistent" contracts/reputation-scoring-contract/src/storage.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 5296
🌐 Web query:
Soroban instance storage single ledger entry per contract persistent storage unbounded user balances docs
💡 Result:
In Soroban, Instance storage is physically stored within a single ledger entry associated with the contract instance [1][2]. Because it resides in this shared entry, it is subject to a hard size limit defined by the network's maximum ledger entry size (typically in the order of 100 KB) [1][3][4]. Due to this structural limitation, Instance storage is not suitable for data that can grow in an unbounded fashion, such as user balances or large lists of records [3][4][5]. Attempting to store unbounded data in Instance storage can lead to Denial of Service (DoS) risks where the contract becomes unable to function once the ledger entry reaches its capacity [5]. For unbounded data like user balances, Persistent storage should be used instead [6][5]. Unlike Instance storage, Persistent storage allows each key-value pair to reside in its own separate ledger entry [6][7]. This design provides effectively unbounded aggregate capacity because each entry has its own independent lifetime (TTL) and capacity limit [6][7]. Using separate entries also avoids the performance overhead of loading the entire Instance storage map on every contract invocation, as the host only loads the specific entry required for the transaction [3][4][7].
Citations:
- 1: https://developers.stellar.org/docs/build/guides/storage/choosing-the-right-storage
- 2: https://github.com/stellar/stellar-protocol/blob/master/core/cap-0046-05.md
- 3: https://docs.rs/soroban-sdk/latest/soroban%5Fsdk/storage/struct.Storage.html
- 4: https://github.com/stellar/rs-soroban-sdk/blob/main/soroban-sdk/src/storage.rs
- 5: https://veridise.com/blog/learn-blockchain/how-to-develop-securely-on-soroban-storage-types-with-unbounded-data/
- 6: https://www.certik.com/blog/soroban-contract-state-management
- 7: https://developers.stellar.org/docs/build/guides/storage/storage-strategies
Move per-expert scoring/counters to persistent storage.
ExpertScore(Address) and ExpertReviews(Address) can grow with the number of experts, so storing them in instance storage loads a shared 100 KB ledger entry on every call and can hit instance-storage size limits. Review and ExpertStats already use persistent storage here; use the same approach for these per-expert values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/reputation-scoring-contract/src/storage.rs` around lines 53 - 79,
Update get_expert_score, set_expert_score, get_expert_reviews, and
set_expert_reviews to use env.storage().persistent() instead of instance() for
the per-expert DataKey::ExpertScore and DataKey::ExpertReviews entries, matching
the storage approach used by Review and ExpertStats.
| #[test] | ||
| fn test_submit_review_success() { | ||
| let (_env, _admin, user, expert, _vault_id, client) = setup_with_vault(); | ||
|
|
||
| let res = client.try_submit_review(&user, &1u64, &4u32); | ||
| assert!(res.is_ok()); | ||
|
|
||
| // Verify review stored | ||
| let review = client.get_review(&1u64).unwrap(); | ||
| assert_eq!(review.booking_id, 1); | ||
| assert_eq!(review.reviewer, user); | ||
| assert_eq!(review.expert, expert); | ||
| assert_eq!(review.score, 4); | ||
|
|
||
| // Verify expert stats updated | ||
| let stats = client.get_expert_stats(&expert); | ||
| assert_eq!(stats.total_score, 4); | ||
| assert_eq!(stats.review_count, 1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the actual ExpertStats field name used across the reputation contract.
rg -nP '\bstruct\s+ExpertStats\b' -A5 contracts/reputation-scoring-contract/src/types.rs
rg -nP '\breview_count\b|\btotal_reviews\b' contracts/reputation-scoring-contract/srcRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 883
Align ExpertStats field naming with issue #54.
The contract uses review_count (types.rs, tests, storage init, and review submission), while issue #54 specifies ExpertStats { total_score: u64, total_reviews: u64 }. Either rename review_count to total_reviews or explicitly update the matching spec so this intentional deviation is clear.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/reputation-scoring-contract/src/test.rs` around lines 216 - 234,
The ExpertStats field name review_count conflicts with the issue `#54`
specification. Rename the field to total_reviews across the ExpertStats
definition and all related storage initialization, review submission logic,
getters, and tests, including test_submit_review_success, while preserving the
existing counter behavior.
| /// Aggregate reputation stats for an expert | ||
| #[contracttype] | ||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub struct ExpertStats { | ||
| pub total_score: u64, | ||
| pub review_count: u32, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
ExpertStats schema still doesn't match issue #54; previously-flagged fix wasn't applied.
This was already raised as major: issue #54 specifies ExpertStats { total_score: u64, total_reviews: u64 }, but the struct still ships review_count: u32. The author's prior reply committed to renaming this to total_reviews: u64, but the field remains unchanged. This propagates downstream: storage.rs get_expert_stats default (review_count: 0), contract.rs stats.review_count += 1, and lib.rs's public get_expert_stats return type all still expose the narrower/incorrectly-named field.
♻️ Proposed fix
pub struct ExpertStats {
pub total_score: u64,
- pub review_count: u32,
+ pub total_reviews: u64,
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/reputation-scoring-contract/src/types.rs` around lines 14 - 20,
Update the ExpertStats schema to rename review_count to total_reviews and change
its type from u32 to u64. Propagate this contract across storage.rs
get_expert_stats defaults, contract.rs increment logic, and lib.rs's public
get_expert_stats return path, ensuring all references use total_reviews with
u64-compatible values.
| /// Mirror of PaymentVault's BookingStatus for cross-contract deserialization | ||
| #[contracttype] | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| #[repr(u32)] | ||
| pub enum BookingStatus { | ||
| Pending = 0, | ||
| Complete = 1, | ||
| Rejected = 2, | ||
| Reclaimed = 3, | ||
| Cancelled = 5, | ||
| } | ||
|
|
||
| /// Mirror of PaymentVault's BookingRecord for cross-contract deserialization | ||
| #[contracttype] | ||
| #[derive(Clone, Debug)] | ||
| pub struct BookingRecord { | ||
| pub id: u64, | ||
| pub user: Address, | ||
| pub expert: Address, | ||
| pub rate_per_second: i128, | ||
| pub max_duration: u64, | ||
| pub total_deposit: i128, | ||
| pub status: BookingStatus, | ||
| pub created_at: u64, | ||
| pub started_at: Option<u64>, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'lib.rs' contracts/payment-vault-contract
rg -n -A15 'enum BookingStatus' contracts/payment-vault-contract/src/lib.rs
rg -n -B2 -A12 'pub struct BookingRecord' contracts/payment-vault-contract/src/lib.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 217
🏁 Script executed:
#!/bin/bash
set -u
echo "== tracked contract type files =="
git ls-files | rg '(^|/)payment-vault-contract/src/|(^|/)reputation-scoring-contract/src/types.rs$|(^|/)Cargo.toml$'
echo
echo "== payment-vault BookingStatus definitions =="
rg -n -C8 'enum BookingStatus|struct BookingRecord|rated_per_second|total_deposit|booking_status|BookingStatus|Cancelled|Reclaimed' contracts/payment-vault-contract
echo
echo "== reputation get_booking callers =="
rg -n -C8 'get_booking|BookingRecord|BookingStatus|submit_review|BookingNotComplete' contracts/reputation-scoring-contract contracts/payment-vault-contractRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -u
echo "== reputation types.rs =="
cat -n contracts/reputation-scoring-contract/src/types.rs
echo
echo "== reputation review submission and get_booking usage =="
rg -n -C10 'submit_review|submit.*review|get_booking|BookingRecord|BookingStatus|DisputedAndResolved|BookingNotComplete|trap|unwrap|expect' contracts/reputation-scoring-contractRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 40438
Mirror PaymentVault’s BookingRecord fields exactly.
PaymentVault::BookingRecord has three extra dispute fields that this reputation contract omits. Add dispute_user_refund, dispute_expert_pay, and dispute_remainder_recovered; otherwise get_booking returns a longer tuple than BookingRecords deserializer expects and submit_review traps before it can return BookingNotComplete or validate the booking user/status.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/reputation-scoring-contract/src/types.rs` around lines 22 - 47,
Update the BookingRecord struct to mirror PaymentVault::BookingRecord exactly by
adding the dispute_user_refund, dispute_expert_pay, and
dispute_remainder_recovered fields with matching types and ordering. Keep the
existing fields unchanged so cross-contract deserialization consumes the full
get_booking tuple and submit_review can perform its normal validation.
|
@Bosun-Josh121 please merge pr |
3 participants
Summary
Closes #54
Test plan
Summary by CodeRabbit