Open
Conversation
Add category_id (u32) to ExpertRecord, propagated through add_expert, update_profile, batch_update_profiles, and preserved across ban/unban. Includes test for full category_id lifecycle. Closes LightForgeHub#41 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 Closes LightForgeHub#54 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR extends the identity registry's expert record structure with a Changes
Sequence DiagramsequenceDiagram
participant Reviewer
participant ReputationContract
participant PaymentVault
participant Storage
Reviewer->>ReputationContract: submit_review(booking_id, score)
ReputationContract->>ReputationContract: Validate score (1-5)
ReputationContract->>ReputationContract: Check contract initialized & not paused
ReputationContract->>Storage: Check !has_review(booking_id)
Storage-->>ReputationContract: Review exists?
ReputationContract->>PaymentVault: get_booking(booking_id)
PaymentVault-->>ReputationContract: BookingRecord
ReputationContract->>ReputationContract: Verify status == Complete
ReputationContract->>ReputationContract: Verify reviewer == booking.user
ReputationContract->>Storage: set_review(booking_id, ReviewRecord)
ReputationContract->>Storage: Get expert stats (or default)
ReputationContract->>Storage: Update expert stats (total_score += score, review_count++)
ReputationContract->>ReputationContract: Emit review_submitted event
ReputationContract-->>Reviewer: Ok(())
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
Collaborator
|
@arandomogg pls address the major and critical code rabbit comments and also remove changes to identity-registry-contract. Thanks |
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
3 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
Closes #54
Test plan
Summary by CodeRabbit
Release Notes
New Features
Tests