Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

reserve operation should ensure repatriate_reserved works. #12701

Closed
gavofyork opened this issue Nov 14, 2022 · 6 comments · Fixed by #12951
Closed

reserve operation should ensure repatriate_reserved works. #12701

gavofyork opened this issue Nov 14, 2022 · 6 comments · Fixed by #12951
Assignees

Comments

@gavofyork
Copy link
Member

Any pallet implementing ReservableCurrency or fungible::MutateHold should ensure that the reserve/hold operation can be followed by a slash or repatriation of the reserved amount without bringing the state becoming invalid. I.e. something like this should always succeed:

Balances::make_free_balance_be(1, 1000);
if Balances::reserve(1, 1000).is_ok() {
	System::inc_consumers(1);
	assert_ok!(Balances::slash_reserved(1, 1000);
	assert_eq!(System::consumers(1), 1);
	assert_eq!(System::providers(1), 1);
}

Right now, this will fail as the balance is both reserved/slashed and used for a provider ref.

The solution is to ensure that any reserved amounts are not considered toward the ED (only the free, unlocked balance); if a transfer, reserve or lock operation were to take the unlocked free balance below the ED then it should fail.

@ruseinov
Copy link
Contributor

some work towards that goal: #12004
planning to take over this sometime soon

@gavofyork
Copy link
Member Author

@ruseinov any update here?

@ruseinov
Copy link
Contributor

@gavofyork on it now!

@ruseinov ruseinov self-assigned this Nov 29, 2022
@ruseinov
Copy link
Contributor

@gavofyork this is done here: #12004
All this PR needs is test fixes, which is what I'm going to take care of shortly. Meanwhile I would appreciate a quick check if the solution is a correct one.

According to the specs of this ticket it should be. In my opinion it could be better, since we are basically duplicating some of that logic between ensure_can_withdraw and can_hold.

On the other hand, if we're planning to deprecate Currency altogether - that's probably not such a bad thing. Now that I look at this code it makes little sense that both can_reserve and reserve were calling ensure_can_withdraw.
I could not find any usage occurrences of can_reserve in any code in substrate, which leads me to believe that the initial idea was to call that in reserve, which is what we are doing now.

@gavofyork
Copy link
Member Author

Should be fundamentally fixed with #12951

@Polkadot-Forum
Copy link

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-41-v0-9-42/2828/1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants