Skip to content

fix: only destroy funds on refund if collateral is BelowMinimum #866

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rflechtner merged 4 commits into from
Mar 6, 2025
Merged

fix: only destroy funds on refund if collateral is BelowMinimum #866

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f02e12d
fix: only destroy funds on refund if collateral is BelowMinimum
rflechtner Mar 3, 2025
a8e1a83
test: add negative test to check that funds are not burnt if account …
rflechtner Mar 4, 2025
4e31596
fixup! test: add negative test to check that funds are not burnt if a…
rflechtner Mar 5, 2025
87cb2e8
test: MaxConsumers limit does not lead to burn of funds
rflechtner Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions pallets/pallet-bonded-coins/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub mod pallet {
Create as CreateFungibles, Destroy as DestroyFungibles, Inspect as InspectFungibles,
Mutate as MutateFungibles,
},
tokens::{Fortitude, Precision as WithdrawalPrecision, Preservation, Provenance},
tokens::{DepositConsequence, Fortitude, Precision as WithdrawalPrecision, Preservation, Provenance},
AccountTouch,
},
Hashable, Parameter,
Expand Down Expand Up @@ -1119,8 +1119,7 @@ pub mod pallet {

if amount.is_zero()
|| T::Collaterals::can_deposit(pool_details.collateral.clone(), &who, amount, Provenance::Extant)
.into_result()
.is_err()
== DepositConsequence::BelowMinimum

@abdulmth abdulmth Mar 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in what case into_result().is_err() would be true? 🤔

@rflechtner rflechtner Mar 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DepositConsequence has multiple error cases and one success case. In all cases but the success case this would have been false, only the success case yields true. In other words, the transfer below can now still fail (because there's error cases we are not covering in this branch), leading to the extrinsic failing and being rolled back. As I explained in the PR description though, that would be intentional.

{
// Funds are burnt but the collateral received is not sufficient to be deposited
// to the account. This is tolerated as otherwise we could have edge cases where
Expand Down
43 changes: 42 additions & 1 deletion pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
// If you feel like getting in touch with us, you can do so at info@botlabs.org
use frame_support::{
assert_err, assert_ok,
traits::fungibles::{Create, Inspect, Mutate},
traits::fungibles::{roles::Inspect as InspectRole, Create, Inspect, Mutate},
};
use frame_system::{pallet_prelude::OriginFor, RawOrigin};
use sp_runtime::TokenError;
Expand Down Expand Up @@ -266,6 +266,47 @@ fn refund_below_min_balance() {
});
}

#[test]
fn refund_account_fails_when_account_blocked() {
Comment on lines +269 to +270

@rflechtner rflechtner Mar 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abdulmth We have a test that checks the minimum balance case above this one, and I added a test here that ensures no balance is burnt when the beneficiary account cannot receive the funds due to any other reason (here: being blocked). This test fails if you revert the changes I made to the refund_account extrinsic.

let pool_details = generate_pool_details(
vec![DEFAULT_BONDED_CURRENCY_ID],
get_linear_bonding_curve(),
false,
Some(PoolStatus::Refunding),
Some(ACCOUNT_00),
None,
None,
None,
);
let pool_id: AccountIdOf<Test> = calculate_pool_id(&[DEFAULT_BONDED_CURRENCY_ID]);

ExtBuilder::default()
.with_native_balances(vec![(ACCOUNT_01, ONE_HUNDRED_KILT)])
.with_pools(vec![(pool_id.clone(), pool_details)])
.with_collaterals(vec![DEFAULT_COLLATERAL_CURRENCY_ID])
.with_bonded_balance(vec![
(DEFAULT_COLLATERAL_CURRENCY_ID, pool_id.clone(), ONE_HUNDRED_KILT),
(DEFAULT_COLLATERAL_CURRENCY_ID, ACCOUNT_01, ONE_HUNDRED_KILT),
(DEFAULT_BONDED_CURRENCY_ID, ACCOUNT_01, ONE_HUNDRED_KILT),
])
.build_and_execute_with_sanity_tests(|| {
let origin: OriginFor<Test> = RawOrigin::Signed(ACCOUNT_01).into();

Assets::block(
RawOrigin::Signed(Assets::owner(DEFAULT_COLLATERAL_CURRENCY_ID).unwrap()).into(),
DEFAULT_COLLATERAL_CURRENCY_ID,
ACCOUNT_01,
)
.expect("Failed to block account for test");

// Ensure the refund_account call fails due to pool not being in refunding state
Comment thread
ntn-x2 marked this conversation as resolved.
Outdated
assert_err!(
BondingPallet::refund_account(origin, pool_id.clone(), ACCOUNT_01, 0, 1),
TokenError::Blocked
);
});
}

#[test]
fn refund_account_fails_when_pool_not_refunding() {
let pool_details = generate_pool_details(
Expand Down