Spike 1: Add Soroban Events for Indexer#69
Spike 1: Add Soroban Events for Indexer#69codebestia wants to merge 8 commits intoTrustless-Work:single-release-developfrom
Conversation
WalkthroughA new Rust-based smart contract module, "Contract-Events-for-Indexer," has been introduced to manage escrow workflows with Soroban event publishing for indexer integration. The implementation covers escrow initialization, funding, milestone management, dispute handling, and fund release, with comprehensive event emission, error handling, validation, fee calculation, storage structures, and extensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract
participant Token
participant Indexer
User->>Contract: initialize_escrow(escrow_props)
Contract->>Contract: validate & store escrow
Contract-->>Indexer: emit Event (escrow, created)
User->>Contract: fund_escrow(signer, amount)
Contract->>Token: transfer(signer -> contract, amount)
Contract-->>Indexer: emit Event (escrow, funded)
User->>Contract: change_milestone_status(index, status, evidence)
Contract->>Contract: update milestone
Contract-->>Indexer: emit Event (escrow, milestone_marked)
User->>Contract: approve_milestone(index, flag)
Contract->>Contract: update milestone approval
Contract-->>Indexer: emit Event (escrow, milestone_approved)
User->>Contract: release_funds(signer)
Contract->>Contract: validate & calculate fees
Contract->>Token: distribute funds
Contract-->>Indexer: emit Event (escrow, released)
User->>Contract: dispute_escrow(signer)
Contract->>Contract: set disputed flag
Contract-->>Indexer: emit Event (escrow, dispute_raised)
User->>Contract: resolve_dispute(resolver, splits)
Contract->>Contract: validate & distribute funds
Contract->>Token: distribute funds
Contract-->>Indexer: emit Event (escrow, dispute_resolved)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (21)
contracts/Contract-Events-for-Indexer/src/storage/types.rs (4)
18-25: Consider using an enum for milestone status instead of String.Using
Stringfor thestatusfield reduces type safety and could lead to inconsistent status values. Consider defining aMilestoneStatusenum with variants likePending,InProgress,Completed,Rejected.+#[contracttype] +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum MilestoneStatus { + Pending, + InProgress, + Completed, + Rejected, +} #[contracttype] #[derive(Clone, Debug, PartialEq, Eq)] pub struct Milestone { pub description: String, - pub status: String, + pub status: MilestoneStatus, pub evidence: String, pub approved: bool, }
27-36: Consider consolidating roles to reduce complexity.The
Rolesstruct defines 6 different address roles, which may increase gas costs and complexity. Consider whether some roles could be combined or if all are necessary for the escrow functionality.
54-59: Missing PartialEq and Eq traits for AddressBalance.The
AddressBalancestruct is missingPartialEqandEqderive traits, which are present on other structs and may be needed for comparisons and testing.#[contracttype] -#[derive(Clone)] +#[derive(Clone, PartialEq, Eq)] pub struct AddressBalance {
61-66: DataKey enum may need expansion for comprehensive storage.The
DataKeyenum currently only hasAdminandEscrowvariants. As the contract grows, you may need additional keys for storing milestones, disputes, or other entities separately.Consider adding variants like:
#[derive(Clone)] #[contracttype] pub enum DataKey { Admin, Escrow, + Milestone(String), // milestone_id + Dispute(String), // dispute_id + UserEscrows(Address), // user-specific escrow lists }contracts/Contract-Events-for-Indexer/Cargo.toml (1)
1-16: Well-configured Cargo.toml for Soroban smart contract development.The manifest is properly structured with correct crate type, workspace dependencies, and development features. The configuration is appropriate for a smart contract spike.
Consider adding metadata for better documentation:
[package] name = "Contract-Events-for-Indexer" version = "0.0.0" edition = "2021" publish = false +authors = ["Trustless Work Team"] +description = "Soroban escrow contract with comprehensive event indexing"contracts/Contract-Events-for-Indexer/src/events/handler.rs (1)
5-13: Event publishing implementation is correct but needs documentation.The event publishing logic is implemented correctly using Soroban's event system. The conversion to
Valtypes and payload construction follows best practices.However, consider the following improvements:
Document the cryptic event topic: The topic
"p_by_spdr"is unclear. Add documentation explaining what this represents.Add function documentation:
+/// Publishes an escrow event for indexing purposes. +/// +/// # Arguments +/// * `e` - The contract environment +/// * `escrow_id` - Unique identifier for the escrow +/// * `escrow` - The escrow data structure to be indexed pub fn escrows_by_contract_id(e: &Env, escrow_id: String, escrow: Escrow) {
- Consider more descriptive event topic:
- let topics = (symbol_short!("p_by_spdr"),); + let topics = (symbol_short!("escrow_upd"),); // or "esc_update"contracts/Contract-Events-for-Indexer/src/lib.rs (1)
39-42: Remove or update the "Work In Progress" comment.The storage module appears to be complete with comprehensive type definitions. The WIP comment may be outdated and could confuse future maintainers.
-/// This module is currently Work In Progress. +/// Storage types and data structures for the escrow contract. mod storage { pub mod types; }contracts/Contract-Events-for-Indexer/src/modules/math/basic.rs (1)
10-18: Excellent use of checked arithmetic for financial safety.The implementation correctly uses Rust's
checked_addandchecked_submethods to prevent overflow and underflow, which is critical for financial operations in smart contracts. The error handling prevents silent arithmetic failures.Consider adding documentation to explain the safety guarantees:
impl BasicArithmetic for BasicMath { + /// Safely adds two i128 values, returning an error on overflow. fn safe_add(a: i128, b: i128) -> Result<i128, ContractError> { a.checked_add(b).ok_or(ContractError::Overflow) } + /// Safely subtracts two i128 values, returning an error on underflow. fn safe_sub(a: i128, b: i128) -> Result<i128, ContractError> { a.checked_sub(b).ok_or(ContractError::Underflow) } }contracts/Contract-Events-for-Indexer/src/modules/math/safe.rs (1)
10-16: Consider adding input validation for divisor.While the implementation handles division by zero through
checked_div, consider adding explicit validation fordivisor == 0at the beginning of the function for clearer error messaging and early return.fn safe_mul_div(amount: i128, multiplier: i128, divisor: i128) -> Result<i128, ContractError> { + if divisor == 0 { + return Err(ContractError::DivisionError); + } + amount .checked_mul(multiplier) .ok_or(ContractError::Overflow)? .checked_div(divisor) .ok_or(ContractError::DivisionError) }contracts/Contract-Events-for-Indexer/README.md (1)
22-26: Consider adding concrete examples and implementation details.The recommendations are valuable, but the documentation would benefit from including:
- Specific event structure examples with data types
- Code snippets showing event emission patterns
- Testing strategy beyond basic event publishing
- Performance benchmarks for event publishing costs
contracts/Contract-Events-for-Indexer/src/core/validators/milestone.rs (1)
19-21: Consider refactoring duplicate bounds checking logic.Both validation functions contain identical bounds checking logic. Consider extracting this into a helper function to reduce code duplication.
+fn validate_milestone_index(escrow: &Escrow, milestone_index: i128) -> Result<(), ContractError> { + if escrow.milestones.is_empty() { + return Err(ContractError::NoMileStoneDefined); + } + + if milestone_index < 0 || milestone_index >= escrow.milestones.len() as i128 { + return Err(ContractError::InvalidMileStoneIndex); + } + + Ok(()) +}Also applies to: 40-42
contracts/Contract-Events-for-Indexer/src/core/validators/escrow.rs (2)
69-72: Consider using reference instead of taking ownership.The
escrow_propertiesparameter is only used for reading values, so it should take a reference instead of ownership for better performance and API design.pub fn validate_initialize_escrow_conditions( e: Env, - escrow_properties: Escrow, + escrow_properties: &Escrow, ) -> Result<(), ContractError> {
81-83: Consider making milestone limit configurable.The hardcoded limit of 10 milestones should be made configurable or defined as a constant to improve maintainability.
+const MAX_MILESTONES: usize = 10; + pub fn validate_initialize_escrow_conditions( // ... parameters ) -> Result<(), ContractError> { // ... other validations - if escrow_properties.milestones.len() > 10 { + if escrow_properties.milestones.len() > MAX_MILESTONES { return Err(ContractError::TooManyMilestones); }contracts/Contract-Events-for-Indexer/src/core/milestone.rs (3)
30-33: Remove redundant milestone index validationThe milestone index is already validated in
validate_milestone_status_change_conditionsat line 24-28. The.ok_or()check here is redundant since the validation ensures the index is valid.-let mut milestone_to_update = existing_escrow - .milestones - .get(milestone_index as u32) - .ok_or(ContractError::InvalidMileStoneIndex)?; +let mut milestone_to_update = existing_escrow + .milestones + .get(milestone_index as u32) + .unwrap();
38-38: Remove trailing whitespace- +
62-65: Remove redundant milestone index validationSame as the previous method, the milestone index is already validated in
validate_milestone_flag_change_conditions. The.ok_or()check is redundant.-let mut milestone_to_update = existing_escrow - .milestones - .get(milestone_index as u32) - .ok_or(ContractError::InvalidMileStoneIndex)?; +let mut milestone_to_update = existing_escrow + .milestones + .get(milestone_index as u32) + .unwrap();contracts/Contract-Events-for-Indexer/src/core/dispute.rs (1)
35-37: Consider removing redundant balance checkThis balance check appears redundant since
validate_dispute_resolution_conditionsat line 47 also validates funds against the current balance.contracts/Contract-Events-for-Indexer/src/tests/test.rs (4)
13-13: Remove commented out importThis commented import is not being used and should be removed to keep the code clean.
-// use test_token::token::{Token, TokenClient};
528-528: Consider extracting hardcoded commission valueThe trustless work commission calculation uses a hardcoded value of 30 (basis points). Consider extracting this to a constant to ensure it matches the contract's actual commission rate.
+ const TRUSTLESS_WORK_COMMISSION_BPS: i128 = 30; // 0.3% let total_amount = amount as i128; - let trustless_work_commission = ((total_amount * 30) / 10000) as i128; + let trustless_work_commission = ((total_amount * TRUSTLESS_WORK_COMMISSION_BPS) / 10000) as i128;
788-788: Consider extracting hardcoded commission valueThe trustless work commission calculation uses a hardcoded value of 30 (basis points). Consider extracting this to a constant for consistency.
+ const TRUSTLESS_WORK_COMMISSION_BPS: i128 = 30; // 0.3% let total_amount = amount as i128; - let trustless_work_commission = ((total_amount * 30) / 10000) as i128; + let trustless_work_commission = ((total_amount * TRUSTLESS_WORK_COMMISSION_BPS) / 10000) as i128;
1132-1132: Consider extracting hardcoded commission valueThe trustless work commission calculation uses a hardcoded value of 30 (basis points). Consider extracting this to a constant for consistency across all tests.
+ const TRUSTLESS_WORK_COMMISSION_BPS: i128 = 30; // 0.3% let total_amount = amount as i128; - let trustless_work_commission = ((total_amount * 30) / 10000) as i128; + let trustless_work_commission = ((total_amount * TRUSTLESS_WORK_COMMISSION_BPS) / 10000) as i128;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.DS_Storeis excluded by!**/.DS_StoreCargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
contracts/Contract-Events-for-Indexer/Cargo.toml(1 hunks)contracts/Contract-Events-for-Indexer/README.md(1 hunks)contracts/Contract-Events-for-Indexer/src/contract.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/core/dispute.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/core/escrow.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/core/milestone.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/core/validators/dispute.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/core/validators/escrow.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/core/validators/milestone.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/error.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/events/handler.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/lib.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/modules/fee/calculator.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/modules/math/basic.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/modules/math/safe.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/storage/types.rs(1 hunks)contracts/Contract-Events-for-Indexer/src/tests/test.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
contracts/Contract-Events-for-Indexer/src/lib.rs (1)
contracts/Contract-Events-for-Indexer/src/events/handler.rs (1)
escrows_by_contract_id(5-13)
contracts/Contract-Events-for-Indexer/src/core/milestone.rs (3)
contracts/Contract-Events-for-Indexer/src/events/handler.rs (1)
escrows_by_contract_id(5-13)contracts/Contract-Events-for-Indexer/src/core/validators/milestone.rs (2)
validate_milestone_flag_change_conditions(27-45)validate_milestone_status_change_conditions(6-24)contracts/Contract-Events-for-Indexer/src/core/escrow.rs (2)
e(126-126)get_escrow(129-134)
contracts/Contract-Events-for-Indexer/src/core/dispute.rs (6)
contracts/Contract-Events-for-Indexer/src/events/handler.rs (1)
escrows_by_contract_id(5-13)contracts/Contract-Events-for-Indexer/src/core/validators/dispute.rs (2)
validate_dispute_flag_change_conditions(43-72)validate_dispute_resolution_conditions(10-40)contracts/Contract-Events-for-Indexer/src/contract.rs (3)
resolve_dispute(164-184)get_escrow(101-103)dispute_escrow(186-194)contracts/Contract-Events-for-Indexer/src/core/escrow.rs (3)
e(126-126)get_escrow(129-134)get_receiver(16-22)contracts/Contract-Events-for-Indexer/src/modules/math/basic.rs (2)
safe_add(6-6)safe_add(11-13)contracts/Contract-Events-for-Indexer/src/modules/fee/calculator.rs (2)
calculate_dispute_fees(33-38)calculate_dispute_fees(67-107)
contracts/Contract-Events-for-Indexer/src/modules/fee/calculator.rs (2)
contracts/Contract-Events-for-Indexer/src/modules/math/safe.rs (2)
safe_mul_div(6-6)safe_mul_div(10-16)contracts/Contract-Events-for-Indexer/src/modules/math/basic.rs (4)
safe_sub(7-7)safe_sub(15-17)safe_add(6-6)safe_add(11-13)
contracts/Contract-Events-for-Indexer/src/core/escrow.rs (3)
contracts/Contract-Events-for-Indexer/src/core/validators/escrow.rs (3)
validate_escrow_property_change_conditions(41-66)validate_initialize_escrow_conditions(69-86)validate_release_conditions(9-38)contracts/Contract-Events-for-Indexer/src/contract.rs (6)
initialize_escrow(44-53)fund_escrow(55-67)get_escrow(101-103)release_funds(69-85)get_multiple_escrow_balances(112-118)get_escrow_by_contract_id(105-110)contracts/Contract-Events-for-Indexer/src/modules/fee/calculator.rs (2)
calculate_standard_fees(28-31)calculate_standard_fees(45-65)
🔇 Additional comments (17)
contracts/Contract-Events-for-Indexer/src/storage/types.rs (1)
3-16: Well-structured core escrow entity with comprehensive fields.The
Escrowstruct effectively captures all necessary escrow data. The field organization is logical and the use of#[contracttype]with appropriate derive traits is correct for Soroban contracts.contracts/Contract-Events-for-Indexer/src/lib.rs (3)
1-16: Excellent module organization with proper separation of concerns.The module structure is well-designed with logical groupings for core business logic, validators, events, and utilities. The re-exports provide a clean API surface.
18-21: Appropriate visibility for event handler re-export.The use of
pub(crate)for the event handler re-export is correct, keeping event publishing internal to the contract while making it accessible across modules.
48-48: EscrowContract implementation verifiedThe
EscrowContractstruct is defined with#[contract]and has a#[contractimpl]block in contracts/Contract-Events-for-Indexer/src/contract.rs (and similarly in the escrow crate). All required Soroban methods are present. No changes needed.contracts/Contract-Events-for-Indexer/src/modules/math/basic.rs (2)
3-8: Well-designed trait-based approach for safe arithmetic.The trait abstraction allows for different implementations and the method signatures are appropriate for safe arithmetic operations in financial contracts.
15-17: ContractError::Underflow is defined and correctly usedThe
Underflowvariant is declared incontracts/Contract-Events-for-Indexer/src/error.rs(line 39) with a matchingDisplayimplementation (“This operation can cause an Underflow”). Its semantics align withchecked_subfailing on out‐of‐bounds subtraction. No changes needed here.contracts/Contract-Events-for-Indexer/src/modules/math/safe.rs (1)
1-17: Well-implemented safe arithmetic module with proper overflow protection.The implementation correctly uses Rust's
checked_mulandchecked_divto prevent arithmetic overflow and division by zero errors. The error handling is appropriate and integrates well with the contract's error system.contracts/Contract-Events-for-Indexer/README.md (1)
1-26: Comprehensive documentation with clear structure and practical insights.The README effectively documents the spike findings and provides valuable context for the Soroban events implementation. The structure covering overview, findings, assumptions, and recommendations is well-organized.
contracts/Contract-Events-for-Indexer/src/error.rs (1)
4-52: Comprehensive error handling system with good coverage.The error enum provides excellent coverage of all contract operations with unique error codes and descriptive variants. The integration with Soroban SDK through the
contracterrorattribute is appropriate.contracts/Contract-Events-for-Indexer/src/core/validators/escrow.rs (2)
8-38: Solid release validation logic with comprehensive checks.The validation correctly checks all necessary conditions for fund release: authorization, completion status, milestone approval, and dispute state. The logic flow is appropriate and follows fail-fast principles.
21-23: All milestone-related error variant usages match definitions in error.rsI’ve confirmed that each
ContractErrorvariant used in the milestone and escrow validators (e.g.NoMileStoneDefined,InvalidMileStoneIndex,OnlyServiceProviderChangeMilstoneStatus,OnlyApproverChangeMilstoneFlag) exactly matches the enum entries insrc/error.rs. There are no reference mismatches in these validator files.If the goal is to correct the spelling of “Milstone” to “Milestone,” that should be done in
error.rs(and then propagated here). As-is, no changes are required in the validator code.contracts/Contract-Events-for-Indexer/src/core/validators/dispute.rs (2)
43-72: Well-structured authorization validationThe validation logic correctly checks for duplicate disputes and comprehensively validates all authorized roles.
31-37: Fix incorrect fund validation logicThe validation conditions appear to be inverted. You're checking if the input funds are less than the net funds (after fees), but the net funds should always be less than the input funds since fees are deducted.
-if approver_funds < fee_result.net_approver_funds { - return Err(ContractError::InsufficientApproverFundsForCommissions); -} - -if receiver_funds < fee_result.net_receiver_funds { - return Err(ContractError::InsufficientServiceProviderFundsForCommissions); -} +if fee_result.net_approver_funds > approver_funds { + return Err(ContractError::InsufficientApproverFundsForCommissions); +} + +if fee_result.net_receiver_funds > receiver_funds { + return Err(ContractError::InsufficientReceiverFundsForCommissions); +}Also note that the error message on line 36 mentions "ServiceProviderFunds" but should be "ReceiverFunds" for consistency.
Likely an incorrect or invalid review comment.
contracts/Contract-Events-for-Indexer/src/modules/fee/calculator.rs (2)
45-65: Correct fee calculation implementationThe standard fee calculation correctly uses safe arithmetic and properly deducts fees in sequence to avoid negative values.
67-107: Well-implemented proportional fee distributionThe dispute fee calculation correctly:
- Handles the edge case of zero total funds
- Distributes fees proportionally based on each party's share
- Uses safe arithmetic to prevent overflow/underflow
contracts/Contract-Events-for-Indexer/src/core/dispute.rs (2)
78-89: Clean dispute flagging implementationThe method correctly follows the pattern of authentication, validation, state update, and event emission.
57-67: Ensure token transfer failures are handled explicitlyBefore finalizing, please confirm whether
TokenClient::transferreturns aResult<_, _>or panics on failure. If it returns aResult, propagate errors with?or handle them; if it panics, consider matching its return value (if any) or wrapping the call in a recoverable construct. Inresolve_dispute(contracts/Contract-Events-for-Indexer/src/core/dispute.rs), each of these calls needs explicit error handling:
- Line 57:
token_client.transfer(&contract_address, &trustless_work_address, &fee_result.trustless_work_fee);- Line 58:
token_client.transfer(&contract_address, &escrow.roles.platform_address, &fee_result.platform_fee);- Line 61 (inside
if net_approver_funds > 0):
token_client.transfer(&contract_address, &escrow.roles.approver, &fee_result.net_approver_funds);- Line 65 (inside
if net_receiver_funds > 0):
token_client.transfer(&contract_address, &receiver, &fee_result.net_receiver_funds);Suggested patterns:
- token_client.transfer(...); + token_client.transfer(...)?; // propagate erroror
if let Err(e) = token_client.transfer(...) { return Err(ContractError::TokenTransferFailed); }
| pub fn update_escrow( | ||
| e: Env, | ||
| plataform_address: Address, | ||
| escrow_properties: Escrow, | ||
| ) -> Result<Escrow, ContractError> { | ||
| let updated_escrow = EscrowManager::change_escrow_properties( | ||
| e.clone(), | ||
| plataform_address.clone(), | ||
| escrow_properties.clone(), | ||
| )?; | ||
|
|
||
| Ok(updated_escrow) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add event emission for escrow updates
The update_escrow function modifies escrow state but doesn't emit an event. For consistency with other state-changing functions and to support proper indexing, an event should be emitted.
pub fn update_escrow(
e: Env,
- plataform_address: Address,
+ platform_address: Address,
escrow_properties: Escrow,
) -> Result<Escrow, ContractError> {
let updated_escrow = EscrowManager::change_escrow_properties(
e.clone(),
- plataform_address.clone(),
+ platform_address.clone(),
escrow_properties.clone(),
)?;
+ e.events().publish(
+ (symbol_short!("escrow"), symbol_short!("updated")),
+ (updated_escrow.engagement_id.clone(), platform_address)
+ );
Ok(updated_escrow)
}
📝 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 update_escrow( | |
| e: Env, | |
| plataform_address: Address, | |
| escrow_properties: Escrow, | |
| ) -> Result<Escrow, ContractError> { | |
| let updated_escrow = EscrowManager::change_escrow_properties( | |
| e.clone(), | |
| plataform_address.clone(), | |
| escrow_properties.clone(), | |
| )?; | |
| Ok(updated_escrow) | |
| } | |
| pub fn update_escrow( | |
| e: Env, | |
| platform_address: Address, | |
| escrow_properties: Escrow, | |
| ) -> Result<Escrow, ContractError> { | |
| let updated_escrow = EscrowManager::change_escrow_properties( | |
| e.clone(), | |
| platform_address.clone(), | |
| escrow_properties.clone(), | |
| )?; | |
| e.events().publish( | |
| (symbol_short!("escrow"), symbol_short!("updated")), | |
| (updated_escrow.engagement_id.clone(), platform_address) | |
| ); | |
| Ok(updated_escrow) | |
| } |
🤖 Prompt for AI Agents
In contracts/Contract-Events-for-Indexer/src/contract.rs around lines 87 to 99,
the update_escrow function updates escrow state but does not emit an event.
Modify the function to emit an event after successfully updating the escrow by
calling the appropriate event emission method with relevant escrow update
details. This will ensure consistency with other state-changing functions and
support proper indexing.
|
|
||
| pub fn update_escrow( | ||
| e: Env, | ||
| plataform_address: Address, |
There was a problem hiding this comment.
Fix typo in parameter name
The parameter name contains a typo: "plataform_address" should be "platform_address".
- plataform_address: Address,
+ platform_address: Address,
Also update the usage on line 94:
- plataform_address.clone(),
+ platform_address.clone(),
📝 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.
| plataform_address: Address, | |
| pub fn update_escrow( | |
| env: Env, | |
| engagement_id: u64, | |
| - plataform_address: Address, | |
| + platform_address: Address, | |
| arbiter_address: Address, | |
| dispute_resolution_address: Address, | |
| ) -> Result<(), Error> { | |
| // Delegate to manager with updated properties | |
| EscrowManager::change_escrow_properties( | |
| env, | |
| engagement_id, | |
| - plataform_address.clone(), | |
| + platform_address.clone(), | |
| arbiter_address.clone(), | |
| dispute_resolution_address.clone(), | |
| )?; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In contracts/Contract-Events-for-Indexer/src/contract.rs at line 89, correct the
typo in the parameter name from "plataform_address" to "platform_address". Also,
update all usages of this parameter, including the one at line 94, to use the
corrected name "platform_address" to maintain consistency and avoid errors.
| pub fn fund_escrow( | ||
| e: Env, | ||
| signer: Address, | ||
| amount_to_deposit: i128, | ||
| ) -> Result<(), ContractError> { | ||
| signer.require_auth(); | ||
| let escrow = Self::get_escrow(e.clone())?; | ||
| let token_client = TokenClient::new(&e, &escrow.trustline.address); | ||
| token_client.transfer(&signer, &e.current_contract_address(), &amount_to_deposit); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add amount validation and error handling
The method should validate that amount_to_deposit is positive and handle potential transfer failures.
pub fn fund_escrow(
e: Env,
signer: Address,
amount_to_deposit: i128,
) -> Result<(), ContractError> {
signer.require_auth();
+ if amount_to_deposit <= 0 {
+ return Err(ContractError::AmountCannotBeZero);
+ }
let escrow = Self::get_escrow(e.clone())?;
let token_client = TokenClient::new(&e, &escrow.trustline.address);
token_client.transfer(&signer, &e.current_contract_address(), &amount_to_deposit);
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 fund_escrow( | |
| e: Env, | |
| signer: Address, | |
| amount_to_deposit: i128, | |
| ) -> Result<(), ContractError> { | |
| signer.require_auth(); | |
| let escrow = Self::get_escrow(e.clone())?; | |
| let token_client = TokenClient::new(&e, &escrow.trustline.address); | |
| token_client.transfer(&signer, &e.current_contract_address(), &amount_to_deposit); | |
| Ok(()) | |
| } | |
| pub fn fund_escrow( | |
| e: Env, | |
| signer: Address, | |
| amount_to_deposit: i128, | |
| ) -> Result<(), ContractError> { | |
| signer.require_auth(); | |
| if amount_to_deposit <= 0 { | |
| return Err(ContractError::AmountCannotBeZero); | |
| } | |
| let escrow = Self::get_escrow(e.clone())?; | |
| let token_client = TokenClient::new(&e, &escrow.trustline.address); | |
| token_client.transfer(&signer, &e.current_contract_address(), &amount_to_deposit); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In contracts/Contract-Events-for-Indexer/src/core/escrow.rs around lines 32 to
42, the fund_escrow function lacks validation for the amount_to_deposit and does
not handle transfer errors. Add a check to ensure amount_to_deposit is positive,
returning an appropriate ContractError if not. Also, modify the
token_client.transfer call to handle its Result return value, propagating any
errors instead of ignoring them.
| escrow.flags.released = true; | ||
| e.storage().instance().set(&DataKey::Escrow, &escrow); | ||
|
|
||
| let contract_address = e.current_contract_address(); | ||
| let token_client = TokenClient::new(&e, &escrow.trustline.address); | ||
|
|
||
| if token_client.balance(&contract_address) < escrow.amount { | ||
| return Err(ContractError::EscrowBalanceNotEnoughToSendEarnings); | ||
| } | ||
|
|
||
| let fee_result = | ||
| FeeCalculator::calculate_standard_fees(escrow.amount as i128, escrow.platform_fee as i128)?; | ||
|
|
||
| token_client.transfer(&contract_address, &trustless_work_address, &fee_result.trustless_work_fee); | ||
| token_client.transfer(&contract_address, &escrow.roles.platform_address, &fee_result.platform_fee); | ||
|
|
||
| let receiver = Self::get_receiver(&escrow); | ||
| token_client.transfer(&contract_address, &receiver, &fee_result.receiver_amount); |
There was a problem hiding this comment.
Critical: Update state after successful transfers
The escrow state is marked as released before the token transfers occur. If any transfer fails, the escrow will be in an inconsistent state (marked as released but funds not distributed).
validate_release_conditions(&escrow, &release_signer)?;
-escrow.flags.released = true;
-e.storage().instance().set(&DataKey::Escrow, &escrow);
-
let contract_address = e.current_contract_address();
let token_client = TokenClient::new(&e, &escrow.trustline.address);
if token_client.balance(&contract_address) < escrow.amount {
return Err(ContractError::EscrowBalanceNotEnoughToSendEarnings);
}
let fee_result =
FeeCalculator::calculate_standard_fees(escrow.amount as i128, escrow.platform_fee as i128)?;
token_client.transfer(&contract_address, &trustless_work_address, &fee_result.trustless_work_fee);
token_client.transfer(&contract_address, &escrow.roles.platform_address, &fee_result.platform_fee);
let receiver = Self::get_receiver(&escrow);
token_client.transfer(&contract_address, &receiver, &fee_result.receiver_amount);
+
+escrow.flags.released = true;
+e.storage().instance().set(&DataKey::Escrow, &escrow);
📝 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.
| escrow.flags.released = true; | |
| e.storage().instance().set(&DataKey::Escrow, &escrow); | |
| let contract_address = e.current_contract_address(); | |
| let token_client = TokenClient::new(&e, &escrow.trustline.address); | |
| if token_client.balance(&contract_address) < escrow.amount { | |
| return Err(ContractError::EscrowBalanceNotEnoughToSendEarnings); | |
| } | |
| let fee_result = | |
| FeeCalculator::calculate_standard_fees(escrow.amount as i128, escrow.platform_fee as i128)?; | |
| token_client.transfer(&contract_address, &trustless_work_address, &fee_result.trustless_work_fee); | |
| token_client.transfer(&contract_address, &escrow.roles.platform_address, &fee_result.platform_fee); | |
| let receiver = Self::get_receiver(&escrow); | |
| token_client.transfer(&contract_address, &receiver, &fee_result.receiver_amount); | |
| validate_release_conditions(&escrow, &release_signer)?; | |
| let contract_address = e.current_contract_address(); | |
| let token_client = TokenClient::new(&e, &escrow.trustline.address); | |
| if token_client.balance(&contract_address) < escrow.amount { | |
| return Err(ContractError::EscrowBalanceNotEnoughToSendEarnings); | |
| } | |
| let fee_result = | |
| FeeCalculator::calculate_standard_fees(escrow.amount as i128, escrow.platform_fee as i128)?; | |
| token_client.transfer(&contract_address, &trustless_work_address, &fee_result.trustless_work_fee); | |
| token_client.transfer(&contract_address, &escrow.roles.platform_address, &fee_result.platform_fee); | |
| let receiver = Self::get_receiver(&escrow); | |
| token_client.transfer(&contract_address, &receiver, &fee_result.receiver_amount); | |
| escrow.flags.released = true; | |
| e.storage().instance().set(&DataKey::Escrow, &escrow); |
🤖 Prompt for AI Agents
In contracts/Contract-Events-for-Indexer/src/core/escrow.rs around lines 53 to
70, the escrow state is set to released before the token transfers, risking
inconsistent state if a transfer fails. To fix this, move the line setting
escrow.flags.released = true and the subsequent state update to after all token
transfers have successfully completed, ensuring the state only updates once all
transfers succeed.
| pub fn get_multiple_escrow_balances( | ||
| e: Env, | ||
| signer: Address, | ||
| addresses: Vec<Address>, | ||
| ) -> Result<Vec<AddressBalance>, ContractError> { | ||
| signer.require_auth(); | ||
| const MAX_ESCROWS: u32 = 20; | ||
| if addresses.len() > MAX_ESCROWS { | ||
| return Err(ContractError::TooManyEscrowsRequested); | ||
| } | ||
|
|
||
| let mut balances: Vec<AddressBalance> = Vec::new(&e); | ||
| for address in addresses.iter() { | ||
| let escrow = Self::get_escrow_by_contract_id(e.clone(), &address)?; | ||
| let token_client = TokenClient::new(&e, &escrow.trustline.address); | ||
| let balance = token_client.balance(&address); | ||
| balances.push_back(AddressBalance { | ||
| address: address.clone(), | ||
| balance, | ||
| trustline_decimals: escrow.trustline.decimals, | ||
| }); | ||
| } | ||
| Ok(balances) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle cross-contract call failures and consider performance
The method makes multiple cross-contract calls in a loop which could fail and may have performance implications.
Consider:
- Wrapping the cross-contract call in error handling to continue on failure
- Implementing batch operations if the number of queries is frequently high
- Documenting the gas cost implications of multiple cross-contract calls
for address in addresses.iter() {
- let escrow = Self::get_escrow_by_contract_id(e.clone(), &address)?;
- let token_client = TokenClient::new(&e, &escrow.trustline.address);
- let balance = token_client.balance(&address);
- balances.push_back(AddressBalance {
- address: address.clone(),
- balance,
- trustline_decimals: escrow.trustline.decimals,
- });
+ match Self::get_escrow_by_contract_id(e.clone(), &address) {
+ Ok(escrow) => {
+ let token_client = TokenClient::new(&e, &escrow.trustline.address);
+ let balance = token_client.balance(&address);
+ balances.push_back(AddressBalance {
+ address: address.clone(),
+ balance,
+ trustline_decimals: escrow.trustline.decimals,
+ });
+ }
+ Err(_) => {
+ // Skip failed contract calls or add default entry
+ continue;
+ }
+ }
}
📝 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 get_multiple_escrow_balances( | |
| e: Env, | |
| signer: Address, | |
| addresses: Vec<Address>, | |
| ) -> Result<Vec<AddressBalance>, ContractError> { | |
| signer.require_auth(); | |
| const MAX_ESCROWS: u32 = 20; | |
| if addresses.len() > MAX_ESCROWS { | |
| return Err(ContractError::TooManyEscrowsRequested); | |
| } | |
| let mut balances: Vec<AddressBalance> = Vec::new(&e); | |
| for address in addresses.iter() { | |
| let escrow = Self::get_escrow_by_contract_id(e.clone(), &address)?; | |
| let token_client = TokenClient::new(&e, &escrow.trustline.address); | |
| let balance = token_client.balance(&address); | |
| balances.push_back(AddressBalance { | |
| address: address.clone(), | |
| balance, | |
| trustline_decimals: escrow.trustline.decimals, | |
| }); | |
| } | |
| Ok(balances) | |
| } | |
| pub fn get_multiple_escrow_balances( | |
| e: Env, | |
| signer: Address, | |
| addresses: Vec<Address>, | |
| ) -> Result<Vec<AddressBalance>, ContractError> { | |
| signer.require_auth(); | |
| const MAX_ESCROWS: u32 = 20; | |
| if addresses.len() > MAX_ESCROWS { | |
| return Err(ContractError::TooManyEscrowsRequested); | |
| } | |
| let mut balances: Vec<AddressBalance> = Vec::new(&e); | |
| for address in addresses.iter() { | |
| match Self::get_escrow_by_contract_id(e.clone(), &address) { | |
| Ok(escrow) => { | |
| let token_client = TokenClient::new(&e, &escrow.trustline.address); | |
| let balance = token_client.balance(&address); | |
| balances.push_back(AddressBalance { | |
| address: address.clone(), | |
| balance, | |
| trustline_decimals: escrow.trustline.decimals, | |
| }); | |
| } | |
| Err(_) => { | |
| // Skip failed contract lookups; optionally record defaults or log | |
| continue; | |
| } | |
| } | |
| } | |
| Ok(balances) | |
| } |
🤖 Prompt for AI Agents
In contracts/Contract-Events-for-Indexer/src/core/escrow.rs around lines 97 to
120, the get_multiple_escrow_balances function makes multiple cross-contract
calls inside a loop without error handling, which can cause the entire call to
fail if one call fails and may impact performance. Modify the code to wrap each
cross-contract call in error handling so that failures for individual addresses
do not abort the entire process, allowing the function to continue processing
other addresses. Additionally, consider implementing batch operations if
frequent high-volume queries occur to optimize performance. Finally, add
documentation comments explaining the gas cost implications of making multiple
cross-contract calls in this method.
| ContractError::OnlyServiceProviderChangeMilstoneStatus => { | ||
| write!(f, "Only the service provider can change milestone status") | ||
| } | ||
| ContractError::NoMileStoneDefined => write!(f, "Escrow initialized without milestone"), | ||
| ContractError::InvalidMileStoneIndex => write!(f, "Invalid milestone index"), | ||
| ContractError::OnlyApproverChangeMilstoneFlag => { | ||
| write!(f, "Only the approver can change milestone flag") | ||
| } |
There was a problem hiding this comment.
Update Display implementation to match corrected error variants.
The Display implementation needs to be updated to match the corrected error variant names.
- ContractError::OnlyServiceProviderChangeMilstoneStatus => {
+ ContractError::OnlyServiceProviderChangeMilestoneStatus => {
write!(f, "Only the service provider can change milestone status")
}
- ContractError::NoMileStoneDefined => write!(f, "Escrow initialized without milestone"),
- ContractError::InvalidMileStoneIndex => write!(f, "Invalid milestone index"),
- ContractError::OnlyApproverChangeMilstoneFlag => {
+ ContractError::NoMilestoneDefined => write!(f, "Escrow initialized without milestone"),
+ ContractError::InvalidMilestoneIndex => write!(f, "Invalid milestone index"),
+ ContractError::OnlyApproverChangeMilestoneFlag => {
write!(f, "Only the approver can change milestone flag")
}
📝 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.
| ContractError::OnlyServiceProviderChangeMilstoneStatus => { | |
| write!(f, "Only the service provider can change milestone status") | |
| } | |
| ContractError::NoMileStoneDefined => write!(f, "Escrow initialized without milestone"), | |
| ContractError::InvalidMileStoneIndex => write!(f, "Invalid milestone index"), | |
| ContractError::OnlyApproverChangeMilstoneFlag => { | |
| write!(f, "Only the approver can change milestone flag") | |
| } | |
| ContractError::OnlyServiceProviderChangeMilestoneStatus => { | |
| write!(f, "Only the service provider can change milestone status") | |
| } | |
| ContractError::NoMilestoneDefined => write!(f, "Escrow initialized without milestone"), | |
| ContractError::InvalidMilestoneIndex => write!(f, "Invalid milestone index"), | |
| ContractError::OnlyApproverChangeMilestoneFlag => { | |
| write!(f, "Only the approver can change milestone flag") | |
| } |
🤖 Prompt for AI Agents
In contracts/Contract-Events-for-Indexer/src/error.rs around lines 103 to 110,
the Display implementation uses outdated or incorrect error variant names.
Review the current enum variant names for ContractError and update all
references in the Display match arms to use the corrected variant names exactly
as defined, ensuring consistency and correctness in error message handling.
| ContractError::AmountToDepositGreatherThanEscrowAmount => { | ||
| write!(f, "Amount to deposit is greater than the escrow amount") | ||
| } |
There was a problem hiding this comment.
Fix typo in error variant name.
There's a spelling error in the error variant name.
- AmountToDepositGreatherThanEscrowAmount => {
- write!(f, "Amount to deposit is greater than the escrow amount")
+ AmountToDepositGreaterThanEscrowAmount => {
+ write!(f, "Amount to deposit is greater than the escrow amount")
📝 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.
| ContractError::AmountToDepositGreatherThanEscrowAmount => { | |
| write!(f, "Amount to deposit is greater than the escrow amount") | |
| } | |
| ContractError::AmountToDepositGreaterThanEscrowAmount => { | |
| write!(f, "Amount to deposit is greater than the escrow amount") | |
| } |
🤖 Prompt for AI Agents
In contracts/Contract-Events-for-Indexer/src/error.rs around lines 123 to 125,
the error variant name "AmountToDepositGreatherThanEscrowAmount" contains a
typo. Rename the variant to "AmountToDepositGreaterThanEscrowAmount" to correct
the spelling of "Greather" to "Greater" consistently in the codebase.
| } | ||
|
|
||
| #[test] | ||
| fn test_initialize_excrow() { |
There was a problem hiding this comment.
Fix typo in test function name
The function name contains a typo: "excrow" should be "escrow".
-fn test_initialize_excrow() {
+fn test_initialize_escrow() {
📝 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.
| fn test_initialize_excrow() { | |
| fn test_initialize_escrow() { |
🤖 Prompt for AI Agents
In contracts/Contract-Events-for-Indexer/src/tests/test.rs at line 46, the test
function name has a typo: "test_initialize_excrow" should be corrected to
"test_initialize_escrow". Rename the function to fix the spelling mistake.
| let _receiver_address = Address::generate(&env); | ||
|
|
||
| let amount: i128 = 100_000_000; | ||
| let platform_fee = (0.3 * 10i128.pow(18) as f64) as i128; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid floating-point arithmetic for fee calculations
Using floating-point arithmetic for financial calculations can lead to precision issues. Consider using integer arithmetic instead.
- let platform_fee = (0.3 * 10i128.pow(18) as f64) as i128;
+ let platform_fee = 3 * 10i128.pow(17); // 0.3 * 10^18
📝 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.
| let platform_fee = (0.3 * 10i128.pow(18) as f64) as i128; | |
| let platform_fee = 3 * 10i128.pow(17); // 0.3 * 10^18 |
🤖 Prompt for AI Agents
In contracts/Contract-Events-for-Indexer/src/tests/test.rs at line 160, the
calculation of platform_fee uses floating-point arithmetic which can cause
precision errors in financial calculations. Replace the floating-point operation
with integer arithmetic by directly computing the fee as an integer, for example
by multiplying the base unit (10^18) by the fee numerator and dividing by the
denominator, to maintain precision without using floats.
| let release_signer_address = Address::generate(&env); | ||
| let dispute_resolver_address = Address::generate(&env); | ||
| let amount: i128 = 100_000_000; | ||
| let platform_fee = (0.3 * 10i128.pow(18) as f64) as i128; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid floating-point arithmetic for fee calculations
Using floating-point arithmetic for financial calculations can lead to precision issues. Consider using integer arithmetic instead.
- let platform_fee = (0.3 * 10i128.pow(18) as f64) as i128;
+ let platform_fee = 3 * 10i128.pow(17); // 0.3 * 10^18
📝 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.
| let platform_fee = (0.3 * 10i128.pow(18) as f64) as i128; | |
| let platform_fee = 3 * 10i128.pow(17); // 0.3 * 10^18 |
🤖 Prompt for AI Agents
In contracts/Contract-Events-for-Indexer/src/tests/test.rs at line 303, the
calculation of platform_fee uses floating-point arithmetic which can cause
precision errors in financial calculations. Replace the floating-point operation
with integer arithmetic by expressing the fee as a fraction or using integer
multiplication and division to maintain precision without floating-point
numbers.
1 participant
Pull Request | Trustless Work
1. Issue Link
2. Brief Description of the Issue
This PR is focused on implementing comprehensive Soroban events for the Trustless Work Smart Escrow contract to enable better indexing and monitoring capabilities. The implementation adds structured events for all major contract operations including escrow creation, funding, milestone management, dispute handling, and fund releases.
3. Changes Made
4. Evidence After Solution
5. Important Notes
Ongoing local test of event emission
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores