Use can_hold instead of can_reserve in fn hold by KiChjang · Pull Request #12004 · paritytech/substrate

KiChjang · 2022-08-10T15:55:11Z

Per title, also tries to use safe math.

It seems to me that #8454 really intended the code to be calling InspectHold::can_hold rather than ReservableCurrency::can_reserve, but perhaps due to merge conflicts, the can_hold call has been clobbered by can_reserve.

Part of #8285 and paritytech/polkadot-sdk#327.

shawntabrizi · 2022-08-10T17:32:13Z

Test?

shawntabrizi

looks sensible to me

gavofyork · 2022-08-11T11:01:01Z

No test?

stale · 2022-09-14T05:24:24Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

…le-impl-in-balance

KiChjang · 2022-09-27T19:43:33Z

frame/balances/src/lib.rs

-		})?;
-		Ok(())
+		ensure!(
+			<Self as fungible::InspectHold<T::AccountId>>::can_hold(who, amount),


Okay, after writing unit tests, I found out that there is a difference between can_reserve and can_hold: the latter ensures that the reserved/held amount doesn't make the free balance go below the existential deposit. Should we still continue using can_hold here then? Or should we change can_hold to match the semantics of can_reserve?

if we change can_hold what that mess up any existing code using can_hold / what are the tradeoffs?

We want people to be able to use fungible traits instead of Currency in the future. See #8285 and #8453. We currently use Currency traits everywhere, and we'd like to change that.

sam0x17

Looks good to me once we have a definitive answer about the can_hold / can_reserve thing

kianenigma · 2022-10-11T07:54:55Z

7a6db84 Looks good.

stale · 2022-11-10T09:48:43Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

…le-impl-in-balance

gavofyork

holding/reserving should never kill an account, so adding the keep_alive arg doesn't make sense here.

ruseinov · 2022-11-29T15:54:45Z

bot rebase

…le-impl-in-balance

paritytech-processbot · 2022-11-29T15:54:52Z

Rebased

As per this comment: #12004 (review) This reverts commit 7a6db84.

…` into account.

ruseinov · 2022-11-29T17:22:23Z

Would appreciate input in terms of logic soundness for can_reserve.

The important differences between the current code and the last review:

keep_alive for can_hold has been reverted as we should never reap the account on hold.
ensure_can_withdraw has been removed from can_reserve as it is a subset of the new logic.
can_reserve now takes ExistentialDeposit into account to make sure there is no overdraft.
reserve is now enforcing can_reserve.

What this basically means is that can_reserve basically replicates can_hold logic in terms of balance assumptions.

paritytech-cicd-pr · 2022-12-02T13:11:05Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2110851

ruseinov · 2022-12-03T15:48:51Z

A bit stuck on this one while fixing other pallets, duplicating my post in FRAME Element room for posterity:

Basically, after introducing the change to can_reserve and reserve we have to reserve here

substrate/frame/contracts/src/storage/meter.rs

Line 489 in 493b58b

.and_then(|_| T::Currency::reserve(contract, *amount));

taking into account the ED. .and_then(|_| T::Currency::reserve(contract, *amount - ED)); . I'm not sure it's the correct behaviour. Any insight would be much appreciated.
This also then breaks the refund logic.

ruseinov · 2022-12-05T11:50:19Z

This breaks the current implementation of contracts pallet, currently blocked until @athei figures out how we want to go about those changes.

ruseinov · 2022-12-06T12:39:06Z

Additional notes:

Modify pallet-balances to take care of the below:

having a non-zero reserved balance should imply a consumer ref.
anything which alters the reserved balance should inc consumer refs when it goes from is_zero() -> !is_zero() and dec the consumer ref when the opposite is
true.

But I'm not sure how this can solve the contracts issue. Because if we only transfer the ED - it will be unreserved and therefore there won't be a consumer ref.

athei · 2022-12-06T16:34:14Z

But I'm not sure how this can solve the contracts issue. Because if we only transfer the ED - it will be unreserved and therefore there won't be a consumer ref.

pallet-contracts transfers at least the ED on contract instantiation and then reserves it.

ruseinov · 2022-12-08T10:57:50Z

But I'm not sure how this can solve the contracts issue. Because if we only transfer the ED - it will be unreserved and therefore there won't be a consumer ref.

pallet-contracts transfers at least the ED on contract instantiation and then reserves it.

Yeah, but we can't reserve the ED anymore due to the changes in can_reserve (which is called in reserve). Or rather we can, but we still need to have at least ED in free balance.

athei · 2022-12-08T16:13:12Z

I thought a while about this any my preferred solution would be that a contract can act as a sufficient for an account. It is a consumer because it depends on the existence of its account (and by extension its balance). But it is also a provider because it takes storage deposit which covers both the existential deposit and the data structures within pallet-contracts.

Up until now this was achieved by transferring this deposit to the contract and reserving it there. This does no longer work with this PR as the existential deposit must be free balance. This issue arises because pallet-contracts is a sufficient only indirectly (by enforcing a certain balance). I think that should change and pallet-contracts should call System::inc_sufficients directly when creating a contract. The workflow I imagine is as as follows:

Contract is about to be instantiated
System::inc_sufficients(contract)
Currency::transfer(caller, contract, storage_deposit)
Currency::reserve(contract, storage_deposit)

Step 4. is where it currently fails because we cannot reserve all the contract's balance as some of it needs to be free. We could work around this by partly leaving some balance free and creating a consumer to prevent the balance to be send away. However, I think that feels a bit weird when part of the deposit is free and some is reserved. Additionally, a storage migration is needed that changes all contract's accounts to have some free balance. This is why I suggest changing the can_reserve logic to the following which would allow the above workflow without a storage migration:

Allow reserving the ED (as it was the case before this PR) but remove the provider created by pallet-balances when doing so. If this would reap the account (because there is no other provider) we won`t allow the reserve because we never want the reserve to kill an account. Not sure if this is a sound solution. Need some more thinking and debate.

Another suggestion made by @gavofyork is to automatically add a consumer to an account whenever there is a non zero reserved balance. This would also work for contracts as we could send part of the initial balance as free balance as mentioned above. However, this also needs this big storage migration of all existing accounts with reserved balance.

stale · 2023-01-14T16:26:11Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

ruseinov · 2023-01-17T16:24:30Z

This has been stale for a while as we did not reach consensus on how to proceed with this as far as I understand.
This should fix it: #12951

stale · 2023-02-17T00:10:59Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

gavofyork · 2023-02-22T17:13:27Z

Superseded by #12951.

Use can_hold instead of can_reserve in fn hold

408ede3

KiChjang requested review from ggwpez, kianenigma and shawntabrizi August 10, 2022 15:55

shawntabrizi approved these changes Aug 10, 2022

View reviewed changes

kianenigma approved these changes Aug 11, 2022

View reviewed changes

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 14, 2022

Add tests for fungible::hold

462a9de

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 27, 2022

Merge remote-tracking branch 'origin/master' into kckyeung/fix-fungib…

8be79c2

…le-impl-in-balance

KiChjang commented Sep 27, 2022

View reviewed changes

cargo fmt

13d2261

sam0x17 approved these changes Sep 27, 2022

View reviewed changes

Add keep_alive parameter to can_hold

7a6db84

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Nov 10, 2022

Merge remote-tracking branch 'origin/master' into kckyeung/fix-fungib…

f41bd2a

…le-impl-in-balance

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Nov 15, 2022

gavofyork suggested changes Nov 17, 2022

View reviewed changes

ruseinov mentioned this pull request Nov 18, 2022

reserve operation should ensure repatriate_reserved works. #12701

Closed

Merge remote-tracking branch 'origin/master' into kckyeung/fix-fungib…

3730f24

…le-impl-in-balance

Revert "Add keep_alive parameter to can_hold"

9420645

As per this comment: #12004 (review) This reverts commit 7a6db84.

ruseinov self-assigned this Nov 29, 2022

Make sure ReservableCurrency::can_reserve takes `ExistentialDeposit…

17e226b

…` into account.

ruseinov requested review from bkchr and gavofyork November 29, 2022 17:19

ruseinov added 8 commits November 29, 2022 17:29

Enforce within .

5153396

fix reserve

b41b36b

fix tests

7916a0a

fix bench

b58674d

fix assets tests

84e648c

fix asset bench

8b50397

add a clarification

37ab667

fix benches

6b7da31

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jan 14, 2023

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jan 17, 2023

stale bot added the A3-stale label Feb 17, 2023

gavofyork closed this Feb 22, 2023

Conversation

KiChjang commented Aug 10, 2022

Uh oh!

shawntabrizi commented Aug 10, 2022

Uh oh!

shawntabrizi left a comment

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Aug 11, 2022

Uh oh!

stale bot commented Sep 14, 2022

Uh oh!

KiChjang Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam0x17 Sep 27, 2022

Choose a reason for hiding this comment

Uh oh!

KiChjang Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

sam0x17 left a comment

Choose a reason for hiding this comment

Uh oh!

kianenigma commented Oct 11, 2022

Uh oh!

stale bot commented Nov 10, 2022

Uh oh!

gavofyork left a comment

Choose a reason for hiding this comment

Uh oh!

ruseinov commented Nov 29, 2022

Uh oh!

paritytech-processbot bot commented Nov 29, 2022

Uh oh!

ruseinov commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paritytech-cicd-pr commented Dec 2, 2022

Uh oh!

ruseinov commented Dec 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ruseinov commented Dec 5, 2022

Uh oh!

ruseinov commented Dec 6, 2022

Uh oh!

athei commented Dec 6, 2022

Uh oh!

ruseinov commented Dec 8, 2022

Uh oh!

athei commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Jan 14, 2023

Uh oh!

ruseinov commented Jan 17, 2023

Uh oh!

stale bot commented Feb 17, 2023

Uh oh!

gavofyork commented Feb 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

KiChjang Sep 27, 2022 •

edited

Loading

ruseinov commented Nov 29, 2022 •

edited

Loading

ruseinov commented Dec 3, 2022 •

edited

Loading

athei commented Dec 8, 2022 •

edited

Loading