feat: automated dispute penalties#62
Closed
arandomogg wants to merge 3 commits intoLightForgeHub:mainfrom
Closed
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 <[email protected]>
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 <[email protected]>
…ties Add admin-only penalize_expert with saturating score deduction, InvalidPenalty guard on zero amount, pause check, event emission, and 7 new tests covering all paths. Closes LightForgeHub#55
📝 WalkthroughWalkthroughThe pull request adds category tracking to expert records in the identity registry contract and implements a comprehensive review and penalty system in the reputation scoring contract. The identity registry now persists a Changes
Sequence DiagramsequenceDiagram
actor User as Reviewer / User
participant RC as ReputationScoring<br/>Contract
participant Vault as Payment Vault<br/>(Cross-Contract)
participant Storage as Persistent<br/>Storage
User->>RC: submit_review(booking_id, score)
RC->>RC: Check initialized & not paused
RC->>RC: Validate score ∈ [1, 5]
RC->>RC: Check not already reviewed
RC->>Vault: get_booking(booking_id)
Vault-->>RC: BookingRecord
RC->>RC: Verify status == Complete
RC->>RC: Verify reviewer == booking.user
RC->>Storage: set_review(booking_id, ReviewRecord)
RC->>Storage: get_expert_stats(expert)
Storage-->>RC: ExpertStats
RC->>RC: Update stats<br/>(total_score += score,<br/>review_count += 1)
RC->>Storage: set_expert_stats(expert, updated)
RC->>RC: Emit review_submitted event
RC-->>User: Success
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related Issues
Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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 |
Collaborator
|
@arandomogg please ensure errors, function parameters and so on.. are exactly as specified in issue. Also remove changes to unrelated contracts(identity-registry) and remove other changes not related to task(submit review) |
Collaborator
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/identity-registry-contract/src/lib.rs (1)
47-55:⚠️ Potential issue | 🟠 MajorPlease split the registry API expansion out of this penalties PR.
These
category_idadditions widen the identity-registry contract interface in a PR that is otherwise scoped to dispute penalties inreputation-scoring. Keeping both together makes review and rollout riskier, and it conflicts with the earlier request to remove unrelated identity-registry changes.Also applies to: 89-100
🤖 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 47 - 55, The add_expert API was expanded with data_uri and category_id which widens the contract surface; revert this change by restoring the original add_expert signature and call into contract::verify_expert to its prior form (remove the data_uri and category_id parameters from pub fn add_expert and from the contract::verify_expert invocation), and undo the same related additions in the other affected block (the similar change around the 89-100 region) so the registry interface remains unchanged for this penalties PR.contracts/identity-registry-contract/src/contract.rs (1)
19-35:⚠️ Potential issue | 🟠 MajorDon't zero out
category_idinbatch_add_experts.This path still allows re-verifying any non-
Verifiedexpert, so a previously banned expert can come back through it. Hard-coding0here silently erases that expert's stored category metadata. Preserve the existing category when a record already exists, or extend the batch payload so first-time batch imports can provide it.💡 Proposed fix
- for expert in experts { - let status = storage::get_expert_status(&env, &expert); + for expert in experts { + let existing = storage::get_expert_record(&env, &expert); + let status = existing.status; if status == ExpertStatus::Verified { return Err(RegistryError::AlreadyVerified); } // Default empty URI for batch adds let empty_uri = String::from_str(&env, ""); - storage::set_expert_record(&env, &expert, ExpertStatus::Verified, empty_uri, 0); + storage::set_expert_record( + &env, + &expert, + ExpertStatus::Verified, + empty_uri, + existing.category_id, + ); storage::add_expert_to_index(&env, &expert); events::emit_status_change(&env, expert, status, ExpertStatus::Verified, admin.clone()); }🤖 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 19 - 35, batch_add_experts currently calls storage::set_expert_record(..., 0) which wipes an existing expert's category_id; instead, when iterating in batch_add_experts fetch the existing record (via storage::get_expert_status / storage::get_expert_record or equivalent) and if a record exists reuse its category_id when calling storage::set_expert_record; for new experts either keep the current default behavior or modify the batch_add_experts signature to accept a parallel Vec<category_id> so first-time imports can supply a category rather than hard-coding 0.contracts/identity-registry-contract/src/types.rs (1)
15-22:⚠️ Potential issue | 🟡 MinorStorage-breaking schema change if this is a contract upgrade—clarify deployment scenario.
Adding
category_idchangesExpertRecord's XDR serialization format. If records already exist in persisted storage from a prior version, deserialization will fail; theunwrap_or()fallback inget_expert_record()only handles missing keys, not deserialization errors. If this is a fresh deployment (version 0.0.0), no existing data exists to migrate. If this is an upgrade to a previously deployed contract, implement versioning or a migration path before deployment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/identity-registry-contract/src/types.rs` around lines 15 - 22, You added a new field category_id to ExpertRecord which breaks XDR serialization for existing on-ledger records; if this is an upgrade you must provide a migration/versioning path rather than only handling missing keys—either make the new field optional (e.g., Option) to preserve old serialized format, or implement explicit versioned deserialization/migration in get_expert_record() (catch/deserialise older layout, populate category_id default, and re-save or return migrated record), and add a contract-level schema version marker so future upgrades can detect and run migrations before relying on the new ExpertRecord layout.
🧹 Nitpick comments (2)
contracts/reputation-scoring-contract/src/storage.rs (1)
53-69: Consider adding TTL extension for persistent storage entries.Persistent storage entries in Soroban require TTL management to prevent expiration. The
set_reviewandset_expert_statsfunctions write to persistent storage but don't extend TTL, and the read functions (get_review,get_expert_stats) don't extend TTL on access either.For data that should persist long-term (reviews, expert stats), consider calling
extend_ttlwhen writing or accessing these entries to prevent unexpected data loss.Example for set_review
pub fn set_review(env: &Env, booking_id: u64, review: &ReviewRecord) { + let key = DataKey::Review(booking_id); env.storage() .persistent() - .set(&DataKey::Review(booking_id), review); + .set(&key, review); + env.storage() + .persistent() + .extend_ttl(&key, LIFETIME_THRESHOLD, BUMP_AMOUNT); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/reputation-scoring-contract/src/storage.rs` around lines 53 - 69, Persistent storage entries for reviews and expert stats (DataKey::Review, functions set_review/get_review and set_expert_stats/get_expert_stats) currently never extend their TTL, so add calls to persistent().extend_ttl(...) when writing and when reading these keys: call extend_ttl for the same DataKey used in set_review and set_expert_stats immediately after set(...) and also in get_review/get_expert_stats when a value is found to refresh the TTL; use a sensible TTL constant or config value and ensure you reference the same key (DataKey::Review(booking_id) and the expert stats key) so entries won't expire unexpectedly.contracts/reputation-scoring-contract/src/test.rs (1)
289-300: Consider adding test for maximum score boundary (5).The
test_score_boundary_valuestest verifies the minimum valid score (1) but doesn't test the maximum valid score (5) in isolation. Whiletest_submit_review_emits_eventuses score 5, a dedicated boundary test for the maximum would provide clearer coverage documentation.Optional: Add max boundary test
#[test] fn test_score_boundary_max() { // Test with fresh setup to isolate max score boundary let env = Env::default(); env.mock_all_auths(); let vault_id = env.register(MockVault, ()); let vault_client = MockVaultClient::new(&env, &vault_id); let contract_id = env.register(ReputationScoringContract, ()); let client = ReputationScoringContractClient::new(&env, &contract_id); let admin = Address::generate(&env); let user = Address::generate(&env); let expert = Address::generate(&env); client.init(&admin, &vault_id); vault_client.set_booking(&user, &expert, &1u32); let res = client.try_submit_review(&user, &1u64, &5u32); assert!(res.is_ok()); let stats = client.get_expert_stats(&expert); assert_eq!(stats.total_score, 5); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/reputation-scoring-contract/src/test.rs` around lines 289 - 300, Add a dedicated max-boundary test that mirrors the minimum test but submits score 5: create a fresh environment (either reuse setup_with_vault() or the expanded init flow shown), register vault and contract, set_booking for user→expert, call try_submit_review(&user, &1u64, &5u32) and assert it's Ok, then call get_expert_stats(&expert) and assert stats.total_score == 5 and stats.review_count == 1; name the test e.g. test_score_boundary_max to make intent explicit.
🤖 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 cross-contract call to PaymentVault is currently
deserializing the result directly into BookingRecord via the env.invoke_contract
call using Symbol::new(env, "get_booking") and will panic when the vault returns
None; change the invocation to deserialize into Option<BookingRecord> (e.g.,
Option<BookingRecord>) and handle the None case explicitly (map to a Result::Err
with a new error variant like BookingNotFound or an appropriate existing error),
ensuring you use the same vault_address, booking_id and get_booking symbols and
avoid direct deserialization to BookingRecord.
In `@contracts/reputation-scoring-contract/src/events.rs`:
- Around line 24-29: The event payload for expert_penalized is missing the
required new_score; update the function signature expert_penalized(env: &Env,
expert: &Address, amount: u64, new_score: u64) and include new_score in the
published payload (publish(topics, (expert.clone(), amount, new_score))); then
update the caller in contract.rs that invokes expert_penalized to pass the
updated stats.total_score (the value after the saturating subtraction) as the
new_score argument so consumers receive the resulting reputation in the event.
---
Outside diff comments:
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 19-35: batch_add_experts currently calls
storage::set_expert_record(..., 0) which wipes an existing expert's category_id;
instead, when iterating in batch_add_experts fetch the existing record (via
storage::get_expert_status / storage::get_expert_record or equivalent) and if a
record exists reuse its category_id when calling storage::set_expert_record; for
new experts either keep the current default behavior or modify the
batch_add_experts signature to accept a parallel Vec<category_id> so first-time
imports can supply a category rather than hard-coding 0.
In `@contracts/identity-registry-contract/src/lib.rs`:
- Around line 47-55: The add_expert API was expanded with data_uri and
category_id which widens the contract surface; revert this change by restoring
the original add_expert signature and call into contract::verify_expert to its
prior form (remove the data_uri and category_id parameters from pub fn
add_expert and from the contract::verify_expert invocation), and undo the same
related additions in the other affected block (the similar change around the
89-100 region) so the registry interface remains unchanged for this penalties
PR.
In `@contracts/identity-registry-contract/src/types.rs`:
- Around line 15-22: You added a new field category_id to ExpertRecord which
breaks XDR serialization for existing on-ledger records; if this is an upgrade
you must provide a migration/versioning path rather than only handling missing
keys—either make the new field optional (e.g., Option) to preserve old
serialized format, or implement explicit versioned deserialization/migration in
get_expert_record() (catch/deserialise older layout, populate category_id
default, and re-save or return migrated record), and add a contract-level schema
version marker so future upgrades can detect and run migrations before relying
on the new ExpertRecord layout.
---
Nitpick comments:
In `@contracts/reputation-scoring-contract/src/storage.rs`:
- Around line 53-69: Persistent storage entries for reviews and expert stats
(DataKey::Review, functions set_review/get_review and
set_expert_stats/get_expert_stats) currently never extend their TTL, so add
calls to persistent().extend_ttl(...) when writing and when reading these keys:
call extend_ttl for the same DataKey used in set_review and set_expert_stats
immediately after set(...) and also in get_review/get_expert_stats when a value
is found to refresh the TTL; use a sensible TTL constant or config value and
ensure you reference the same key (DataKey::Review(booking_id) and the expert
stats key) so entries won't expire unexpectedly.
In `@contracts/reputation-scoring-contract/src/test.rs`:
- Around line 289-300: Add a dedicated max-boundary test that mirrors the
minimum test but submits score 5: create a fresh environment (either reuse
setup_with_vault() or the expanded init flow shown), register vault and
contract, set_booking for user→expert, call try_submit_review(&user, &1u64,
&5u32) and assert it's Ok, then call get_expert_stats(&expert) and assert
stats.total_score == 5 and stats.review_count == 1; name the test e.g.
test_score_boundary_max to make intent explicit.
🪄 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: 80b92b95-0afc-457c-b564-f1c184b81220
📒 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
Comment on lines
+82
to
+86
| let booking: BookingRecord = env.invoke_contract( | ||
| &vault_address, | ||
| &Symbol::new(env, "get_booking"), | ||
| soroban_sdk::vec![env, booking_id.into_val(env)], | ||
| ); |
There was a problem hiding this comment.
Critical: Cross-contract call expects BookingRecord but vault returns Option<BookingRecord>.
The get_booking function in PaymentVault (per context snippet at contracts/payment-vault-contract/src/lib.rs:128-130) returns Option<BookingRecord>, but this code deserializes directly to BookingRecord. For non-existent booking IDs, the vault will return None, causing XDR deserialization to fail with a panic rather than a graceful error.
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)?;You may also want a more specific error variant like BookingNotFound for this case to distinguish it from an incomplete booking.
📝 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.
Suggested change
| let booking: BookingRecord = env.invoke_contract( | |
| &vault_address, | |
| &Symbol::new(env, "get_booking"), | |
| soroban_sdk::vec![env, booking_id.into_val(env)], | |
| ); | |
| 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)?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/reputation-scoring-contract/src/contract.rs` around lines 82 - 86,
The cross-contract call to PaymentVault is currently deserializing the result
directly into BookingRecord via the env.invoke_contract call using
Symbol::new(env, "get_booking") and will panic when the vault returns None;
change the invocation to deserialize into Option<BookingRecord> (e.g.,
Option<BookingRecord>) and handle the None case explicitly (map to a Result::Err
with a new error variant like BookingNotFound or an appropriate existing error),
ensuring you use the same vault_address, booking_id and get_booking symbols and
avoid direct deserialization to BookingRecord.
Comment on lines
+24
to
+29
| /// Emitted when an expert is penalized via dispute. | ||
| pub fn expert_penalized(env: &Env, expert: &Address, amount: u64) { | ||
| let topics = (symbol_short!("penalize"),); | ||
| env.events() | ||
| .publish(topics, (expert.clone(), amount)); | ||
| } |
There was a problem hiding this comment.
Event payload missing new_score per issue requirements.
Issue #55 specifies the event signature as expert_penalized(expert, penalty_points, new_score), but the implementation only emits (expert, amount). Consider adding new_score to the payload so event consumers can track the resulting reputation without an additional query.
Proposed fix
-/// Emitted when an expert is penalized via dispute.
-pub fn expert_penalized(env: &Env, expert: &Address, amount: u64) {
+/// Emitted when an expert is penalized via dispute.
+pub fn expert_penalized(env: &Env, expert: &Address, amount: u64, new_score: u64) {
let topics = (symbol_short!("penalize"),);
env.events()
- .publish(topics, (expert.clone(), amount));
+ .publish(topics, (expert.clone(), amount, new_score));
}This would also require updating the call site in contract.rs to pass stats.total_score after the saturating subtraction.
📝 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.
Suggested change
| /// Emitted when an expert is penalized via dispute. | |
| pub fn expert_penalized(env: &Env, expert: &Address, amount: u64) { | |
| let topics = (symbol_short!("penalize"),); | |
| env.events() | |
| .publish(topics, (expert.clone(), amount)); | |
| } | |
| /// Emitted when an expert is penalized via dispute. | |
| pub fn expert_penalized(env: &Env, expert: &Address, amount: u64, new_score: u64) { | |
| let topics = (symbol_short!("penalize"),); | |
| env.events() | |
| .publish(topics, (expert.clone(), amount, new_score)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/reputation-scoring-contract/src/events.rs` around lines 24 - 29,
The event payload for expert_penalized is missing the required new_score; update
the function signature expert_penalized(env: &Env, expert: &Address, amount:
u64, new_score: u64) and include new_score in the published payload
(publish(topics, (expert.clone(), amount, new_score))); then update the caller
in contract.rs that invokes expert_penalized to pass the updated
stats.total_score (the value after the saturating subtraction) as the new_score
argument so consumers receive the resulting reputation in the event.
Collaborator
|
also address comments from coderabbit |
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 #55
Test plan
Summary by CodeRabbit
New Features
Tests