feat: implemented Add metadata_uri and verification_status to Collateral struct#158
Merged
anonfedora merged 4 commits intoanonfedora:masterfrom Apr 2, 2026
Merged
feat: implemented Add metadata_uri and verification_status to Collateral struct#158anonfedora merged 4 commits intoanonfedora:masterfrom
anonfedora merged 4 commits intoanonfedora:masterfrom
Conversation
|
@Ugasutun Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCollateral records now include Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Escrow as EscrowManager
participant Storage as PersistentLedger
participant Events as EventSink
Client->>Escrow: dispute_escrow(escrow_id, caller)
Escrow->>Escrow: authorize caller & check status == Active
Escrow->>Storage: load escrow entry
Escrow->>Storage: update status -> Disputed
Escrow->>Storage: extend_ttl(DEFAULT_TTL_LEDGER_COUNT)
Escrow->>Events: emit esc_dsp(escrow_id, caller)
Escrow-->>Client: Ok / Error
sequenceDiagram
participant Admin as Admin
participant Collateral as CollateralRegistry
participant Storage as PersistentLedger
participant Events as EventSink
Admin->>Collateral: verify_collateral(id)
Collateral->>Storage: load collateral(id)
Collateral->>Collateral: set is_verified = true
Collateral->>Storage: persist updated collateral
Collateral->>Events: emit coll_verified(id)
Collateral-->>Admin: Ok / Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/collateral-registry/src/lib.rs`:
- Around line 387-390: The verify_collateral logic currently always sets
collateral.is_verified = true; change it to implement toggle/settable behavior
in the verify_collateral function: read the incoming intent (either a boolean
parameter like desired_verified or a toggle flag) and update
collateral.is_verified accordingly (either set to the provided value or flip its
current value), then persist with env.storage().persistent().set(&id,
&collateral) and publish a clear event reflecting the new state (use the
existing (symbol_short!("coll_verified"),) for verified and emit a corresponding
event or include the new boolean in the payload so callers can distinguish
verify vs unverify); ensure you update any function signature or event payload
used by verify_collateral to accept/propagate the desired state.
- Around line 47-48: The Collateral struct used for cross-contract calls was
changed to add metadata_uri: String and is_verified: bool in the
collateral-registry crate, but the Collateral definition in the risk-assessment
crate still uses the old shape; update the Collateral struct in the
risk-assessment code to include the new fields (metadata_uri and is_verified)
with the same types and ensure its (de)serialization derives match the registry
(e.g., Borsh/Serde derives) and update any places that construct, decode, or
pattern-match on Collateral (references: the Collateral struct, metadata_uri,
is_verified and any cross-contract decode/serialize helpers) so cross-contract
decoding succeeds.
- Around line 713-740: Remove the call to env.mock_all_auths() and instead
perform the setup and calls with explicit signer contexts: call
CollateralRegistry::initialize and CollateralRegistry::register_collateral as
the admin (so admin.require_auth() passes), then invoke
CollateralRegistry::verify_collateral using the unauthorized address (the
previously unused unauthorized variable) as the caller so admin.require_auth()
fails and the call returns Err(ContractError::Unauthorized); locate and update
the test around env.mock_all_auths(), CollateralRegistry::initialize,
register_collateral, and verify_collateral to use explicit signer context
methods (e.g., set source/auth to admin for setup and to unauthorized for the
failing verify) rather than mocking all auths.
🪄 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: 5fe84a6f-6ab3-4eb3-94b7-00d3be27bace
📒 Files selected for processing (1)
contracts/collateral-registry/src/lib.rs
Comment on lines
+47
to
+48
| pub metadata_uri: String, | ||
| pub is_verified: bool, |
There was a problem hiding this comment.
Cross-contract Collateral schema is now inconsistent and can break integration.
After adding metadata_uri and is_verified here, contracts/risk-assessment/src/lib.rs (Lines 274-286 in provided context) still uses the old Collateral shape. This can break cross-contract decoding at runtime.
Suggested alignment patch (risk-assessment side)
// contracts/risk-assessment/src/lib.rs
#[contracttype]
#[derive(Clone, Debug)]
pub struct Collateral {
pub id: u64,
pub owner: Address,
pub face_value: i128,
pub realized_value: i128,
pub expiry_ts: u64,
+ pub metadata_uri: String,
+ pub is_verified: bool,
pub registered_at: u64,
pub last_valuation_ts: u64,
pub locked: bool,
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/collateral-registry/src/lib.rs` around lines 47 - 48, The
Collateral struct used for cross-contract calls was changed to add metadata_uri:
String and is_verified: bool in the collateral-registry crate, but the
Collateral definition in the risk-assessment crate still uses the old shape;
update the Collateral struct in the risk-assessment code to include the new
fields (metadata_uri and is_verified) with the same types and ensure its
(de)serialization derives match the registry (e.g., Borsh/Serde derives) and
update any places that construct, decode, or pattern-match on Collateral
(references: the Collateral struct, metadata_uri, is_verified and any
cross-contract decode/serialize helpers) so cross-contract decoding succeeds.
Comment on lines
+387
to
+390
| collateral.is_verified = true; | ||
| env.storage().persistent().set(&id, &collateral); | ||
|
|
||
| env.events().publish((symbol_short!("coll_verified"),), (id,)); |
There was a problem hiding this comment.
verify_collateral does not implement toggle/set behavior from the objective.
Line 387 always forces is_verified = true. The linked objective calls for verification status to be toggle/settable; currently there is no way to unverify.
Possible fix (toggle behavior)
- collateral.is_verified = true;
+ collateral.is_verified = !collateral.is_verified;
env.storage().persistent().set(&id, &collateral);
- env.events().publish((symbol_short!("coll_verified"),), (id,));
+ env.events()
+ .publish((symbol_short!("coll_verified"),), (id, collateral.is_verified));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/collateral-registry/src/lib.rs` around lines 387 - 390, The
verify_collateral logic currently always sets collateral.is_verified = true;
change it to implement toggle/settable behavior in the verify_collateral
function: read the incoming intent (either a boolean parameter like
desired_verified or a toggle flag) and update collateral.is_verified accordingly
(either set to the provided value or flip its current value), then persist with
env.storage().persistent().set(&id, &collateral) and publish a clear event
reflecting the new state (use the existing (symbol_short!("coll_verified"),) for
verified and emit a corresponding event or include the new boolean in the
payload so callers can distinguish verify vs unverify); ensure you update any
function signature or event payload used by verify_collateral to
accept/propagate the desired state.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/escrow-manager/src/lib.rs`:
- Around line 628-640: dispute_escrow currently transitions EscrowStatus::Active
-> EscrowStatus::Disputed but there is no authorized path to move out of
Disputed, allowing funds to be frozen; add an explicit resolution flow:
introduce an authorization check and transition function(s) (or extend existing
ones) that allow an authorized resolver (e.g. arbitrator, buyer/seller
consensus, or lender via a new resolve_dispute function) to move
EscrowStatus::Disputed -> EscrowStatus::Released or Refund and call
env.storage().persistent().set to persist the updated status and clear/renew
TTL; update release_funds_on_confirmation and refund_escrow to accept
EscrowStatus::Disputed when the caller is an authorized resolver (check
identities/roles) or alternatively add resolve_dispute and resolve_dispute_ttl
functions that perform the state change, handle funds/collateral settlement, and
manage DEFAULT_TTL_LEDGER_COUNT accordingly so disputes can be legitimately
closed.
🪄 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: 07308041-6e7c-47bc-8d73-ceb4f37fd5f6
📒 Files selected for processing (1)
contracts/escrow-manager/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/escrow-manager/src/lib.rs (1)
1189-1231:⚠️ Potential issue | 🔴 CriticalTest calls non-existent
get_merchant_escrowsmethod — will not compile.The test
test_get_merchant_escrowscallst.escrow_client.get_merchant_escrows()at lines 1216, 1223, and 1229, but this method does not exist in theEscrowManagerimplementation. Either implement theget_merchant_escrowsmethod inEscrowManageror remove this test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/lib.rs` around lines 1189 - 1231, The test calls a non-existent method get_merchant_escrows on t.escrow_client (test_get_merchant_escrows), so add a corresponding public method on EscrowManager or remove the test; preferred fix: implement EscrowManager::get_merchant_escrows(&self, seller: &Address) -> Vec<EscrowId> (or the crate's escrow id type) that returns all escrow IDs where the escrow.seller == *seller by either maintaining a seller->Vec<EscrowId> index or scanning stored escrows, and ensure the method is exposed on the client (t.escrow_client) with matching signature used by the test.
🧹 Nitpick comments (1)
contracts/escrow-manager/src/lib.rs (1)
616-693: Missing test coverage for new dispute functions.The
dispute_escrowandrenew_dispute_ttlfunctions have no corresponding tests. Consider adding tests for:
dispute_escrowsuccess (buyer/seller/lender raises dispute)dispute_escrowwith unauthorized callerdispute_escrowon non-active escrowrenew_dispute_ttlsuccessrenew_dispute_ttlon non-disputed escrowrenew_dispute_ttlwith unauthorized callerWould you like me to generate the test cases for these functions?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/lib.rs` around lines 616 - 693, Add unit/integration tests covering dispute_escrow and renew_dispute_ttl: create escrows in Active state and assert dispute_escrow succeeds when called by escrow.buyer, escrow.seller, and escrow.lender and that Escrow.status becomes Disputed and event symbol_short!("esc_dsp") is published; assert dispute_escrow returns ContractError::Unauthorized for other callers and ContractError::EscrowNotActive when called on non-Active escrows; for renew_dispute_ttl, assert success when called by buyer/seller/lender on a Disputed escrow (and that symbol_short!("dsp_rnew") is published), assert ContractError::EscrowNotActive for non-Disputed escrows, and ContractError::Unauthorized for unauthorized callers; use the same storage setup helpers and error variants (EscrowNotFound, EscrowNotActive, Unauthorized) and functions dispute_escrow and renew_dispute_ttl to locate and exercise the logic.
🤖 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/escrow-manager/src/lib.rs`:
- Around line 674-677: The check that enforces TTL renewal only for disputed
escrows currently returns ContractError::EscrowNotActive which is misleading;
add a new error variant ContractError::EscrowNotDisputed (or similarly named) to
the error enum and replace the return that follows the EscrowStatus::Disputed
check with Err(ContractError::EscrowNotDisputed), and update the surrounding
comment to reflect that this guard specifically requires the escrow to be in
EscrowStatus::Disputed.
---
Outside diff comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 1189-1231: The test calls a non-existent method
get_merchant_escrows on t.escrow_client (test_get_merchant_escrows), so add a
corresponding public method on EscrowManager or remove the test; preferred fix:
implement EscrowManager::get_merchant_escrows(&self, seller: &Address) ->
Vec<EscrowId> (or the crate's escrow id type) that returns all escrow IDs where
the escrow.seller == *seller by either maintaining a seller->Vec<EscrowId> index
or scanning stored escrows, and ensure the method is exposed on the client
(t.escrow_client) with matching signature used by the test.
---
Nitpick comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 616-693: Add unit/integration tests covering dispute_escrow and
renew_dispute_ttl: create escrows in Active state and assert dispute_escrow
succeeds when called by escrow.buyer, escrow.seller, and escrow.lender and that
Escrow.status becomes Disputed and event symbol_short!("esc_dsp") is published;
assert dispute_escrow returns ContractError::Unauthorized for other callers and
ContractError::EscrowNotActive when called on non-Active escrows; for
renew_dispute_ttl, assert success when called by buyer/seller/lender on a
Disputed escrow (and that symbol_short!("dsp_rnew") is published), assert
ContractError::EscrowNotActive for non-Disputed escrows, and
ContractError::Unauthorized for unauthorized callers; use the same storage setup
helpers and error variants (EscrowNotFound, EscrowNotActive, Unauthorized) and
functions dispute_escrow and renew_dispute_ttl to locate and exercise the logic.
🪄 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: f05904d7-72e0-4717-924c-2b479e095e8c
📒 Files selected for processing (1)
contracts/escrow-manager/src/lib.rs
Comment on lines
+674
to
+677
| // Only allow TTL renewal for active disputes | ||
| if escrow.status != EscrowStatus::Disputed { | ||
| return Err(ContractError::EscrowNotActive); | ||
| } |
There was a problem hiding this comment.
Misleading error code for non-disputed escrows.
Returning EscrowNotActive when the escrow isn't in Disputed state is semantically confusing. An escrow in Active status would fail with EscrowNotActive, which is contradictory.
Consider adding a dedicated EscrowNotDisputed error variant for clarity, or at minimum update the comment to clarify the intent.
🔧 Proposed fix
Add a new error variant:
pub enum ContractError {
// ...existing variants...
ConsensusNotMet = 12,
+ EscrowNotDisputed = 13,
}Then use it:
// Only allow TTL renewal for active disputes
if escrow.status != EscrowStatus::Disputed {
- return Err(ContractError::EscrowNotActive);
+ return Err(ContractError::EscrowNotDisputed);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/escrow-manager/src/lib.rs` around lines 674 - 677, The check that
enforces TTL renewal only for disputed escrows currently returns
ContractError::EscrowNotActive which is misleading; add a new error variant
ContractError::EscrowNotDisputed (or similarly named) to the error enum and
replace the return that follows the EscrowStatus::Disputed check with
Err(ContractError::EscrowNotDisputed), and update the surrounding comment to
reflect that this guard specifically requires the escrow to be in
EscrowStatus::Disputed.
anonfedora
approved these changes
Mar 28, 2026
Owner
|
Fix failing CI, please |
anonfedora
approved these changes
Apr 2, 2026
4 tasks
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
2 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.
Closes #148
PR Description: Add metadata_uri and verification_status to Collateral struct
Overview
This PR adds external document link storage and verification state tracking to the CollateralRegistry smart contract in the StelloVault protocol.
Changes Made
1. Collateral Struct Enhancement
metadata_uri: Stringfield to store IPFS/S3 links to off-chain documentationis_verified: boolfield to track verification status2. New Function: verify_collateral
CollateralVerifiedevent on successful verification3. Updated register_collateral Function
metadata_uri: Stringparameter to accept external document links during registration4. Test Coverage
test_verify_collateral_success- Verifies successful admin verificationtest_verify_collateral_unauthorized- Ensures non-admin cannot verifytest_verify_collateral_not_found- Handles non-existent collateralDefinition of Done (DoD) Checklist
Files Modified
contracts/collateral-registry/src/lib.rsBreaking Changes
register_collateralfunction signature has changed to include an additional parameterSummary by CodeRabbit
New Features
Tests