feat: M009 CosmWasm service-escrow contract with unit tests#77
feat: M009 CosmWasm service-escrow contract with unit tests#77brawlaphant wants to merge 1 commit intoregen-network:mainfrom
Conversation
Implements the M009 Service Provision Escrow mechanism as a CosmWasm smart contract with milestone-based payment release, dispute resolution, and governance-tunable parameters per the SPEC. Key features: - Service agreement lifecycle: Proposed -> Funded -> InProgress -> Completed - Sequential milestone management with deliverable IRI tracking - Provider bond locking (configurable 5-25% of escrow) - Platform fees on milestone approval + completion (configurable 0-5%) - Cancellation with fee from Funded state (configurable 0-10%) - Three-way dispute resolution: ClientWins, ProviderWins, Split - Arbiter DAO gating on dispute resolution - Milestone revision tracking with configurable max revisions - Paginated query endpoints for agreements, milestones, escrow balance, disputes - Admin-gated config updates with parameter bounds validation 37 unit tests covering: - Happy path (3 milestones through completion) - All three dispute resolution outcomes - Cancellation from Proposed and Funded states - Authorization guards on all execute messages - Milestone ordering enforcement - Revision limits - Escrow balance tracking after partial approvals - Pagination and status filtering - Parameter validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a milestone-based service escrow contract for the Regen Network ecosystem, facilitating secure payments and dispute resolution. The implementation covers the full agreement lifecycle, including proposal, funding, milestone submission, and arbiter-mediated resolution. Key feedback identifies critical logic errors in fee distribution that could lead to insufficient contract balances and the premature release of provider bonds during disputes. Further improvements are suggested to enforce exact payments to prevent locked funds, handle rounding issues during bond splits, and maintain state consistency for released funds and fees.
| let milestone_payment = agreement.milestones[milestone_index as usize].payment; | ||
| let platform_fee = milestone_payment.multiply_ratio(config.platform_fee_rate_bps, 10_000u128); | ||
| let provider_payment = milestone_payment - platform_fee; | ||
|
|
||
| agreement.milestones[milestone_index as usize].status = MilestoneStatus::Approved; | ||
| agreement.milestones[milestone_index as usize].approved_at = Some(env.block.time); | ||
| agreement.total_released += provider_payment; | ||
| agreement.total_fees += platform_fee; | ||
|
|
||
| let mut msgs = vec![]; | ||
|
|
||
| if !provider_payment.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.provider.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: provider_payment }], | ||
| }); | ||
| } | ||
| if !platform_fee.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: config.community_pool.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: platform_fee }], | ||
| }); | ||
| } | ||
|
|
||
| let next_idx = milestone_index + 1; | ||
| if next_idx >= agreement.milestones.len() as u32 { | ||
| agreement.status = AgreementStatus::Completed; | ||
| agreement.completed_at = Some(env.block.time); | ||
|
|
||
| if !agreement.provider_bond.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.provider.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: agreement.provider_bond }], | ||
| }); | ||
| } | ||
| let completion_fee = agreement.escrow_amount.multiply_ratio(config.platform_fee_rate_bps, 10_000u128); | ||
| if !completion_fee.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: config.community_pool.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: completion_fee }], | ||
| }); | ||
| agreement.total_fees += completion_fee; | ||
| } |
There was a problem hiding this comment.
There is a critical logic error in the fee distribution for the final milestone. The contract calculates a completion_fee (1% of total escrow) in addition to the standard platform_fee (1% of milestone payment). However, the client only funds the escrow_amount (sum of milestone payments). If all milestone payments are distributed, the contract will have a zero balance for the escrow, causing the BankMsg::Send for the completion_fee to fail due to insufficient funds. The completion fee should be deducted from the final milestone's provider payment to ensure the total outflow does not exceed the funded escrow.
let milestone_payment = agreement.milestones[milestone_index as usize].payment;
let platform_fee = milestone_payment.multiply_ratio(config.platform_fee_rate_bps, 10_000u128);
let mut provider_payment = milestone_payment - platform_fee;
agreement.milestones[milestone_index as usize].status = MilestoneStatus::Approved;
agreement.milestones[milestone_index as usize].approved_at = Some(env.block.time);
let mut msgs = vec![];
let next_idx = milestone_index + 1;
let is_last = next_idx >= agreement.milestones.len() as u32;
if is_last {
agreement.status = AgreementStatus::Completed;
agreement.completed_at = Some(env.block.time);
let completion_fee = agreement.escrow_amount.multiply_ratio(config.platform_fee_rate_bps, 10_000u128);
provider_payment = provider_payment.saturating_sub(completion_fee);
if !completion_fee.is_zero() {
msgs.push(BankMsg::Send {
to_address: config.community_pool.to_string(),
amount: vec![Coin { denom: config.denom.clone(), amount: completion_fee }],
});
agreement.total_fees += completion_fee;
}
if !agreement.provider_bond.is_zero() {
msgs.push(BankMsg::Send {
to_address: agreement.provider.to_string(),
amount: vec![Coin { denom: config.denom.clone(), amount: agreement.provider_bond }],
});
}
} else {
agreement.current_milestone = next_idx;
agreement.milestones[next_idx as usize].status = MilestoneStatus::InProgress;
agreement.status = AgreementStatus::InProgress;
}
agreement.total_released += provider_payment;
agreement.total_fees += platform_fee;
if !provider_payment.is_zero() {
msgs.push(BankMsg::Send {
to_address: agreement.provider.to_string(),
amount: vec![Coin { denom: config.denom.clone(), amount: provider_payment }],
});
}
if !platform_fee.is_zero() {
msgs.push(BankMsg::Send {
to_address: config.community_pool.to_string(),
amount: vec![Coin { denom: config.denom.clone(), amount: platform_fee }],
});
}| DisputeResolution::ProviderWins => { | ||
| let provider_receives = disputed_amount + agreement.provider_bond - arbiter_fee; | ||
| if !provider_receives.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.provider.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: provider_receives }], | ||
| }); | ||
| } | ||
| if !arbiter_fee.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: config.community_pool.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: arbiter_fee }], | ||
| }); | ||
| } | ||
| let next_idx = dispute.milestone_index + 1; | ||
| if next_idx < agreement.milestones.len() as u32 { | ||
| agreement.current_milestone = next_idx; | ||
| agreement.milestones[next_idx as usize].status = MilestoneStatus::InProgress; | ||
| agreement.status = AgreementStatus::InProgress; | ||
| agreement.provider_bond = Uint128::zero(); | ||
| } else { | ||
| agreement.status = AgreementStatus::Completed; | ||
| agreement.completed_at = Some(env.block.time); | ||
| } |
There was a problem hiding this comment.
The ProviderWins resolution logic releases the entire provider_bond even if the agreement is set to continue to the next milestone. This violates Security Invariant 2 in the specification, which states the bond must be locked for the entire agreement lifetime. Releasing the bond early leaves the client unprotected for subsequent milestones. The bond should only be released if next_idx >= agreement.milestones.len().
DisputeResolution::ProviderWins => {
let provider_receives = disputed_amount.saturating_sub(arbiter_fee);
if !provider_receives.is_zero() {
msgs.push(BankMsg::Send {
to_address: agreement.provider.to_string(),
amount: vec![Coin { denom: config.denom.clone(), amount: provider_receives }],
});
}
if !arbiter_fee.is_zero() {
msgs.push(BankMsg::Send {
to_address: config.community_pool.to_string(),
amount: vec![Coin { denom: config.denom.clone(), amount: arbiter_fee }],
});
}
let next_idx = dispute.milestone_index + 1;
if next_idx < agreement.milestones.len() as u32 {
agreement.current_milestone = next_idx;
agreement.milestones[next_idx as usize].status = MilestoneStatus::InProgress;
agreement.status = AgreementStatus::InProgress;
} else {
if !agreement.provider_bond.is_zero() {
msgs.push(BankMsg::Send {
to_address: agreement.provider.to_string(),
amount: vec![Coin { denom: config.denom.clone(), amount: agreement.provider_bond }],
});
agreement.provider_bond = Uint128::zero();
}
agreement.status = AgreementStatus::Completed;
agreement.completed_at = Some(env.block.time);
}
}| let bond_coin = must_pay(&info, &config.denom)?; | ||
| if bond_coin < agreement.provider_bond { | ||
| return Err(ContractError::InsufficientFunds { | ||
| required: agreement.provider_bond.to_string(), sent: bond_coin.to_string(), | ||
| }); |
There was a problem hiding this comment.
The contract does not enforce exact payment of the provider bond. If a provider sends more than the required agreement.provider_bond, the excess funds will be locked in the contract but not recorded in the agreement state, making them unrecoverable through the standard refund/release mechanisms. This also applies to execute_fund for the client's escrow payment.
| let bond_coin = must_pay(&info, &config.denom)?; | |
| if bond_coin < agreement.provider_bond { | |
| return Err(ContractError::InsufficientFunds { | |
| required: agreement.provider_bond.to_string(), sent: bond_coin.to_string(), | |
| }); | |
| let bond_coin = must_pay(&info, &config.denom)?; | |
| if bond_coin != agreement.provider_bond { | |
| return Err(ContractError::InsufficientFunds { | |
| required: agreement.provider_bond.to_string(), sent: bond_coin.to_string(), | |
| }); | |
| } |
| let bond_half = agreement.provider_bond.multiply_ratio(1u128, 2u128); | ||
| if !bond_half.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.client.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: bond_half }], | ||
| }); | ||
| msgs.push(BankMsg::Send { | ||
| to_address: config.community_pool.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: bond_half }], | ||
| }); |
There was a problem hiding this comment.
When splitting the provider bond (e.g., in ClientWins), using multiply_ratio(1, 2) for both halves can leave 1 token stuck in the contract due to rounding down of odd amounts. It is safer to calculate the first half and then subtract it from the total to get the second half.
let bond_half = agreement.provider_bond.multiply_ratio(1u128, 2u128);
let bond_remainder = agreement.provider_bond.saturating_sub(bond_half);
if !bond_half.is_zero() {
msgs.push(BankMsg::Send {
to_address: agreement.client.to_string(),
amount: vec![Coin { denom: config.denom.clone(), amount: bond_half }],
});
}
if !bond_remainder.is_zero() {
msgs.push(BankMsg::Send {
to_address: config.community_pool.to_string(),
amount: vec![Coin { denom: config.denom.clone(), amount: bond_remainder }],
});
}| DisputeResolution::ClientWins => { | ||
| let client_receives = disputed_amount - arbiter_fee; | ||
| if !client_receives.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.client.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: client_receives }], | ||
| }); | ||
| } | ||
| let bond_half = agreement.provider_bond.multiply_ratio(1u128, 2u128); | ||
| if !bond_half.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.client.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: bond_half }], | ||
| }); | ||
| msgs.push(BankMsg::Send { | ||
| to_address: config.community_pool.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: bond_half }], | ||
| }); | ||
| } | ||
| if !arbiter_fee.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: config.community_pool.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: arbiter_fee }], | ||
| }); | ||
| } | ||
| let remaining = remaining_escrow(&agreement); | ||
| if !remaining.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.client.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: remaining }], | ||
| }); | ||
| } | ||
| agreement.status = AgreementStatus::Completed; | ||
| agreement.completed_at = Some(env.block.time); | ||
| } | ||
| DisputeResolution::ProviderWins => { | ||
| let provider_receives = disputed_amount + agreement.provider_bond - arbiter_fee; | ||
| if !provider_receives.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.provider.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: provider_receives }], | ||
| }); | ||
| } | ||
| if !arbiter_fee.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: config.community_pool.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: arbiter_fee }], | ||
| }); | ||
| } | ||
| let next_idx = dispute.milestone_index + 1; | ||
| if next_idx < agreement.milestones.len() as u32 { | ||
| agreement.current_milestone = next_idx; | ||
| agreement.milestones[next_idx as usize].status = MilestoneStatus::InProgress; | ||
| agreement.status = AgreementStatus::InProgress; | ||
| agreement.provider_bond = Uint128::zero(); | ||
| } else { | ||
| agreement.status = AgreementStatus::Completed; | ||
| agreement.completed_at = Some(env.block.time); | ||
| } | ||
| } | ||
| DisputeResolution::Split { client_percent } => { | ||
| let client_share = disputed_amount.multiply_ratio(*client_percent as u128, 100u128); | ||
| let provider_share = disputed_amount - client_share; | ||
| let arbiter_fee_half = arbiter_fee.multiply_ratio(1u128, 2u128); | ||
|
|
||
| let client_receives = client_share.saturating_sub(arbiter_fee_half); | ||
| if !client_receives.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.client.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: client_receives }], | ||
| }); | ||
| } | ||
| let provider_receives = (provider_share + agreement.provider_bond).saturating_sub(arbiter_fee_half); | ||
| if !provider_receives.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.provider.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: provider_receives }], | ||
| }); | ||
| } | ||
| if !arbiter_fee.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: config.community_pool.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: arbiter_fee }], | ||
| }); | ||
| } | ||
| let remaining = remaining_escrow(&agreement); | ||
| if !remaining.is_zero() { | ||
| msgs.push(BankMsg::Send { | ||
| to_address: agreement.client.to_string(), | ||
| amount: vec![Coin { denom: config.denom.clone(), amount: remaining }], | ||
| }); | ||
| } | ||
| agreement.status = AgreementStatus::Completed; | ||
| agreement.completed_at = Some(env.block.time); | ||
| } |
There was a problem hiding this comment.
The execute_resolve_dispute function fails to update agreement.total_released and agreement.total_fees. These fields are critical for the EscrowBalance query to correctly calculate the remaining_escrow. Every time funds are sent to the provider or the community pool (including arbiter fees and bond slashes), these counters must be incremented accordingly.
Summary
contracts/service-escrow/Contract structure
Cargo.tomlsrc/lib.rssrc/error.rsContractErrorenum with 16 error variantssrc/state.rsConfig,ServiceAgreement,Milestone,Disputestructs + storage keyssrc/msg.rsInstantiateMsg,ExecuteMsg(11 variants),QueryMsg(8 variants) + responsessrc/contract.rsSpec alignment
Proposed -> Funded -> InProgress -> MilestoneReview -> Completed(+Disputed,Cancelled)Test plan
cargo buildcompiles cleanly (no warnings)cargo test— 37/37 tests passmechanisms/m009-service-escrow/SPEC.mdacceptance testsphase-3/3.1-smart-contract-specs.mdcontract state/message specs🤖 Generated with Claude Code