From c08ed8e887b37c69f7d3b935112d9dc546214024 Mon Sep 17 00:00:00 2001 From: Hussein Ait Lahcen Date: Tue, 4 Jan 2022 13:29:14 +0100 Subject: [PATCH 1/7] refund remaining reward on offer cancellation --- frame/bonded-finance/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/frame/bonded-finance/src/lib.rs b/frame/bonded-finance/src/lib.rs index abd5e9be8b4..837a90f9d91 100644 --- a/frame/bonded-finance/src/lib.rs +++ b/frame/bonded-finance/src/lib.rs @@ -238,6 +238,13 @@ pub mod pallet { }; let offer_account = Self::account_id(offer_id); T::NativeCurrency::transfer(&offer_account, &issuer, T::Stake::get(), true)?; + T::Currency::transfer( + offer.reward.asset, + &offer_account, + &issuer, + offer.reward.amount, + true, + )?; BondOffers::::remove(offer_id); Self::deposit_event(Event::::OfferCancelled { offer_id }); Ok(()) From 08d948764bb39ad4bbe5b0964ef496a574662f9a Mon Sep 17 00:00:00 2001 From: Hussein Ait Lahcen Date: Tue, 4 Jan 2022 13:30:30 +0100 Subject: [PATCH 2/7] remove unused errors and non required check --- frame/bonded-finance/src/lib.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/frame/bonded-finance/src/lib.rs b/frame/bonded-finance/src/lib.rs index 837a90f9d91..0063d03ce2e 100644 --- a/frame/bonded-finance/src/lib.rs +++ b/frame/bonded-finance/src/lib.rs @@ -101,10 +101,6 @@ pub mod pallet { pub enum Error { /// The offer could not be found. BondOfferNotFound, - /// Not enough native currency to create a new offer. - NotEnoughStake, - /// Not enough asset to bond. - NotEnoughAsset, /// Someone tried to submit an invalid offer. InvalidBondOffer, /// Someone tried to bond an already completed offer. @@ -310,12 +306,6 @@ pub mod pallet { // NOTE(hussein-aitlahcen): can't overflow, subsumed by `offer.valid()` in // `do_offer` let value = nb_of_bonds * offer.bond_price; - ensure!( - T::Currency::can_withdraw(offer.asset, from, value) - .into_result() - .is_ok(), - Error::::NotEnoughAsset - ); let reward_share = T::Convert::convert( multiply_by_rational( T::Convert::convert(nb_of_bonds), From bcb83eec9644798f3bbc55cd0bf76a53a15fc4f6 Mon Sep 17 00:00:00 2001 From: Hussein Ait Lahcen Date: Tue, 4 Jan 2022 13:31:23 +0100 Subject: [PATCH 3/7] introduce bond offer beneficiary --- frame/bonded-finance/src/lib.rs | 9 +++++---- frame/bonded-finance/src/tests.rs | 16 +++++++++++++++- frame/composable-traits/src/bonded_finance.rs | 10 ++++++---- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/frame/bonded-finance/src/lib.rs b/frame/bonded-finance/src/lib.rs index 0063d03ce2e..fec368894a7 100644 --- a/frame/bonded-finance/src/lib.rs +++ b/frame/bonded-finance/src/lib.rs @@ -82,7 +82,8 @@ pub mod pallet { <::Currency as FungiblesInspect>>::Balance; pub(crate) type NativeBalanceOf = <::NativeCurrency as FungibleInspect>>::Balance; - pub(crate) type BondOfferOf = BondOffer, BalanceOf, BlockNumberOf>; + pub(crate) type BondOfferOf = + BondOffer, AssetIdOf, BalanceOf, BlockNumberOf>; #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] @@ -221,7 +222,7 @@ pub mod pallet { /// Emits a `OfferCancelled`. #[pallet::weight(10_000)] pub fn cancel(origin: OriginFor, offer_id: T::BondOfferId) -> DispatchResult { - let (issuer, _) = Self::get_offer(offer_id)?; + let (issuer, offer) = Self::get_offer(offer_id)?; match (ensure_signed(origin.clone()), T::AdminOrigin::ensure_origin(origin)) { // Continue on admin origin (_, Ok(_)) => {}, @@ -315,7 +316,7 @@ pub mod pallet { .map_err(|_| ArithmeticError::Overflow)?, ); let offer_account = Self::account_id(offer_id); - T::Currency::transfer(offer.asset, from, &offer_account, value, true)?; + T::Currency::transfer(offer.asset, from, &offer.beneficiary, value, true)?; let current_block = frame_system::Pallet::::current_block_number(); T::Vesting::vested_transfer( offer.reward.asset, @@ -332,7 +333,7 @@ pub mod pallet { BondDuration::Finite { return_in } => { T::Vesting::vested_transfer( offer.asset, - &offer_account, + &offer.beneficiary, from, VestingSchedule { start: current_block, diff --git a/frame/bonded-finance/src/tests.rs b/frame/bonded-finance/src/tests.rs index fa8c9c4ace3..a59e7771155 100644 --- a/frame/bonded-finance/src/tests.rs +++ b/frame/bonded-finance/src/tests.rs @@ -54,6 +54,7 @@ macro_rules! prop_assert_ok { #[test] fn valid_offer() { assert!(BondOffer { + beneficiary: ALICE, asset: MockCurrencyId::BTC, bond_price: MIN_VESTED_TRANSFER as _, nb_of_bonds: 100_000_u128, @@ -66,6 +67,7 @@ fn valid_offer() { } .valid(MinVestedTransfer::get() as _, MinReward::get())); assert!(BondOffer { + beneficiary: ALICE, asset: MockCurrencyId::BTC, bond_price: MIN_VESTED_TRANSFER as _, nb_of_bonds: 1_u128, @@ -78,6 +80,7 @@ fn valid_offer() { } .valid(MinVestedTransfer::get() as _, MinReward::get())); assert!(BondOffer { + beneficiary: ALICE, asset: MockCurrencyId::BTC, bond_price: 1_000_000 + MIN_VESTED_TRANSFER as u128, nb_of_bonds: 100_000_u128, @@ -95,6 +98,7 @@ fn valid_offer() { fn invalid_offer() { // invalid bond_price assert!(!BondOffer { + beneficiary: ALICE, asset: MockCurrencyId::BTC, bond_price: MIN_VESTED_TRANSFER as u128 - 1, nb_of_bonds: 100_000_u128, @@ -109,6 +113,7 @@ fn invalid_offer() { // invalid nb_of_bonds assert!(!BondOffer { + beneficiary: ALICE, asset: MockCurrencyId::BTC, bond_price: MIN_VESTED_TRANSFER as _, nb_of_bonds: 0, @@ -123,6 +128,7 @@ fn invalid_offer() { // invalid maturity assert!(!BondOffer { + beneficiary: ALICE, asset: MockCurrencyId::BTC, bond_price: 1_000_000 + MIN_VESTED_TRANSFER as u128, nb_of_bonds: 100_000_u128, @@ -137,6 +143,7 @@ fn invalid_offer() { // invalid reward assert!(!BondOffer { + beneficiary: ALICE, asset: MockCurrencyId::BTC, bond_price: 1_000_000 + MIN_VESTED_TRANSFER as u128, nb_of_bonds: 100_000_u128, @@ -147,6 +154,7 @@ fn invalid_offer() { // invalid reward: < MinVested assert!(!BondOffer { + beneficiary: ALICE, asset: MockCurrencyId::BTC, bond_price: 1_000_000 + MIN_VESTED_TRANSFER as u128, nb_of_bonds: 100_000_u128, @@ -161,6 +169,7 @@ fn invalid_offer() { // invalid reward maturity assert!(!BondOffer { + beneficiary: ALICE, asset: MockCurrencyId::BTC, bond_price: 1_000_000 + MIN_VESTED_TRANSFER as u128, nb_of_bonds: 100_000_u128, @@ -189,8 +198,9 @@ prop_compose! { reward_amount in MIN_REWARD..Balance::MAX / 2, reward_maturity in 1..BlockNumber::MAX / 2 ) - -> BondOffer { + -> BondOffer { BondOffer { + beneficiary: ALICE, asset: MockCurrencyId::BTC, bond_price, nb_of_bonds, @@ -442,6 +452,8 @@ proptest! { System::assert_last_event(Event::BondedFinance(crate::Event::OfferCancelled { offer_id })); + prop_assert_eq!(Tokens::balance(offer.reward.asset, &ALICE), offer.reward.amount); + Ok(()) })?; } @@ -472,6 +484,8 @@ proptest! { System::assert_last_event(Event::BondedFinance(crate::Event::OfferCancelled { offer_id })); + prop_assert_eq!(Tokens::balance(offer.reward.asset, &ALICE), offer.reward.amount); + Ok(()) })?; } diff --git a/frame/composable-traits/src/bonded_finance.rs b/frame/composable-traits/src/bonded_finance.rs index 6cbd1dec57d..9e7ccbc8510 100644 --- a/frame/composable-traits/src/bonded_finance.rs +++ b/frame/composable-traits/src/bonded_finance.rs @@ -14,7 +14,7 @@ pub trait BondedFinance { /// Create a new offer. fn offer( from: &Self::AccountId, - offer: BondOffer, + offer: BondOffer, ) -> Result; /// Bond for an offer. @@ -36,7 +36,9 @@ pub enum BondDuration { /// The Bond offer. #[derive(Clone, Encode, Decode, PartialEq, Eq, RuntimeDebug, TypeInfo)] -pub struct BondOffer { +pub struct BondOffer { + /// The account that will receive the locked assets. + pub beneficiary: AccountId, /// Asset to be locked. Unlockable after `duration`. pub asset: AssetId, /// Price of a bond. @@ -60,8 +62,8 @@ pub struct BondOfferReward { pub maturity: BlockNumber, } -impl - BondOffer +impl + BondOffer { /// An offer is completed once all it's nb_of_bonds has been sold. pub fn completed(&self) -> bool { From 497565d6d39d049b78a1c6721bfb1aaae3272fb4 Mon Sep 17 00:00:00 2001 From: Hussein Ait Lahcen Date: Tue, 4 Jan 2022 13:45:35 +0100 Subject: [PATCH 4/7] introduce refund-on-cancel tests and refactor to use `composable-tests-helpers` --- Cargo.lock | 1 + frame/bonded-finance/Cargo.toml | 1 + frame/bonded-finance/src/tests.rs | 109 ++++++++++++++++-------------- 3 files changed, 62 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5bfbbe12b36..0552354979f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6446,6 +6446,7 @@ dependencies = [ name = "pallet-bonded-finance" version = "0.0.1" dependencies = [ + "composable-tests-helpers", "composable-traits", "frame-benchmarking", "frame-support", diff --git a/frame/bonded-finance/Cargo.toml b/frame/bonded-finance/Cargo.toml index 45a9dabf27d..5b38f6e4bc7 100644 --- a/frame/bonded-finance/Cargo.toml +++ b/frame/bonded-finance/Cargo.toml @@ -36,6 +36,7 @@ proptest-derive = "0.3" serde = { version = "1.0.124" } orml-tokens = { git = "https://github.com/open-web3-stack/open-runtime-module-library", rev = "17a791edf431d7d7aee1ea3dfaeeb7bc21944301" } pallet-vesting = { path = "../../frame/vesting" } +composable-tests-helpers = { path = "../composable-tests-helpers", default-features = false } [features] default = ["std"] diff --git a/frame/bonded-finance/src/tests.rs b/frame/bonded-finance/src/tests.rs index a59e7771155..49fd5a43c03 100644 --- a/frame/bonded-finance/src/tests.rs +++ b/frame/bonded-finance/src/tests.rs @@ -4,6 +4,7 @@ use super::*; use crate::utils::MIN_VESTED_TRANSFER; +use composable_tests_helpers::{prop_assert_acceptable_computation_error, prop_assert_ok}; use composable_traits::bonded_finance::{BondDuration, BondOffer, BondOfferReward}; use frame_support::{ error::BadOrigin, @@ -14,42 +15,6 @@ use frame_support::{ }; use mock::{Event, *}; use proptest::prelude::*; -use sp_runtime::helpers_128bit::multiply_by_rational; - -macro_rules! prop_assert_epsilon { - ($x:expr, $y:expr) => {{ - let precision = 100; - let epsilon = 1; - let upper = precision + epsilon; - let lower = precision - epsilon; - let q = multiply_by_rational($x, precision, $y).expect("qed;"); - prop_assert!( - upper >= q && q >= lower, - "({}) => {} >= {} * {} / {} >= {}", - q, - upper, - $x, - precision, - $y, - lower - ); - }}; -} - -macro_rules! prop_assert_ok { - ($cond:expr) => { - prop_assert_ok!($cond, concat!("assertion failed: ", stringify!($cond))) - }; - - ($cond:expr, $($fmt:tt)*) => { - if let Err(e) = $cond { - let message = format!($($fmt)*); - let message = format!("Expected Ok(_), got {:?}, {} at {}:{}", e, message, file!(), line!()); - return ::std::result::Result::Err( - proptest::test_runner::TestCaseError::fail(message)); - } - }; -} #[test] fn valid_offer() { @@ -186,10 +151,10 @@ fn invalid_offer() { prop_compose! { // NOTE(hussein-aitlahcen): we use u32 before casting to avoid overflows /// Pseudo random valid simple offer - fn simple_offer(min_contracts: Balance) + fn simple_offer(min_nb_of_bonds: Balance) ( bond_price in MIN_VESTED_TRANSFER as u128..u32::MAX as Balance, - nb_of_bonds in min_contracts..u32::MAX as Balance, + nb_of_bonds in min_nb_of_bonds..u32::MAX as Balance, maturity in prop_oneof![ Just(BondDuration::Infinite), (1..BlockNumber::MAX / 2).prop_map(|return_in| BondDuration::Finite { return_in }) @@ -258,6 +223,54 @@ proptest! { })?; } + #[test] + fn cancel_refund_reward(offer in simple_offer(2)) { + ExtBuilder::build().execute_with(|| { + prop_assert_ok!(Tokens::mint_into(NATIVE_CURRENCY_ID, &ALICE, Stake::get())); + prop_assert_ok!(Tokens::mint_into(offer.reward.asset, &ALICE, offer.reward.amount)); + + prop_assert_eq!(Tokens::balance(offer.reward.asset, &ALICE), offer.reward.amount); + let offer_id = BondedFinance::do_offer(&ALICE, offer.clone()); + prop_assert_ok!(offer_id); + let offer_id = offer_id.expect("impossible; qed"); + + // Bob bond and take half of the reward + let half_nb_of_bonds = offer.nb_of_bonds / 2; + let half_reward = offer.reward.amount / 2; + prop_assert_ok!(Tokens::mint_into(offer.asset, &BOB, half_nb_of_bonds * offer.bond_price)); + prop_assert_ok!(BondedFinance::do_bond(offer_id, &BOB, half_nb_of_bonds)); + + // Alice cancel the offer + prop_assert_ok!(BondedFinance::cancel(Origin::signed(ALICE), offer_id)); + + // The remaining half is refunded to alice + prop_assert_acceptable_computation_error!(Tokens::balance(offer.reward.asset, &ALICE), half_reward); + + Ok(()) + })?; + } + + #[test] + fn cancel_refund_stake(offer in simple_offer(1)) { + ExtBuilder::build().execute_with(|| { + prop_assert_ok!(Tokens::mint_into(NATIVE_CURRENCY_ID, &ALICE, Stake::get())); + prop_assert_ok!(Tokens::mint_into(offer.reward.asset, &ALICE, offer.reward.amount)); + + prop_assert_eq!(Tokens::balance(offer.reward.asset, &ALICE), offer.reward.amount); + let offer_id = BondedFinance::do_offer(&ALICE, offer.clone()); + prop_assert_ok!(offer_id); + let offer_id = offer_id.expect("impossible; qed"); + + // Alice cancel the offer + prop_assert_ok!(BondedFinance::cancel(Origin::signed(ALICE), offer_id)); + + // The stake is refunded + prop_assert_eq!(Tokens::balance(NATIVE_CURRENCY_ID, &ALICE), Stake::get()); + + Ok(()) + })?; + } + #[test] fn isolated_accounts(offer_a in simple_offer(1), offer_b in simple_offer(1)) { ExtBuilder::build().execute_with(|| { @@ -331,21 +344,21 @@ proptest! { prop_assert_ok!(offer_id); let offer_id = offer_id.expect("impossible; qed"); - let half_contracts = offer.nb_of_bonds / 2; + let half_nb_of_bonds = offer.nb_of_bonds / 2; let half_reward = offer.reward.amount / 2; - prop_assert_ok!(Tokens::mint_into(offer.asset, &BOB, half_contracts * offer.bond_price)); - let bob_reward = BondedFinance::do_bond(offer_id, &BOB, half_contracts); + prop_assert_ok!(Tokens::mint_into(offer.asset, &BOB, half_nb_of_bonds * offer.bond_price)); + let bob_reward = BondedFinance::do_bond(offer_id, &BOB, half_nb_of_bonds); prop_assert_ok!(bob_reward); let bob_reward = bob_reward.expect("impossible; qed;"); - prop_assert_ok!(Tokens::mint_into(offer.asset, &CHARLIE, half_contracts * offer.bond_price)); - let charlie_reward = BondedFinance::do_bond(offer_id, &CHARLIE, half_contracts); + prop_assert_ok!(Tokens::mint_into(offer.asset, &CHARLIE, half_nb_of_bonds * offer.bond_price)); + let charlie_reward = BondedFinance::do_bond(offer_id, &CHARLIE, half_nb_of_bonds); prop_assert_ok!(charlie_reward); let charlie_reward = charlie_reward.expect("impossible; qed;"); - prop_assert_epsilon!(bob_reward, half_reward); - prop_assert_epsilon!(charlie_reward, half_reward); + prop_assert_acceptable_computation_error!(bob_reward, half_reward); + prop_assert_acceptable_computation_error!(charlie_reward, half_reward); prop_assert!(Tokens::can_withdraw(offer.reward.asset, &BOB, bob_reward) == WithdrawConsequence::Frozen); prop_assert!(Tokens::can_withdraw(offer.reward.asset, &CHARLIE, charlie_reward) == WithdrawConsequence::Frozen); @@ -444,6 +457,7 @@ proptest! { prop_assert_ok!(BondedFinance::cancel(Origin::signed(ALICE), offer_id)); prop_assert_eq!(Tokens::balance(NATIVE_CURRENCY_ID, &ALICE), Stake::get()); + prop_assert_eq!(Tokens::balance(offer.reward.asset, &ALICE), offer.reward.amount); prop_assert_eq!( BondedFinance::bond(Origin::signed(BOB), offer_id, offer.nb_of_bonds), @@ -452,8 +466,6 @@ proptest! { System::assert_last_event(Event::BondedFinance(crate::Event::OfferCancelled { offer_id })); - prop_assert_eq!(Tokens::balance(offer.reward.asset, &ALICE), offer.reward.amount); - Ok(()) })?; } @@ -476,6 +488,7 @@ proptest! { prop_assert_ok!(BondedFinance::cancel(Origin::root(), offer_id)); prop_assert_eq!(Tokens::balance(NATIVE_CURRENCY_ID, &ALICE), Stake::get()); + prop_assert_eq!(Tokens::balance(offer.reward.asset, &ALICE), offer.reward.amount); prop_assert_eq!( BondedFinance::bond(Origin::signed(BOB), offer_id, offer.nb_of_bonds), @@ -484,8 +497,6 @@ proptest! { System::assert_last_event(Event::BondedFinance(crate::Event::OfferCancelled { offer_id })); - prop_assert_eq!(Tokens::balance(offer.reward.asset, &ALICE), offer.reward.amount); - Ok(()) })?; } From 4a48e27f975e2498b27616fd9ca2552db82c04c6 Mon Sep 17 00:00:00 2001 From: Hussein Ait Lahcen Date: Tue, 4 Jan 2022 14:06:27 +0100 Subject: [PATCH 5/7] add expected final asset owner tests --- frame/bonded-finance/src/tests.rs | 44 +++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/frame/bonded-finance/src/tests.rs b/frame/bonded-finance/src/tests.rs index 49fd5a43c03..1bdc6d25823 100644 --- a/frame/bonded-finance/src/tests.rs +++ b/frame/bonded-finance/src/tests.rs @@ -271,9 +271,53 @@ proptest! { })?; } + #[test] + fn expected_final_owner(offer in simple_offer(1)) { + ExtBuilder::build().execute_with(|| { + prop_assert_ok!(Tokens::mint_into(NATIVE_CURRENCY_ID, &ALICE, Stake::get())); + prop_assert_ok!(Tokens::mint_into(offer.reward.asset, &ALICE, offer.reward.amount)); + let offer_id = BondedFinance::do_offer(&ALICE, offer.clone()); + prop_assert_ok!(offer_id); + let offer_id = offer_id.expect("impossible; qed"); + + prop_assert_ok!(Tokens::mint_into(offer.asset, &BOB, offer.total_price().expect("impossible; qed;"))); + prop_assert_ok!(BondedFinance::bond(Origin::signed(BOB), offer_id, offer.nb_of_bonds)); + prop_assert_eq!( + BondedFinance::bond(Origin::signed(BOB), offer_id, offer.nb_of_bonds), + Err(Error::::OfferCompleted.into()) + ); + + + match offer.maturity { + BondDuration::Infinite => { + prop_assert_eq!( + Tokens::balance(offer.asset, &offer.beneficiary), + offer.total_price().expect("impossible; qed;") + ); + } + BondDuration::Finite { return_in } => { + prop_assert_eq!( + Tokens::balance(offer.asset, &offer.beneficiary), + 0 + ); + System::set_block_number(return_in); + prop_assert_ok!(Vesting::claim(Origin::signed(BOB), offer.asset)); + prop_assert_eq!( + Tokens::balance(offer.asset, &BOB), + offer.total_price().expect("impossible; qed;") + ); + } + } + + Ok(()) + })?; + } + #[test] fn isolated_accounts(offer_a in simple_offer(1), offer_b in simple_offer(1)) { ExtBuilder::build().execute_with(|| { + System::set_block_number(1); + prop_assert_ok!(Tokens::mint_into(NATIVE_CURRENCY_ID, &ALICE, Stake::get())); prop_assert_ok!(Tokens::mint_into(offer_a.reward.asset, &ALICE, offer_a.reward.amount)); let offer_a_id = BondedFinance::do_offer(&ALICE, offer_a.clone()); From 05f6091eeaac9b56e55116e394164a37bd7bd0a5 Mon Sep 17 00:00:00 2001 From: Hussein Ait Lahcen Date: Tue, 4 Jan 2022 14:20:14 +0100 Subject: [PATCH 6/7] update benchmarking to match new changes --- frame/bonded-finance/src/benchmarks.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/bonded-finance/src/benchmarks.rs b/frame/bonded-finance/src/benchmarks.rs index 108212e73f9..aab86a5ec90 100644 --- a/frame/bonded-finance/src/benchmarks.rs +++ b/frame/bonded-finance/src/benchmarks.rs @@ -34,6 +34,7 @@ where T: Config, { BondOffer { + beneficiary: whitelisted_caller(), asset: bond_asset, bond_price: BalanceOf::::from(MIN_VESTED_TRANSFER), maturity: BondDuration::Finite { return_in: BlockNumberOf::::from(1u32) }, From 9aa97488ba031bbbd6ec0804b4151070a5e8f037 Mon Sep 17 00:00:00 2001 From: Hussein Ait Lahcen Date: Tue, 4 Jan 2022 14:58:11 +0100 Subject: [PATCH 7/7] make sure cancellation is transactional --- frame/bonded-finance/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/bonded-finance/src/lib.rs b/frame/bonded-finance/src/lib.rs index fec368894a7..1be53b80082 100644 --- a/frame/bonded-finance/src/lib.rs +++ b/frame/bonded-finance/src/lib.rs @@ -221,6 +221,7 @@ pub mod pallet { /// /// Emits a `OfferCancelled`. #[pallet::weight(10_000)] + #[transactional] pub fn cancel(origin: OriginFor, offer_id: T::BondOfferId) -> DispatchResult { let (issuer, offer) = Self::get_offer(offer_id)?; match (ensure_signed(origin.clone()), T::AdminOrigin::ensure_origin(origin)) {