From 1ab952cece8d1a6b8ffc15c2e8a42cc2e299a0dd Mon Sep 17 00:00:00 2001 From: mr-t Date: Wed, 30 Aug 2023 04:22:15 +0200 Subject: [PATCH] fix bug on bounty update (#730) * check Bounty is correct in all tests * fix update bounty with lower amount and owner accidentally send funds * cleanup and test update with sending wrong denom --- .../external/cw-bounties/src/contract.rs | 8 +- contracts/external/cw-bounties/src/tests.rs | 646 +++++++++++++----- 2 files changed, 463 insertions(+), 191 deletions(-) diff --git a/contracts/external/cw-bounties/src/contract.rs b/contracts/external/cw-bounties/src/contract.rs index 503ce66ec..181e65839 100644 --- a/contracts/external/cw-bounties/src/contract.rs +++ b/contracts/external/cw-bounties/src/contract.rs @@ -7,7 +7,7 @@ use cosmwasm_std::{ }; use cw2::set_contract_version; use cw_paginate_storage::paginate_map_values; -use cw_utils::must_pay; +use cw_utils::{may_pay, must_pay}; use crate::{ error::ContractError, @@ -243,7 +243,7 @@ pub fn update( }; // Check if amount is greater or less than original amount - let old_amount = bounty.amount; + let old_amount = bounty.amount.clone(); match new_amount.amount.cmp(&old_amount.amount) { Ordering::Greater => { // If new amount is greater, check funds sent plus @@ -259,7 +259,9 @@ pub fn update( } Ordering::Less => { // If new amount is less, pay out difference to owner - let diff = old_amount.amount - new_amount.amount; + // in case owner accidentally sent funds, send back as well + let funds_send = may_pay(&info, &bounty.amount.denom)?; + let diff = old_amount.amount - new_amount.amount + funds_send; let msg = BankMsg::Send { to_address: info.sender.to_string(), amount: vec![Coin { diff --git a/contracts/external/cw-bounties/src/tests.rs b/contracts/external/cw-bounties/src/tests.rs index 7a12d2503..111ffd523 100644 --- a/contracts/external/cw-bounties/src/tests.rs +++ b/contracts/external/cw-bounties/src/tests.rs @@ -2,15 +2,22 @@ use cosmwasm_std::{coin, Addr, Coin, Empty, StdResult, Uint128}; use cw_multi_test::{App, AppResponse, Contract, ContractWrapper, Executor}; use cw_utils::PaymentError; -use crate::{msg::InstantiateMsg, state::BountyStatus, ContractError}; +use crate::{ + msg::InstantiateMsg, + state::{Bounty, BountyStatus}, + ContractError, +}; pub struct Test { pub app: App, - pub addr: Addr, + pub contract: Addr, pub owner: Addr, pub recipient: Addr, } +const ATOM_DENOM: &str = "uatom"; +const JUNO_DENOM: &str = "ujuno"; + impl Test { pub fn new() -> Self { let owner = Addr::unchecked("owner"); @@ -21,7 +28,7 @@ impl Test { .init_balance( storage, &owner, - vec![coin(10000, "ujuno"), coin(10000, "uatom")], + vec![coin(10000, JUNO_DENOM), coin(10000, ATOM_DENOM)], ) .unwrap(); router @@ -29,12 +36,12 @@ impl Test { .init_balance( storage, &recipient, - vec![coin(10000, "ujuno"), coin(10000, "uatom")], + vec![coin(10000, JUNO_DENOM), coin(10000, ATOM_DENOM)], ) .unwrap(); }); let code_id = app.store_code(bounty_countract()); - let addr = app + let contract = app .instantiate_contract( code_id, owner.clone(), @@ -48,7 +55,7 @@ impl Test { .unwrap(); Self { app, - addr, + contract, owner, recipient, } @@ -66,17 +73,20 @@ impl Test { title, description, }; - let res = - self.app - .execute_contract(self.owner.clone(), self.addr.clone(), &msg, send_funds)?; + let res = self.app.execute_contract( + self.owner.clone(), + self.contract.clone(), + &msg, + send_funds, + )?; Ok(res) } pub fn close(&mut self, id: u64) -> Result { let msg = crate::msg::ExecuteMsg::Close { id }; - let res = self - .app - .execute_contract(self.owner.clone(), self.addr.clone(), &msg, &[])?; + let res = + self.app + .execute_contract(self.owner.clone(), self.contract.clone(), &msg, &[])?; Ok(res) } @@ -94,9 +104,12 @@ impl Test { title, description, }; - let res = - self.app - .execute_contract(self.owner.clone(), self.addr.clone(), &msg, send_funds)?; + let res = self.app.execute_contract( + self.owner.clone(), + self.contract.clone(), + &msg, + send_funds, + )?; Ok(res) } @@ -105,15 +118,15 @@ impl Test { id, recipient: self.recipient.to_string(), }; - let res = self - .app - .execute_contract(self.owner.clone(), self.addr.clone(), &msg, &[])?; + let res = + self.app + .execute_contract(self.owner.clone(), self.contract.clone(), &msg, &[])?; Ok(res) } pub fn query(&self, id: u64) -> StdResult { let msg = crate::msg::QueryMsg::Bounty { id }; - self.app.wrap().query_wasm_smart(&self.addr, &msg) + self.app.wrap().query_wasm_smart(&self.contract, &msg) } } @@ -130,27 +143,42 @@ fn bounty_countract() -> Box> { pub fn test_create_bounty() { let mut test = Test::new(); + let bounty_amount = 100; let balance_before = test .app .wrap() - .query_balance(test.owner.clone(), "ujuno") + .query_balance(test.owner.clone(), JUNO_DENOM) .unwrap(); // create bounty test.create( - coin(100, "ujuno"), + coin(bounty_amount, JUNO_DENOM), "title".to_string(), Some("description".to_string()), - &[coin(100, "ujuno")], + &[coin(bounty_amount, JUNO_DENOM)], ) .unwrap(); + // - assert bounty + let bounty = test.query(1).unwrap(); + assert_eq!( + bounty, + Bounty { + id: 1, + amount: coin(bounty_amount, JUNO_DENOM), + title: "title".to_string(), + description: Some("description".to_string()), + status: BountyStatus::Open, + created_at: test.app.block_info().time.seconds(), + updated_at: None, + } + ); // assert balance let balance_after = test .app .wrap() - .query_balance(test.owner.clone(), "ujuno") + .query_balance(test.owner.clone(), JUNO_DENOM) .unwrap(); assert!( - balance_before.amount.u128() == (balance_after.amount.u128() + 100), + balance_before.amount.u128() == (balance_after.amount.u128() + bounty_amount), "before: {}, after: {}", balance_before.amount.u128(), balance_after.amount.u128() @@ -159,7 +187,7 @@ pub fn test_create_bounty() { // create bounty without sending funds let err: ContractError = test .create( - coin(100, "ujuno"), + coin(bounty_amount, JUNO_DENOM), "title".to_string(), Some("description".to_string()), &[], @@ -172,10 +200,10 @@ pub fn test_create_bounty() { // create bounty with lower amount let err: ContractError = test .create( - coin(100, "ujuno"), + coin(bounty_amount, JUNO_DENOM), "title".to_string(), Some("description".to_string()), - &[coin(50, "ujuno")], + &[coin(bounty_amount / 2, JUNO_DENOM)], ) .unwrap_err() .downcast() @@ -183,18 +211,18 @@ pub fn test_create_bounty() { assert_eq!( err, ContractError::InvalidAmount { - expected: Uint128::new(100), - actual: Uint128::new(50) + expected: Uint128::new(bounty_amount), + actual: Uint128::new(bounty_amount / 2) } ); // create bounty with bigger amount let err: ContractError = test .create( - coin(100, "ujuno"), + coin(bounty_amount, JUNO_DENOM), "title".to_string(), Some("description".to_string()), - &[coin(150, "ujuno")], + &[coin(2 * bounty_amount, JUNO_DENOM)], ) .unwrap_err() .downcast() @@ -202,8 +230,8 @@ pub fn test_create_bounty() { assert_eq!( err, ContractError::InvalidAmount { - expected: Uint128::new(100), - actual: Uint128::new(150) + expected: Uint128::new(bounty_amount), + actual: Uint128::new(2 * bounty_amount) } ); } @@ -211,18 +239,27 @@ pub fn test_create_bounty() { #[test] pub fn test_close_bounty() { let mut test = Test::new(); + let bounty_amount = 100; // create bounty test.create( - coin(100, "ujuno"), + coin(bounty_amount, JUNO_DENOM), "title".to_string(), Some("description".to_string()), - &[coin(100, "ujuno")], + &[coin(bounty_amount, JUNO_DENOM)], ) .unwrap(); // close bounty test.close(1).unwrap(); + // - assert bounty status + let bounty = test.query(1).unwrap(); + assert_eq!( + bounty.status, + BountyStatus::Closed { + closed_at: test.app.block_info().time.seconds(), + } + ); // close bounty again let err: ContractError = test.close(1).unwrap_err().downcast().unwrap(); @@ -231,195 +268,428 @@ pub fn test_close_bounty() { #[test] pub fn test_update_bounty() { - let mut test = Test::new(); - - let initial_juno_balance = test - .app - .wrap() - .query_balance(test.owner.clone(), "ujuno") + let bounty_amount = 100; + // case: update bounty with higher amount + { + let mut test = Test::new(); + let initial_juno_balance = test + .app + .wrap() + .query_balance(test.owner.clone(), JUNO_DENOM) + .unwrap(); + // create bounty + test.create( + coin(bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(bounty_amount, JUNO_DENOM)], + ) .unwrap(); - let initial_atom_balance = test - .app - .wrap() - .query_balance(test.owner.clone(), "uatom") + + test.update( + 1, + coin(2 * bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(bounty_amount, JUNO_DENOM)], + ) .unwrap(); - // create bounty - test.create( - coin(100, "ujuno"), - "title".to_string(), - Some("description".to_string()), - &[coin(100, "ujuno")], - ) - .unwrap(); + // - assert bounty + let bounty = test.query(1).unwrap(); + assert_eq!( + bounty, + Bounty { + id: 1, + amount: coin(2 * bounty_amount, JUNO_DENOM), + title: "title".to_string(), + description: Some("description".to_string()), + status: BountyStatus::Open, + created_at: test.app.block_info().time.seconds(), + updated_at: Some(test.app.block_info().time.seconds()), + } + ); + // assert balance + let balance_after = test + .app + .wrap() + .query_balance(test.owner.clone(), JUNO_DENOM) + .unwrap(); + assert!( + initial_juno_balance.amount.u128() == (balance_after.amount.u128() + 2 * bounty_amount), + "before: {}, after: {}", + initial_juno_balance.amount.u128(), + balance_after.amount.u128() + ); + } - // update bounty - test.update( - 1, - coin(200, "ujuno"), - "title".to_string(), - Some("description".to_string()), - &[coin(100, "ujuno")], - ) - .unwrap(); - // assert balance - let balance_after = test - .app - .wrap() - .query_balance(test.owner.clone(), "ujuno") + // case: update bounty with lower amount + { + let mut test = Test::new(); + let initial_juno_balance = test + .app + .wrap() + .query_balance(test.owner.clone(), JUNO_DENOM) + .unwrap(); + // create bounty + test.create( + coin(bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(bounty_amount, JUNO_DENOM)], + ) .unwrap(); - assert!( - initial_juno_balance.amount.u128() == (balance_after.amount.u128() + 200), - "before: {}, after: {}", - initial_juno_balance.amount.u128(), - balance_after.amount.u128() - ); - // update bounty without sending funds - let err: ContractError = test - .update( + test.update( 1, - coin(300, "ujuno"), + coin(bounty_amount / 2, JUNO_DENOM), "title".to_string(), Some("description".to_string()), - &[], + &[], // no funds needed, since update amount is lower ) - .unwrap_err() - .downcast() .unwrap(); - assert_eq!(err, ContractError::PaymentError(PaymentError::NoFunds {})); + // - assert bounty + let bounty = test.query(1).unwrap(); + assert_eq!( + bounty, + Bounty { + id: 1, + amount: coin(bounty_amount / 2, JUNO_DENOM), + title: "title".to_string(), + description: Some("description".to_string()), + status: BountyStatus::Open, + created_at: test.app.block_info().time.seconds(), + updated_at: Some(test.app.block_info().time.seconds()), + } + ); + // assert balance + let balance_after = test + .app + .wrap() + .query_balance(test.owner.clone(), JUNO_DENOM) + .unwrap(); + assert!( + initial_juno_balance.amount.u128() == (balance_after.amount.u128() + bounty_amount / 2), + "before: {}, after: {}", + initial_juno_balance.amount.u128(), + balance_after.amount.u128() + ); + } - // update bounty with lower amount - let err: ContractError = test - .update( - 1, - coin(300, "ujuno"), + // case: update bounty with lower amount + owner accidentally send funds + { + let mut test = Test::new(); + let initial_juno_balance = test + .app + .wrap() + .query_balance(test.owner.clone(), JUNO_DENOM) + .unwrap(); + // create bounty + test.create( + coin(bounty_amount, JUNO_DENOM), "title".to_string(), Some("description".to_string()), - &[coin(50, "ujuno")], + &[coin(bounty_amount, JUNO_DENOM)], ) - .unwrap_err() - .downcast() .unwrap(); - assert_eq!( - err, - ContractError::InvalidAmount { - expected: Uint128::new(100), - actual: Uint128::new(250) - } - ); - // update bounty with bigger amount - let err: ContractError = test - .update( + test.update( 1, - coin(300, "ujuno"), + coin(bounty_amount / 2, JUNO_DENOM), "title".to_string(), Some("description".to_string()), - &[coin(150, "ujuno")], + &[coin(bounty_amount, JUNO_DENOM)], ) - .unwrap_err() - .downcast() .unwrap(); - assert_eq!( - err, - ContractError::InvalidAmount { - expected: Uint128::new(100), - actual: Uint128::new(350) - } - ); + // - assert bounty + let bounty = test.query(1).unwrap(); + assert_eq!( + bounty, + Bounty { + id: 1, + amount: coin(bounty_amount / 2, JUNO_DENOM), + title: "title".to_string(), + description: Some("description".to_string()), + status: BountyStatus::Open, + created_at: test.app.block_info().time.seconds(), + updated_at: Some(test.app.block_info().time.seconds()), + } + ); + // assert balance + let balance_after = test + .app + .wrap() + .query_balance(test.owner.clone(), JUNO_DENOM) + .unwrap(); + assert!( + initial_juno_balance.amount.u128() == (balance_after.amount.u128() + bounty_amount / 2), + "before: {}, after: {}", + initial_juno_balance.amount.u128(), + balance_after.amount.u128() + ); + } - // update bounty with different denom - test.update( - 1, - coin(200, "uatom"), - "title".to_string(), - Some("description".to_string()), - &[coin(200, "uatom")], - ) - .unwrap(); - // assert juno balance - let juno_balance_after = test - .app - .wrap() - .query_balance(test.owner.clone(), "ujuno") + // case: update bounty sending incorrect funds + { + let mut test = Test::new(); + // create bounty + test.create( + coin(bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(bounty_amount, JUNO_DENOM)], + ) .unwrap(); - assert!( - juno_balance_after.amount == initial_juno_balance.amount, - "before: {}, after: {}", - initial_juno_balance.amount.u128(), - balance_after.amount.u128() - ); - let atom_balance_after = test - .app - .wrap() - .query_balance(test.owner.clone(), "uatom") + let err: ContractError = test + .update( + 1, + coin(3 * bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!(err, ContractError::PaymentError(PaymentError::NoFunds {})); + + // update bounty with lower amount + let err: ContractError = test + .update( + 1, + coin(2 * bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(bounty_amount / 2, JUNO_DENOM)], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!( + err, + ContractError::InvalidAmount { + expected: Uint128::new(bounty_amount), + actual: Uint128::new(bounty_amount + bounty_amount / 2) + } + ); + + // update bounty with bigger amount + let err: ContractError = test + .update( + 1, + coin(2 * bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(2 * bounty_amount, JUNO_DENOM)], // 100 already in bounty, now sending 200 + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!( + err, + ContractError::InvalidAmount { + expected: Uint128::new(bounty_amount), + actual: Uint128::new(3 * bounty_amount) + } + ); + } + + // case: update bounty with different denom + { + let mut test = Test::new(); + let initial_juno_balance = test + .app + .wrap() + .query_balance(test.owner.clone(), JUNO_DENOM) + .unwrap(); + let initial_atom_balance = test + .app + .wrap() + .query_balance(test.owner.clone(), ATOM_DENOM) + .unwrap(); + // create bounty + test.create( + coin(bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(bounty_amount, JUNO_DENOM)], + ) .unwrap(); - assert!( - atom_balance_after.amount.u128() == (initial_atom_balance.amount.u128() - 200), - "before: {}, after: {}", - initial_atom_balance.amount.u128(), - atom_balance_after.amount.u128() - ); - // test closed bounty - test.close(1).unwrap(); - let err: ContractError = test - .update( + test.update( 1, - coin(200, "ujuno"), + coin(200, ATOM_DENOM), "title".to_string(), Some("description".to_string()), - &[coin(200, "ujuno")], + &[coin(200, ATOM_DENOM)], ) - .unwrap_err() - .downcast() .unwrap(); - assert_eq!(err, ContractError::NotOpen {}); + // - assert bounty + let bounty = test.query(1).unwrap(); + assert_eq!( + bounty, + Bounty { + id: 1, + amount: coin(2 * bounty_amount, ATOM_DENOM), + title: "title".to_string(), + description: Some("description".to_string()), + status: BountyStatus::Open, + created_at: test.app.block_info().time.seconds(), + updated_at: Some(test.app.block_info().time.seconds()), + } + ); + // assert juno balance + let juno_balance_after = test + .app + .wrap() + .query_balance(test.owner.clone(), JUNO_DENOM) + .unwrap(); + assert!( + juno_balance_after.amount == initial_juno_balance.amount, + "before: {}, after: {}", + initial_juno_balance.amount.u128(), + juno_balance_after.amount.u128() + ); + // assert atom balance + let atom_balance_after = test + .app + .wrap() + .query_balance(test.owner.clone(), ATOM_DENOM) + .unwrap(); + assert!( + atom_balance_after.amount.u128() == (initial_atom_balance.amount.u128() - 200), + "before: {}, after: {}", + initial_atom_balance.amount.u128(), + atom_balance_after.amount.u128() + ); + } + + // case: update bounty with different denom, but owner sends with incorrect denom + { + let mut test = Test::new(); + // create bounty + test.create( + coin(bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(bounty_amount, JUNO_DENOM)], + ) + .unwrap(); + + let err: ContractError = test + .update( + 1, + coin(200, ATOM_DENOM), // update with atom denom + "title".to_string(), + Some("description".to_string()), + &[coin(200, JUNO_DENOM)], // but send juno denom + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!( + err, + ContractError::PaymentError(PaymentError::MissingDenom(ATOM_DENOM.to_string())) + ); + } + + // case: update on closed bounty + { + let mut test = Test::new(); + // create bounty + test.create( + coin(bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(bounty_amount, JUNO_DENOM)], + ) + .unwrap(); + + test.close(1).unwrap(); + let err: ContractError = test + .update( + 1, + coin(2 * bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(2 * bounty_amount, JUNO_DENOM)], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!(err, ContractError::NotOpen {}); + } } #[test] pub fn test_pay_out_bounty() { - let mut test = Test::new(); + let bounty_amount = 100; + // case: payout + { + let mut test = Test::new(); + test.create( + coin(bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(bounty_amount, JUNO_DENOM)], + ) + .unwrap(); - // create bounty - test.create( - coin(100, "ujuno"), - "title".to_string(), - Some("description".to_string()), - &[coin(100, "ujuno")], - ) - .unwrap(); + let initial_juno_balance = test + .app + .wrap() + .query_balance(test.recipient.clone(), JUNO_DENOM) + .unwrap(); - let initial_juno_balance = test - .app - .wrap() - .query_balance(test.recipient.clone(), "ujuno") - .unwrap(); - // pay out bounty - test.pay_out(1).unwrap(); - // assert balance - let balance_after = test - .app - .wrap() - .query_balance(test.recipient.clone(), "ujuno") + test.pay_out(1).unwrap(); + // assert balance + let balance_after = test + .app + .wrap() + .query_balance(test.recipient.clone(), JUNO_DENOM) + .unwrap(); + assert!( + initial_juno_balance.amount.u128() + bounty_amount == (balance_after.amount.u128()), + "before: {}, after: {}", + initial_juno_balance.amount.u128(), + balance_after.amount.u128() + ); + // assert bounty + let bounty = test.query(1).unwrap(); + assert_eq!( + bounty, + Bounty { + id: 1, + amount: coin(bounty_amount, JUNO_DENOM), + title: "title".to_string(), + description: Some("description".to_string()), + status: BountyStatus::Claimed { + claimed_by: test.recipient.to_string(), + claimed_at: test.app.block_info().time.seconds() + }, + created_at: test.app.block_info().time.seconds(), + updated_at: None, + } + ); + + // - test bounty already claimed + let err: ContractError = test.pay_out(1).unwrap_err().downcast().unwrap(); + assert_eq!(err, ContractError::NotOpen {}); + } + + // case: payout of closed bounty, this covered above, but just to be sure and test on manual close + { + let mut test = Test::new(); + test.create( + coin(bounty_amount, JUNO_DENOM), + "title".to_string(), + Some("description".to_string()), + &[coin(bounty_amount, JUNO_DENOM)], + ) .unwrap(); - assert!( - initial_juno_balance.amount.u128() + 100 == (balance_after.amount.u128()), - "before: {}, after: {}", - initial_juno_balance.amount.u128(), - balance_after.amount.u128() - ); - // assert bounty claimed - let bounty = test.query(1).unwrap(); - assert_eq!( - bounty.status, - BountyStatus::Claimed { - claimed_by: test.recipient.to_string(), - claimed_at: test.app.block_info().time.seconds() - } - ); + test.close(1).unwrap(); - // test bounty already claimed - let err: ContractError = test.pay_out(1).unwrap_err().downcast().unwrap(); - assert_eq!(err, ContractError::NotOpen {}); + // - test bounty already claimed + let err: ContractError = test.pay_out(1).unwrap_err().downcast().unwrap(); + assert_eq!(err, ContractError::NotOpen {}); + } }