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

Changes from 1 commit
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