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

Refactor Account Storage into Accounts Pallet #8254

Closed
wants to merge 35 commits into from

Conversation

shawntabrizi
Copy link
Member

@shawntabrizi shawntabrizi commented Mar 3, 2021

The account abstraction in FRAME System has become increasingly complex, and does not easily allow for customizable Account storage patterns.

Here, we extract the Account Storage from FRAME System and place those items in a new pallet with a simple API.

This allows developers to crate their own account abstraction while still using FRAME System with all of the macros.

FRAME System now only requires the BasicAccount API to be satisfied:

/// A minimal API for creating an account system compatible with FRAME System.
pub trait BasicAccount<AccountId, Index> {
	type AccountInfo;

	/// Return whether an account exists in storage.
	fn account_exists(who: &AccountId) -> bool;

	/// Return the data for an account.
	fn get(who: &AccountId) -> Self::AccountInfo;

	/// Retrieve the account transaction counter from storage.
	fn account_nonce(who: AccountId) -> Index;

	/// Increment a particular account's nonce by 1.
	fn inc_account_nonce(who: AccountId);

	/// Return the storage key for an account.
	fn hashed_key_for(who: AccountId) -> Vec<u8>;
}

FRAME System provides an implementation for a basic account, where the only data about a user is their nonce.

Other pallets may need to apply and use reference counters on accounts to prevent their account deletion. This is now exposed through an extended ReferencedAccount API:

/// An API on top of `BasicAccount` which provides the ability for users to place
/// reference counters on an account, preventing account deletion.
pub trait ReferencedAccount<AccountId, Index>: BasicAccount<AccountId, Index> {
	type RefCount;
	type IncRefStatus;
	type DecRefStatus;
	type IncRefError;
	type DecRefError;

	/// Increment the provider reference counter on an account.
	fn inc_providers(who: &AccountId) -> Self::IncRefStatus;

	/// Decrement the provider reference counter on an account.
	///
	/// This *MUST* only be done once for every time you called `inc_providers` on `who`.
	fn dec_providers(who: &AccountId) -> Result<Self::DecRefStatus, Self::DecRefError>;

	/// Increment the self-sufficient reference counter on an account.
	fn inc_sufficients(who: &AccountId) -> Self::IncRefStatus;

	/// Decrement the sufficients reference counter on an account.
	///
	/// This *MUST* only be done once for every time you called `inc_sufficients` on `who`.
	fn dec_sufficients(who: &AccountId) -> Self::DecRefStatus;

	/// The number of outstanding provider references for the account `who`.
	fn providers(who: &AccountId) -> Self::RefCount;

	/// The number of outstanding sufficient references for the account `who`.
	fn sufficients(who: &AccountId) -> Self::RefCount;

	/// The number of outstanding provider and sufficient references for the account `who`.
	fn reference_count(who: &AccountId) -> Self::RefCount;

	/// Increment the reference counter on an account.
	///
	/// The account `who`'s `providers` must be non-zero or this will return an error.
	fn inc_consumers(who: &AccountId) -> Result<(), Self::IncRefError>;

	/// Decrement the reference counter on an account. This *MUST* only be done once for every time
	/// you called `inc_consumers` on `who`.
	fn dec_consumers(who: &AccountId);

	/// The number of outstanding references for the account `who`.
	fn consumers(who: &AccountId) -> Self::RefCount;

	/// True if the account has some outstanding references.
	fn is_provider_required(who: &AccountId) -> bool;
}

The requirement to use ReferencedAccount will come from specific pallets that require this functionality, but will no longer be an assumption about all accounts in FRAME.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Mar 3, 2021
@kianenigma kianenigma added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 3, 2021
@shawntabrizi shawntabrizi added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Mar 4, 2021
@shawntabrizi
Copy link
Member Author

This is ready for a review on direction and design

@shawntabrizi shawntabrizi requested a review from athei as a code owner March 5, 2021 00:07
@shawntabrizi shawntabrizi changed the title Refactor Account Storage into FRAME Accounts Refactor Account Storage into Accounts Pallet Mar 9, 2021
@shawntabrizi
Copy link
Member Author

shawntabrizi commented Mar 9, 2021

Some feedback when fixing errors. Since Balances pallet uses reference counters, and most pallets use Currency, I had to add the pallet-accounts pallet to nearly every pallet as a dev dependency.

In most of these pallets, the use of reference counting was never relevant or part of the tests. I wanted to implement some dummy struct like NoReferenceCounters which would make all of these functions into noops, but wasnt confident that would be good. The upside is that we do not need to import or use pallet-accounts in most of our pallets, we can simply rely on System + Balances to manage their own storage. The downside is that we would be ignoring some aspect of the balances pallet in our tests, which is the use of reference counters...

I am also thinking to split up the reference counters into smaller traits since most pallets only use one kind of reference counters for their needs.

What do you all think?

Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Looks like the meat of this PR is all in the first 4 commits, so that's where I focused most of my energy. The rest appears to be mostly fixups to propagate the changes through the rest of the system.

I think that overall NoReferenceCounters is a good idea to reduce the amount of dev-dependencies propagated. Let's only pay for what we're using; if we're not using the reference counting, then let's structure things such that they aren't required.

Smaller reference counter traits also seem like a good idea to me, because it's not obvious as a newcomer to the topic what the distinctions between them are.

The major reason I'm not currently approving this is that, if you do take those suggestions within this PR, then substantial work is incoming and an approval is premature. If you'd prefer to put those into new issues/PRs, then I'd be prepared to approve this once CI is happy.

@@ -238,7 +243,8 @@ impl pallet_balances::Config for Runtime {
type Event = Event;
type DustRemoval = ();
type ExistentialDeposit = ExistentialDeposit;
type AccountStore = System;
type AccountStore = Accounts;
type ReferencedAccount = Accounts;
Copy link
Contributor

Choose a reason for hiding this comment

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

name suggestion: AccountReferences?

@gnunicorn gnunicorn added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label May 19, 2021
@paritytech paritytech deleted a comment from cla-bot-2021 bot Jun 3, 2021
@stale stale bot closed this Jul 7, 2021
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. A3-in_progress Pull request is in progress. No review needed at this stage. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants