From ad860f243232ba22f0ea093c3f2cefb58d753dda Mon Sep 17 00:00:00 2001 From: yrong1997 Date: Wed, 15 Sep 2021 14:25:58 +0800 Subject: [PATCH] Revamp&Fix in salp (#298) * Revamp&Fix in salp * Fix per discussion * Fix with bancor return error instead --- node/primitives/src/traits.rs | 3 +- pallets/salp/src/lib.rs | 126 +++++++++++++++++----------------- pallets/salp/src/tests.rs | 120 ++------------------------------ 3 files changed, 69 insertions(+), 180 deletions(-) diff --git a/node/primitives/src/traits.rs b/node/primitives/src/traits.rs index 7f0faf1e7..2d74a5bfa 100644 --- a/node/primitives/src/traits.rs +++ b/node/primitives/src/traits.rs @@ -21,6 +21,7 @@ #![allow(clippy::unnecessary_cast)] use codec::FullCodec; +use frame_support::{dispatch::DispatchError, sp_runtime::TokenError}; use sp_runtime::{ traits::{AtLeast32BitUnsigned, MaybeSerializeDeserialize}, DispatchResult, @@ -171,6 +172,6 @@ pub trait BancorHandler { impl BancorHandler for () { fn add_token(_currency_id: super::CurrencyId, _amount: Balance) -> DispatchResult { - Ok(()) + DispatchResult::from(DispatchError::Token(TokenError::NoFunds)) } } diff --git a/pallets/salp/src/lib.rs b/pallets/salp/src/lib.rs index b9d951649..0e8fec11d 100644 --- a/pallets/salp/src/lib.rs +++ b/pallets/salp/src/lib.rs @@ -92,6 +92,7 @@ pub mod pallet { use frame_support::{ pallet_prelude::{storage::child, *}, sp_runtime::traits::{AccountIdConversion, CheckedAdd, Hash, Saturating, Zero}, + sp_std::convert::TryInto, storage::ChildTriePrefixIterator, PalletId, }; @@ -102,7 +103,7 @@ pub mod pallet { }; use orml_traits::{currency::TransferAll, MultiCurrency, MultiReservableCurrency, XcmTransfer}; use sp_arithmetic::Percent; - use sp_std::{convert::TryInto, prelude::*}; + use sp_std::prelude::*; use xcm::v0::{prelude::XcmError, MultiLocation}; use xcm_support::*; @@ -221,6 +222,11 @@ pub mod pallet { /// The vsToken/vsBond was be unlocked. [who, fund_index, value] Unlocked(AccountIdOf, ParaId, BalanceOf), AllUnlocked(ParaId), + /// Fund status change + Failed(ParaId), + Success(ParaId), + Retired(ParaId), + End(ParaId), /// Proxy ProxyAdded(AccountIdOf), ProxyRemoved(AccountIdOf), @@ -293,11 +299,6 @@ pub mod pallet { ValueQuery, >; - /// The balance can be refunded to users. - #[pallet::storage] - #[pallet::getter(fn refund_pool)] - pub(super) type RefundPool = StorageValue<_, BalanceOf, ValueQuery>; - /// The balance can be redeemed to users. #[pallet::storage] #[pallet::getter(fn redeem_pool)] @@ -321,6 +322,7 @@ pub mod pallet { let fund_new = FundInfo { status: FundStatus::Success, ..fund }; Funds::::insert(index, Some(fund_new)); + Self::deposit_event(Event::::Success(index)); Ok(()) } @@ -339,6 +341,7 @@ pub mod pallet { let fund_new = FundInfo { status: FundStatus::Failed, ..fund }; Funds::::insert(index, Some(fund_new)); + Self::deposit_event(Event::::Failed(index)); Ok(()) } @@ -359,6 +362,7 @@ pub mod pallet { let fund_new = FundInfo { status: FundStatus::Retired, ..fund }; Funds::::insert(index, Some(fund_new)); + Self::deposit_event(Event::::Retired(index)); Ok(()) } @@ -380,6 +384,7 @@ pub mod pallet { let fund_new = FundInfo { status: FundStatus::End, ..fund }; Funds::::insert(index, Some(fund_new)); + Self::deposit_event(Event::::End(index)); Ok(()) } @@ -581,11 +586,14 @@ pub mod pallet { ensure!(raised <= fund.cap, Error::::CapExceeded); let (contributed, status) = Self::contribution(fund.trie_index, &who); - ensure!(status == ContributionStatus::Idle, Error::::InvalidContributionStatus); + ensure!( + status == ContributionStatus::Idle || + status == ContributionStatus::Refunded || + status == ContributionStatus::Redeemed, + Error::::InvalidContributionStatus + ); - if T::TransactType::get() == ParachainTransactType::Xcm && - T::XcmTransferOrigin::get() == TransferOriginType::FromRelayChain - { + if T::TransactType::get() == ParachainTransactType::Xcm { T::MultiCurrency::reserve(T::RelayChainToken::get(), &who, value)?; } @@ -652,9 +660,7 @@ pub mod pallet { FundInfo { raised: fund.raised.saturating_add(contributing), ..fund }; Funds::::insert(index, Some(fund_new)); - if T::TransactType::get() == ParachainTransactType::Xcm && - T::XcmTransferOrigin::get() == TransferOriginType::FromRelayChain - { + if T::TransactType::get() == ParachainTransactType::Xcm { T::MultiCurrency::unreserve(T::RelayChainToken::get(), &who, contributing); T::MultiCurrency::transfer( T::RelayChainToken::get(), @@ -681,9 +687,7 @@ pub mod pallet { contributed, ContributionStatus::Idle, ); - if T::TransactType::get() == ParachainTransactType::Xcm && - T::XcmTransferOrigin::get() == TransferOriginType::FromRelayChain - { + if T::TransactType::get() == ParachainTransactType::Xcm { T::MultiCurrency::unreserve(T::RelayChainToken::get(), &who, contributing); } Self::deposit_event(Event::ContributeFailed(who, index, contributing, message_id)); @@ -710,13 +714,10 @@ pub mod pallet { let amount_withdrew = fund.raised; if fund.status == FundStatus::Retired { - RedeemPool::::set(Self::redeem_pool().saturating_add(amount_withdrew)); - let fund_new = FundInfo { status: FundStatus::RedeemWithdrew, ..fund }; Funds::::insert(index, Some(fund_new)); + RedeemPool::::set(Self::redeem_pool().saturating_add(amount_withdrew)); } else if fund.status == FundStatus::Failed { - RefundPool::::set(Self::refund_pool().saturating_add(amount_withdrew)); - let fund_new = FundInfo { status: FundStatus::RefundWithdrew, ..fund }; Funds::::insert(index, Some(fund_new)); } @@ -731,14 +732,14 @@ pub mod pallet { pub fn refund(origin: OriginFor, #[pallet::compact] index: ParaId) -> DispatchResult { let who = ensure_signed(origin.clone())?; - let fund = Self::funds(index).ok_or(Error::::InvalidParaId)?; + let mut fund = Self::funds(index).ok_or(Error::::InvalidParaId)?; ensure!(fund.status == FundStatus::RefundWithdrew, Error::::InvalidFundStatus); let (contributed, status) = Self::contribution(fund.trie_index, &who); ensure!(contributed > Zero::zero(), Error::::ZeroContribution); ensure!(status == ContributionStatus::Idle, Error::::InvalidContributionStatus); - ensure!(Self::refund_pool() >= contributed, Error::::NotEnoughBalanceInRefundPool); + ensure!(fund.raised >= contributed, Error::::NotEnoughBalanceInRefundPool); #[allow(non_snake_case)] let (vsToken, vsBond) = Self::vsAssets(index, fund.first_slot, fund.last_slot); @@ -751,16 +752,14 @@ pub mod pallet { Error::::NotEnoughReservedAssetsToRefund ); - RefundPool::::set(Self::refund_pool().saturating_sub(contributed)); + fund.raised = fund.raised.saturating_sub(contributed); let balance = T::MultiCurrency::slash_reserved(vsToken, &who, contributed); ensure!(balance == Zero::zero(), Error::::NotEnoughReservedAssetsToRefund); let balance = T::MultiCurrency::slash_reserved(vsBond, &who, contributed); ensure!(balance == Zero::zero(), Error::::NotEnoughReservedAssetsToRefund); - if T::TransactType::get() == ParachainTransactType::Xcm && - T::XcmTransferOrigin::get() == TransferOriginType::FromRelayChain - { + if T::TransactType::get() == ParachainTransactType::Xcm { T::MultiCurrency::transfer( T::RelayChainToken::get(), &Self::fund_account_id(index), @@ -789,17 +788,18 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin.clone())?; - let fund = Self::funds(index).ok_or(Error::::InvalidParaId)?; + let mut fund = Self::funds(index).ok_or(Error::::InvalidParaId)?; ensure!(fund.status == FundStatus::RedeemWithdrew, Error::::InvalidFundStatus); + ensure!(fund.raised >= value, Error::::NotEnoughBalanceInRedeemPool); ensure!(Self::redeem_pool() >= value, Error::::NotEnoughBalanceInRedeemPool); let cur_block = >::block_number(); ensure!(!Self::is_expired(cur_block, fund.last_slot), Error::::VSBondExpired); - if T::TransactType::get() == ParachainTransactType::Proxy { - ensure!(Self::can_redeem(cur_block, fund.last_slot), Error::::UnRedeemableNow); - } + ensure!(Self::can_redeem(cur_block, fund.last_slot), Error::::UnRedeemableNow); + #[allow(non_snake_case)] let (vsToken, vsBond) = Self::vsAssets(index, fund.first_slot, fund.last_slot); + fund.raised = fund.raised.saturating_sub(value); RedeemPool::::set(Self::redeem_pool().saturating_sub(value)); T::MultiCurrency::ensure_can_withdraw(vsToken, &who, value) @@ -809,9 +809,7 @@ pub mod pallet { T::MultiCurrency::withdraw(vsToken, &who, value)?; T::MultiCurrency::withdraw(vsBond, &who, value)?; - if T::TransactType::get() == ParachainTransactType::Xcm && - T::XcmTransferOrigin::get() == TransferOriginType::FromRelayChain - { + if T::TransactType::get() == ParachainTransactType::Xcm { T::MultiCurrency::transfer( T::RelayChainToken::get(), &Self::fund_account_id(index), @@ -819,7 +817,7 @@ pub mod pallet { value, )?; } - Self::put_contribution(fund.trie_index, &who, value, ContributionStatus::Refunded); + Self::put_contribution(fund.trie_index, &who, value, ContributionStatus::Redeemed); Self::deposit_event(Event::Redeemed( who, index, @@ -932,24 +930,49 @@ pub mod pallet { // Must be ok if let Ok(release_amount) = TryInto::>::try_into(release_amount) { - RedeemPool::::set(Self::redeem_pool().saturating_sub(release_amount)); - // Increase the balance of bancor-pool by release-amount - if let Err(err) = + if let Ok(()) = T::BancorPool::add_token(T::RelayChainToken::get(), release_amount) { - log::warn!("Bancor: {:?} on bifrost-bancor.", err); + RedeemPool::::set( + Self::redeem_pool().saturating_sub(release_amount), + ); } } else { log::warn!("Overflow: The balance of redeem-pool exceeds u128."); } } } - T::BaseXcmWeight::get() + T::DbWeight::get().reads(1) } } impl Pallet { + /// Check if the vsBond is `past` the redeemable date + pub(crate) fn is_expired(block: BlockNumberFor, last_slot: LeasePeriod) -> bool { + let block_begin_redeem = Self::block_end_of_lease_period_index(last_slot); + let block_end_redeem = block_begin_redeem.saturating_add(T::VSBondValidPeriod::get()); + + block >= block_end_redeem + } + + /// Check if the vsBond is `in` the redeemable date + pub(crate) fn can_redeem(block: BlockNumberFor, last_slot: LeasePeriod) -> bool { + let block_begin_redeem = Self::block_end_of_lease_period_index(last_slot); + let block_end_redeem = block_begin_redeem.saturating_add(T::VSBondValidPeriod::get()); + + block >= block_begin_redeem && block < block_end_redeem + } + + #[allow(unused)] + pub(crate) fn block_start_of_lease_period_index(slot: LeasePeriod) -> BlockNumberFor { + slot.saturating_mul(T::LeasePeriod::get()) + } + + pub(crate) fn block_end_of_lease_period_index(slot: LeasePeriod) -> BlockNumberFor { + (slot + 1).saturating_mul(T::LeasePeriod::get()) + } + pub fn fund_account_id(index: ParaId) -> T::AccountId { T::PalletId::get().into_sub_account(index) } @@ -1029,31 +1052,6 @@ pub mod pallet { (vsToken, vsBond) } - /// Check if the vsBond is `past` the redeemable date - pub(crate) fn is_expired(block: BlockNumberFor, last_slot: LeasePeriod) -> bool { - let block_begin_redeem = Self::block_end_of_lease_period_index(last_slot); - let block_end_redeem = block_begin_redeem.saturating_add(T::VSBondValidPeriod::get()); - - block >= block_end_redeem - } - - /// Check if the vsBond is `in` the redeemable date - pub(crate) fn can_redeem(block: BlockNumberFor, last_slot: LeasePeriod) -> bool { - let block_begin_redeem = Self::block_end_of_lease_period_index(last_slot); - let block_end_redeem = block_begin_redeem.saturating_add(T::VSBondValidPeriod::get()); - - block >= block_begin_redeem && block < block_end_redeem - } - - #[allow(unused)] - pub(crate) fn block_start_of_lease_period_index(slot: LeasePeriod) -> BlockNumberFor { - slot.saturating_mul(T::LeasePeriod::get()) - } - - pub(crate) fn block_end_of_lease_period_index(slot: LeasePeriod) -> BlockNumberFor { - (slot + 1).saturating_mul(T::LeasePeriod::get()) - } - fn put_contribution( index: TrieIndex, who: &AccountIdOf, diff --git a/pallets/salp/src/tests.rs b/pallets/salp/src/tests.rs index 40f1d45ef..1b28d519d 100644 --- a/pallets/salp/src/tests.rs +++ b/pallets/salp/src/tests.rs @@ -586,8 +586,6 @@ fn withdraw_should_work() { let fund = Salp::funds(3_000).unwrap(); assert_eq!(fund.status, FundStatus::RedeemWithdrew); - assert_eq!(Salp::redeem_pool(), 100); - assert_ok!(Salp::create(Some(ALICE).into(), 4_000, 1_000, 1, SlotLength::get())); assert_ok!(Salp::contribute(Some(BRUCE).into(), 4_000, 100)); assert_ok!(Salp::confirm_contribute( @@ -602,8 +600,6 @@ fn withdraw_should_work() { let fund = Salp::funds(4_000).unwrap(); assert_eq!(fund.status, FundStatus::RefundWithdrew); - - assert_eq!(Salp::refund_pool(), 100); }); } @@ -626,8 +622,6 @@ fn withdraw_when_xcm_error_should_work() { let fund = Salp::funds(3_000).unwrap(); assert_eq!(fund.status, FundStatus::RedeemWithdrew); - assert_eq!(Salp::redeem_pool(), 100); - assert_ok!(Salp::create(Some(ALICE).into(), 4_000, 1_000, 1, SlotLength::get())); assert_ok!(Salp::contribute(Some(BRUCE).into(), 4_000, 100)); assert_ok!(Salp::confirm_contribute( @@ -642,8 +636,6 @@ fn withdraw_when_xcm_error_should_work() { let fund = Salp::funds(4_000).unwrap(); assert_eq!(fund.status, FundStatus::RefundWithdrew); - - assert_eq!(Salp::refund_pool(), 100); }); } @@ -667,8 +659,6 @@ fn double_withdraw_same_fund_should_fail() { let fund = Salp::funds(3_000).unwrap(); assert_eq!(fund.status, FundStatus::RedeemWithdrew); - assert_eq!(Salp::redeem_pool(), 100); - assert_ok!(Salp::create(Some(ALICE).into(), 4_000, 1_000, 1, SlotLength::get())); assert_ok!(Salp::contribute(Some(BRUCE).into(), 4_000, 100)); assert_ok!(Salp::confirm_contribute( @@ -684,8 +674,6 @@ fn double_withdraw_same_fund_should_fail() { let fund = Salp::funds(4_000).unwrap(); assert_eq!(fund.status, FundStatus::RefundWithdrew); - - assert_eq!(Salp::refund_pool(), 100); }); } @@ -761,8 +749,6 @@ fn refund_should_work() { assert_ok!(Salp::withdraw(Some(ALICE).into(), 3_000)); assert_ok!(Salp::refund(Some(BRUCE).into(), 3_000)); - assert_eq!(Salp::refund_pool(), 0); - let fund = Salp::funds(3_000).unwrap(); let (contributed, status) = Salp::contribution(fund.trie_index, &BRUCE); assert_eq!(contributed, 100); @@ -798,8 +784,6 @@ fn refund_when_xcm_error_should_work() { assert_ok!(Salp::withdraw(Some(ALICE).into(), 3_000)); assert_ok!(Salp::refund(Some(BRUCE).into(), 3_000)); - assert_eq!(Salp::refund_pool(), 0); - let fund = Salp::funds(3_000).unwrap(); let (contributed, status) = Salp::contribution(fund.trie_index, &BRUCE); assert_eq!(contributed, 100); @@ -865,32 +849,6 @@ fn refund_without_enough_vsassets_should_fail() { }); } -/// ABSOLUTELY NOT HAPPEN AT NORMAL PROCESS! -#[test] -fn refund_without_enough_balance_in_pool_should_fail() { - new_test_ext().execute_with(|| { - assert_ok!(Salp::create(Some(ALICE).into(), 3_000, 1_000, 1, SlotLength::get())); - assert_ok!(Salp::contribute(Some(BRUCE).into(), 3_000, 100)); - assert_ok!(Salp::confirm_contribute( - Some(ALICE).into(), - BRUCE, - 3_000, - true, - CONTRIBUTON_INDEX - )); - assert_ok!(Salp::fund_fail(Some(ALICE).into(), 3_000)); - assert_ok!(Salp::withdraw(Some(ALICE).into(), 3_000)); - - // ABSOLUTELY NOT HAPPEN AT NORMAL PROCESS! - crate::pallet::RefundPool::::set(50); - - assert_noop!( - Salp::refund(Some(BRUCE).into(), 3_000), - Error::::NotEnoughBalanceInRefundPool - ); - }); -} - #[test] fn refund_with_zero_contribution_should_fail() { new_test_ext().execute_with(|| { @@ -983,8 +941,6 @@ fn dissolve_should_work() { assert_ok!(Salp::withdraw(Some(ALICE).into(), 3_000)); assert_ok!(Salp::fund_end(Some(ALICE).into(), 3_000)); - assert_eq!(Salp::redeem_pool(), (10 * contribute_account_num) as u128); - for _ in 0..remove_times { assert_ok!(Salp::dissolve(Some(ALICE).into(), 3_000)); } @@ -1080,8 +1036,6 @@ fn redeem_should_work() { assert_ok!(Salp::redeem(Some(BRUCE).into(), 3_000, 50)); - assert_eq!(Salp::redeem_pool(), 50); - assert_eq!(Tokens::accounts(BRUCE, vsToken).free, 0); assert_eq!(Tokens::accounts(BRUCE, vsToken).frozen, 0); assert_eq!(Tokens::accounts(BRUCE, vsToken).reserved, 0); @@ -1094,8 +1048,6 @@ fn redeem_should_work() { assert_ok!(Salp::redeem(Some(CATHI).into(), 3_000, 50)); - assert_eq!(Salp::redeem_pool(), 0); - assert_eq!(Tokens::accounts(CATHI, vsToken).free, 0); assert_eq!(Tokens::accounts(CATHI, vsToken).frozen, 0); assert_eq!(Tokens::accounts(CATHI, vsToken).reserved, 0); @@ -1135,47 +1087,9 @@ fn redeem_with_wrong_origin_should_fail() { }); } -#[test] -fn redeem_with_expired_vsbond_should_fail() { - new_test_ext().execute_with(|| { - assert_ok!(Salp::create(Some(ALICE).into(), 3_000, 1_000, 1, SlotLength::get())); - assert_ok!(Salp::contribute(Some(BRUCE).into(), 3_000, 100)); - assert_ok!(Salp::confirm_contribute( - Some(ALICE).into(), - BRUCE, - 3_000, - true, - CONTRIBUTON_INDEX - )); - assert_ok!(Salp::fund_success(Some(ALICE).into(), 3_000)); - assert_ok!(Salp::unlock(Some(BRUCE).into(), BRUCE, 3_000)); - - // Mock the BlockNumber - let block_begin_redeem = (SlotLength::get() + 1) * LeasePeriod::get(); - let block_end_redeem = block_begin_redeem + VSBondValidPeriod::get(); - System::set_block_number(block_end_redeem); - - assert_ok!(Salp::fund_retire(Some(ALICE).into(), 3_000)); - assert_ok!(Salp::withdraw(Some(ALICE).into(), 3_000)); - - #[allow(non_snake_case)] - let (vsToken, vsBond) = Salp::vsAssets(3_000, 1, SlotLength::get()); - - assert_ok!(>::transfer(vsToken, &BRUCE, &CATHI, 50)); - assert_ok!(>::transfer(vsBond, &BRUCE, &CATHI, 50)); - - assert_noop!(Salp::redeem(Some(BRUCE).into(), 3_000, 50), Error::::VSBondExpired); - - assert_noop!(Salp::redeem(Some(CATHI).into(), 3_000, 50), Error::::VSBondExpired); - }); -} - #[test] fn redeem_with_not_redeemable_vsbond_should_fail() { new_test_ext().execute_with(|| { - // Mock redeem-pool already had some balance - crate::pallet::RedeemPool::::set(100); - assert_ok!(Salp::create(Some(ALICE).into(), 3_000, 1_000, 1, SlotLength::get())); assert_ok!(Salp::contribute(Some(BRUCE).into(), 3_000, 100)); assert_ok!(Salp::confirm_contribute( @@ -1264,11 +1178,13 @@ fn redeem_without_enough_balance_in_pool_should_fail() { System::set_block_number(block_begin_redeem); assert_ok!(Salp::fund_retire(Some(ALICE).into(), 3_000)); + assert_ok!(Salp::withdraw(Some(ALICE).into(), 3_000)); // Before withdraw - assert_noop!(Salp::redeem(Some(BRUCE).into(), 3_000, 50), Error::::InvalidFundStatus); - - assert_ok!(Salp::withdraw(Some(ALICE).into(), 3_000)); + assert_noop!( + Salp::redeem(Some(BRUCE).into(), 3_000, 200), + Error::::NotEnoughBalanceInRedeemPool + ); }); } @@ -1307,10 +1223,6 @@ fn release_from_redeem_to_bancor_should_work() { run_to_block(ReleaseCycle::get()); - let release_amount = ReleaseRatio::get() * 100u128; - let redeem_pool_left = 100 - release_amount; - assert_eq!(Salp::redeem_pool(), redeem_pool_left); - // TODO: Check the balance of bancor(Waiting Bancor to Support..) }); } @@ -1326,28 +1238,6 @@ fn check_next_trie_index() { }); } -#[test] -fn check_is_expired() { - new_test_ext().execute_with(|| { - let slot = 10; - let block_end_of_slot = Salp::block_end_of_lease_period_index(slot); - let block_expired = block_end_of_slot + VSBondValidPeriod::get(); - - assert!(Salp::is_expired(block_expired, slot)); - }); -} - -#[test] -fn check_can_redeem() { - new_test_ext().execute_with(|| { - let slot = 10; - let block_end_of_slot = Salp::block_end_of_lease_period_index(slot); - let block_redeemable = block_end_of_slot + VSBondValidPeriod::get() / 2; - - assert!(Salp::can_redeem(block_redeemable, slot)); - }); -} - #[test] fn batch_unlock_should_work() { new_test_ext().execute_with(|| {