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

Insufficient asset quota and deposits #10382

Merged
merged 21 commits into from Dec 9, 2021
Merged

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Nov 28, 2021

Fixes #10378
Polkadot companion: paritytech/polkadot#4390
Cumulus companion: paritytech/cumulus#804

Make two changes:

  • System accounts can only have a maximum number of consumers; once this limit is exhausted, any consumer-requiring operations will fail. This includes new asset-accounts.
  • Asset accounts (i.e. the storage of a specific account's balance of a specific asset) can now have three reasons for existing: the old two (i.e. the asset being "sufficient" or taking up a consumer reference) as well as having a deposit explicitly held for the asset-account. This deposit is placed by using the new touch function and refunded by using the new refund function.

Additional considerations

Consider the possibility of requiring touch to be used to receive any insufficient asset, which would mean the user would have to take active measures to receive an asset but that they could not gather consumer references inadvertently.

Downstream Code Migration

The System config trait frame_system::Config now has an additional item MaxConsumers. e.g.

impl frame_system::Config for Runtime {
  /* snip */
  type MaxConsumers = frame_support::traits::ConstU32<16>;
}

The Assets config trait frame_assets::Config now has an additional item AssetAccountDeposit. e.g.

parameter_types! {
  pub const AssetAccountDeposit: Balance = 5 * CENTS;
}

impl frame_assets::Config for Runtime {
  /* snip */
  type AssetAccountDeposit = AssetAccountDeposit;
}

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Nov 28, 2021
@gavofyork gavofyork added B7-runtimenoteworthy 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 Nov 28, 2021
@gavofyork gavofyork added this to In progress in Runtime via automation Nov 28, 2021
Copy link
Contributor

@adoerr adoerr left a comment

Choose a reason for hiding this comment

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

beefy-mmr, beefy and merkle-mountain-range LGTM

@girazoki
Copy link
Contributor

The impression I have is that the new ExistenceReason enum is designed to avoid migrations with the current boolean type right? So my assumption is that the PR does not imply data migrations?

frame/assets/src/types.rs Outdated Show resolved Hide resolved
frame/assets/src/functions.rs Show resolved Hide resolved
@thiolliere
Copy link
Contributor

the trait FrozenBalance says:

	/// In reality, the balance of every account must be at least the sum of this (if `Some`) and
	/// the asset's minimum_balance, since there may be complications to destroying an asset's
	/// account completely.
	///
	/// If `None` is returned, then nothing special is enforced.
	///
	/// If any operation ever breaks this requirement (which will only happen through some sort of
	/// privileged intervention), then `melted` is called to do any cleanup.

I think it needs to be updated.

frame/assets/src/lib.rs Outdated Show resolved Hide resolved
@gavofyork
Copy link
Member Author

@thiolliere @shawntabrizi @KiChjang Updated.

@gavofyork gavofyork added this to In review in Gav's Board Dec 8, 2021
Copy link
Contributor

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

looks good to me now

Runtime automation moved this from Please Review to Needs Audit Dec 8, 2021
@thiolliere
Copy link
Contributor

the trait FrozenBalance says:

	/// In reality, the balance of every account must be at least the sum of this (if `Some`) and
	/// the asset's minimum_balance, since there may be complications to destroying an asset's
	/// account completely.
	///
	/// If `None` is returned, then nothing special is enforced.
	///
	/// If any operation ever breaks this requirement (which will only happen through some sort of
	/// privileged intervention), then `melted` is called to do any cleanup.

I think it needs to be updated.

@thiolliere How so?

It is now good with #10431

@gavofyork
Copy link
Member Author

@thiolliere An approval would be good :)

Copy link
Contributor

@thiolliere thiolliere left a comment

Choose a reason for hiding this comment

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

looks good to me

@gavofyork gavofyork merged commit 8a3434b into master Dec 9, 2021
@gavofyork gavofyork deleted the gav-insufficient-assets-fix branch December 9, 2021 12:22
Runtime automation moved this from Needs Audit to Done Dec 9, 2021
@shawntabrizi shawntabrizi moved this from Done to Archive in Runtime Dec 9, 2021
@viniul viniul added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Dec 16, 2021
seunlanlege pushed a commit to seunlanlege/substrate that referenced this pull request Dec 17, 2021
* Allow asset accounts to exist by deposit

* Place limit on consumers (and therefore freebie asset accounts)

* Maximum number of assets

* Fixes

* Fixes

* Formatting

* Docs

* Formatting

* Destroyed assets are properly tidied

* Update frame/assets/src/types.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

* Docs

* Docs

* Formatting

* Docs

* Docs

* Fixes

* Fixes

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
HCastano added a commit to paritytech/canvas that referenced this pull request Jan 8, 2022
cmichi pushed a commit to paritytech/canvas that referenced this pull request Jan 10, 2022
* Bump Cumulus, Polkadot, and Substrate

Also bumps some other depenencies

* Remove duplicate `polkadot` dependency

* Update `service.rs`

Changes related to: paritytech/cumulus#835

* Update `command.rs`

* Add `AddressGenerator` config type

From: paritytech/substrate#10521

* Allow Root to execute overweight XCMP messages

From: paritytech/cumulus#799

* Add `header` argument to `collect_collation_info`

From: paritytech/cumulus#882

* Update Cumulus and friends again

* Add Fork ID to genesis config

paritytech/cumulus#870

* Add `state_version` field

paritytech/substrate#9732

* Add `MaxConsumers` config parameter

paritytech/substrate#10382

* Update Substrate compatibility note in README
wischli added a commit to KILTprotocol/kilt-node that referenced this pull request Jan 31, 2022
wischli added a commit to KILTprotocol/kilt-node that referenced this pull request Feb 9, 2022
* chore: bump Polkadot dependencies to v0.9.16

* refactor: use AllPalletsWithSystem hook

* chore: remove crowdloan pallet

* fix: apply no DefaultAccount check

* fix: switch to default MaxEncodedLen

paritytech/substrate#10662

* fix: apply asset quota + runtime state_version

quota: paritytech/substrate#10382
state: paritytech/substrate#9732

* fix: Preimage registrar and Scheduler integration

Scheduler: paritytech/substrate#10356
OriginPriviligeCmp: paritytech/polkadot#4166

* fix: OrderedSet

* fix: apply new fork_id to chainspecs

paritytech/substrate#10463

* fix: apply no default account for SudoConfig

* fix: apply name changes for GrandPa

paritytech/substrate#10463

* fix: EnsureOneOf

paritytech/substrate#10379

* fix: preimage weights

* fix: parachain client

* fix: clippy copied weights

* fix: bump deps

* tests: attempt staking fix

* bench: attempt to fix default accountid for dids

* fix: staking unit test pallet order execution

* fix: did unit benchmarks

* fix: fmt

* fix: revert to old hook order execution

* bench: run manually for preimage + scheduler

* fix: only import TaskManager for try-runtime feature

* fix: use correct scheduler migration + add logs

* refactor: use explicit para runtime executors

* fix: apply code review by @Diiaablo95

* chore: apply suggestion from @weichweich review

* chore: bump deps

* fix: deps
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Allow asset accounts to exist by deposit

* Place limit on consumers (and therefore freebie asset accounts)

* Maximum number of assets

* Fixes

* Fixes

* Formatting

* Docs

* Formatting

* Destroyed assets are properly tidied

* Update frame/assets/src/types.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

* Docs

* Docs

* Formatting

* Docs

* Docs

* Fixes

* Fixes

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
AurevoirXavier added a commit to darwinia-network/darwinia-common that referenced this pull request Sep 8, 2022
AurevoirXavier added a commit to darwinia-network/darwinia-common that referenced this pull request Sep 8, 2022
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Allow asset accounts to exist by deposit

* Place limit on consumers (and therefore freebie asset accounts)

* Maximum number of assets

* Fixes

* Fixes

* Formatting

* Docs

* Formatting

* Destroyed assets are properly tidied

* Update frame/assets/src/types.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

* Docs

* Docs

* Formatting

* Docs

* Docs

* Fixes

* Fixes

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
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. 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
Projects
No open projects
Gav's Board
In review
Development

Successfully merging this pull request may close these issues.

Accounts should have limited amounts of insufficient assets before deposit needed
7 participants