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

Deprecate Currency; introduce holds and freezing into fungible traits #12951

Merged
merged 174 commits into from
Mar 18, 2023

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Dec 16, 2022

Related to paritytech/polkadot-sdk#236
Fixes #12701
Polkadot Companion: paritytech/polkadot#6780
Cumulus Companion: paritytech/cumulus#2334

Introduces Freezing and Holds: two new facilities in Balances pallet and an interface for them through fungible and fungibles. Also reworks some internals of Balances pallet to make it safer. Requires Existential Deposit requirements to be made of the free balance, not the total balance.

Key Points

  • Introduces 4 new items in pallet_balances::Config trait:
    • type MaxHolds
    • type MaxFreezes
    • type HoldIdentifier
    • type FreezeIdentifier
    • Until you introduce pallets which use holds and freezes, then these can safely be set to ConstU32<0> in the case of the first two and () in the case of the second two.
  • Introduces a new dispatchable (extrinsic) in pallet_balances: upgrade_accounts.
  • Renames 2 dispatchables:
    • Balances::set_balance becomes Balances::force_set_balance and only accepts the free balance (not the reserved balance since this no longer makes sense).
    • Balances::transfer becomes Balances::transfer_allow_death
    • Uses of both within your codebases should be renamed as you upgrade the version of your Balances pallet.
  • Alters many function signatures of fungible/fungibles traits making them much more type-safe.
  • Merges the fungible::Transfer trait into fungible::Mutate (and same for fungibles).
  • Requires the fungible::Balanced trait to be explicitly implemented (and same for fungibles).
  • Introduces default implementations for almost all functionality in the fungible/fungibles traits.
  • Silently deprecates Currency traits in favour of fungible traits.
  • Accounts now require the Existential Deposit (Currency::minimum_balance()) in addition to any amount to be reserved. This is a BREAKING CHANGE and is likely to result in failures in tests and benchmarking.
  • An ExistentialDeposit of zero no longer works. If you need to allow any account to have a zero balance and retain associated account data, you must introduce a pallet which allows an unpermissioned frame_system::Pallet::<Runtime>::inc_providers().

Long Description

Some renaming:

  • Balances::set_balance becomes Balances::force_set_balance
  • Balances::transfer becomes Balances::transfer_allow_death

In the latter case, since ecosystem tooling largely depends on using transfer, a compatibility stub for transfer remains, however this naming is DEPRECATED and ecosystem tools should move over to using transfer_allow_death within 3 months at which point the transfer alias will be removed.

In general, it's now better to use the fungible trait API for Balances rather than messing with the dispatchables directly. The long-term plan is to remove the dispatchables and have all pallet-access be through XCM.

Holds and Freezes

Holds are roughly analagous to reserves, but are now explicitly designed to be infallibly slashed. They do not contribute to the ED but do require a provider reference, removing any possibility of account reference counting from being problematic for a slash. They are also always named, ensuring different holds do not accidentally slash each other's balances.

Freezes are essentially the same as locks, except that they overlap with Holds. Since Holds are designed to be infallibly slashed, this means that any logic using a Freeze must handle the possibility of the frozen amount being reduced, potentially to zero. A permissionless function should be provided in order to allow bookkeeping to be updated in this instance.

Both Holds and Freezes require an identifier which is configurable and is expected to be an enum aggregated across all pallet instances of the runtime.

Balances Internals

Alter reserve functionality in Balances so that it does not contribute to the account's existential deposit and does use a consumer reference. Remove the concept of withdraw reasons from locks. Migration is lazy: accounts about to be mutated will be migrated prior. New accounts will automatically be mutated. A permissionless upgrade dispatchable is provided to upgrade dormant accounts.

Typing

Type-safety is improved by removing all naked bool parameters from fungible(s) API functions. In particular, best_effort and force which were both previously boolean are now typed as Precision and Fortitude. Lesser used arguments which have changed are on_hold which is typed as Restriction and minted which is typed as Provenance. This generally makes invocations much more readable.

Privatizations

One function mutate_account (in Balances pallet) has been privatized. It should never have been public since it ignores some important bookkeeping. If you're using it, you probably mean to be using make_free_balance_be instead.

Upgrading

Because reserved/held funds do not any longer contribute to the ED, you may find that certain test and benchmark code no longer works. This is generally due to the case where exactly as many funds were placed in an account as were later reserved. To fix this breakage, simply ensure that at least the currency's minimum_deposit is in the account in addition to any funds which are planned to be reserved. It's usually as simple as appending + T::Currency::minimum_balance() to the argument of make_free_balance_be.

In some circumstances, errors have changed to the more general errors.

After introducing a runtime with this change in it, chains should ensure that all accounts are upgraded by repeatedly calling update_accounts giving it account IDs which are not yet upgraded. A script which does this shall be provided.

Pallets which use the existing set of Currency traits should be refactored to use the fungible traits. Aside from the actual code change, a gateway function (ensure_upgraded) should be introduced and called prior to any mutating operations to ensure that any accounts using the old Currency traits are unreserved/unlocked and said funds are instead placed on hold/frozen. A permissionless fee-free dispatchable should also be introduced (upgrade_accounts), similar to that of the Balances pallet, which is able to do this for dormant accounts.

It is not crucial to get this done in any particular timeframe, however at some point in the future Currency traits, Locks and Reserves will be deprecated and removed, so it will need to happen before then.

Important Notes

Some functions in the Currency traits retain their name and parameter set in the fungible traits and have similar semantics. However, there are many functions with differences in naming, and some with differences in result types and their meaning.

  • Currency::free_balance may be replaced with fungible::Inspect::balance.
  • Currency::make_free_balance_be may be replaced with fungible::Mutate::set_balance.
  • ReservableCurrency::reserved_balance may be replaced with fungible::hold::Inspect::total_balance_on_hold.
  • NamedReservableCurrency::reserve may be replaced with fungible::hold::Mutate::hold.
  • NamedReservableCurrency::unreserve may be replaced with fungible::hold::Mutate::release, however the returned Result must be handled and the inner of Ok has the opposite meaning (i.e. it is the amount released, rather than the amount not released, if any).
  • LockableCurrency::set_lock may generally be replaced with fungible::freeze::Mutate::set_freeze.
  • LockableCurrency::extend_lock may generally be replaced with fungible::freeze::Mutate::extend_freeze.
  • LockableCurrency::remove_lock may be replaced with fungible::freeze::Mutate::thaw.

There are no equivalents for ReservableCurrency::reserve or ::unreserve: an identifier must always be used.

Functions which may result in an account being deleted accept a new Preservation value. This has three variants:

  • Expendable: Equivalent to ExistenceRequirement::AllowDeath
  • Protect: The account must not die. If there are multiple provider refs, then this may still allow the balance to be reduced to zero.
  • Preserve: Not only must the account not die, but the provider reference of the Currency implementation must also be kept in place.

Pallet NIS

NOTE: This updates NIS to use only the fungible API rather than Currency. This is fine since NIS is not deployed anywhere. For other pallets already in production then a progressive migration will be needed.

Pallet NIS has been altered to use active_issuance rather than total_issuance. This means the IgnoredIssuance is whatever should be discounted against active_issuance. It should probably be replaced with ().

TODO

  • Create UnbalancedHold.
  • Implement UnbalancedHold for balances.
  • Implement all the other Hold traits for UnbalancedHold implementations.
  • Deprecate fee_frozen and WithdrawReasons; just have a single frozen field in AccountData.
  • Impl InspectFreeze and MutateFreeze for Balances.
    • Main implementation
    • Tests for MutateFreeze
    • Tests for combination of freezing and locking
  • Support arbitrary IDs for freezes and holds.
  • Change freeze/hold mechanism to not be mutually exclusive.
    • Introduce tests for this.
  • Permissionless fee-free dispatchable for running ensure_upgraded on a non-upgraded account.
    • Benchmarking
    • Test
  • Refactor fungible/fungibles
    • Remove all blanket impls.
    • Events for all mutator traits.
    • Bring fungibles up to date.
    • Make ItemOf work properly again.
    • Make Assets pallet build.
  • Refactor fungible/fungibles/nonfungibles:
    • Rename suspend/resume to shelve/restore.
  • Fix tests using Balances pallet owing to reserves now requiring a consumer ref and not counting toward ED.
  • Script for calling upgrade_accounts: https://github.com/ggwpez/substrate-scripts/blob/master/upgrade-accounts.py
  • Handle dust in legacy uses of mutate account.
  • Fix up remaining usages of old bool API.
  • Fix up assets and balances benchmarks.

Maybe

  • Make Currency an optional feature.

Next up: paritytech/polkadot-sdk#226

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Dec 16, 2022
@gavofyork gavofyork marked this pull request as draft December 16, 2022 12:01
@gavofyork gavofyork added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited T1-runtime This PR/Issue is related to the topic “runtime”. T2-API This PR/Issue is related to APIs.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

reserve operation should ensure repatriate_reserved works.