paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Abstract FRAME's Account System #7792

Open danforbes opened 3 years ago

danforbes commented 3 years ago

I am going to kick things off by mostly paraphrasing/quoting a discussion with @thiolliere & @shawntabrizi related to FRAME's account management capabilities, specifically the management of account data. I am labeling this Issue as unconfirmed for now, because I would like someone like Guillaume and/or Shawn to chime in with more details about the why these capabilities exist and the needs that should be taken into consideration when designing improvements.

I began by asking why it seems that most FRAME runtimes use account data exclusively for the storage of Balances data (e.g. Node Template, Polkadot). Guillaume stated the following, and also mentioned that this is something Shawn had thought about:

Yes you are right, API is not handy to stored multiple pallet information. Though it might be doable by handimplementing some traits.

Shawn followed up with these details:

I think it can be abstracted to be more ergonomic to allow people to define whatever account structure they want and their own apis around those accounts to do this, we must extract the assumptions about the account type from frame_system, and instead have it be its own crate/pallet then you can imagine users could replace the frame_accounts pallet with their own version which is still compatible with frame_system, and this I think is the solution to full flexibility of accounts in substrate, something I am personally interested in as is, we kind of force users to fork frame_system to do fancy things, which is obv the exact opposite of what we want the traits should be super minimal and only what frame system would need to know or modify about an account. for example, a function to get and increment the nonce, to get an increment reference counters, etc but beyond those fundamental functions and information, a developer could design the account however they want even could have each piece of data be its own storage map if they wanted to be really inefficient, but again we leave that entirely to the developer

Shawn also provided a snippet of (pseudo)code:

trait FrameAccount<AccountIndex, RefFormat> {

    fn get_nonce() -> AccountIndex;

    fn set_nonce();

    fn get_ref_count() -> RefFormat;

    fn set_ref_count();
}

I am interested in the handling of account data because I believe it is related to providing friendly, flexible support for identity data.

https://github.com/paritytech/substrate/pull/7363 is an existing PR (Issue https://github.com/paritytech/substrate/issues/7343) that seems related to this.

burdges commented 3 years ago

I'd think Namecoin-like functionality gives a classical example accounts doing something else. It fits that interface fine I think.

I suppose Namecoin gets complicated since you need BLS signatures from BEEFY along with our fancy validator roll up designs to succinctly prove name ownership, but still..

As a rule, one should avoid personal details on public blockchains, preferably not even behind commitments or hashes. There are mechanisms for producing unique information, like proof-of-personhood parties, or maybe signing a fixed message with the RSA key in an epassport, assuming the epassports use roughly RSA-FDH and not RSA-PSS or similar.

gui1117 commented 3 years ago

TLDR: abstracting system account and allowing to store more information in T::AccountData is 2 different problem to me, though they can be done together for better API maybe.

I agree having frame-system as minimal as possible is generally a plus, and abstracting account system can potentially make sense for some usage. But I don't see why abstracting account system is needed to have more complex AccountData stored alongside FRAME account info (nonce and ref count).

If we extract account info into its own pallet, or use a trait instead, anyway we need to provide new types/functions to be able to handle multiple value stored alongside the account info. (like for now polkadot have balances stored in account info, similar trait as StoredMap will still be needed even if account is in its own pallet).

If we just want to have multiple pallets storing their information alongside FRAME account info (nonce and refcount) then the current abstraction is enough. AccountData is abstract and can hold multiple pallet information already, this is what I meant by "API is not handy to stored multiple pallet information. Though it might be doable by handimplementing some traits.".

That said, I agree it is better if ppl have types provided to implement multiple pallet information in AccountData easily.

A simple first thought I have, is just to have AccountData as a tuple of the wanted types, and then give types which implements StoredMap for each element of the tuple. Instead of having pallet_balances::AccountStore being System, it could be SystemTupleElement1. The code would look a bit like:

/// A tuple of at least 1 element.
trait TupleWithAtLeast1Element {
    type Element1: Default;
}

impl<A: Default> TupleWithAtLeast1Element for (A,) {
    type Element1 = A;
}
impl<A: Default, B: Default> TupleWithAtLeast1Element for (A, B) {
    type Element1 = A;
}

/// A type which implements StoredMap of the first element of a tuple account data.
// (name is bad though)
struct SystemTupleElement1For<T>(core::marker::PhantomData<T>);

impl<T: Config> StoredMap<T::AccountId, <T::AccountData as TupleWithAtLeast1Element>::Element1>
for SystemTupleElement1For<T>
where T::AccountData: TupleWithAtLeast1Element
{
    fn get(k: &T::AccountId) -> <T::AccountData as TupleWithAtLeast1Element>::Element1 {
        let _tuple = Account::<T>::get(k).data;
        // I guess all previous element of the tuple needs to implement Decode,
        // or otherwise instead of a tuple we can encode to a more complex structure which counts
        // how much byte are used by each element, for example (but we can probably do better):
        // `len_of_element_1 ++ element_1_encoded ++ len_of_element_2 ++ element_2_encoded ...`
        todo!("extract the element 1 from data")
    }

    fn try_mutate_exists<R, E: From<StoredMapError>>(
        _k: &T::AccountId,
        _f: impl FnOnce(&mut Option<<T::AccountData as TupleWithAtLeast1Element>::Element1>) -> Result<R, E>,
    ) -> Result<R, E> {
        todo!()
    }
}

// Do a macro to generate those codes for various tuple size

// Then in runtime it would look like this:
impl system::Config for Runtime {
...
type AccountData = (Balance, MyPalletBInfo);
}
impl balances::Config for Runtime {
type AccountStore = SystemTupleElement1;
}

But we can improve stuff differently, like AccountData instead of being the type, it could be a tuple of pallets, and each pallets would provide the additional AccountData by implementing a new trait PalletAccountData.

shawntabrizi commented 3 years ago

@thiolliere my general thought is that the entire API around handling and managing account data right now is defined by frame_system. There are tricks we can do within frame_system to try and make the APIs better and more flexible, and we should do that, but end of the day, users may have their own idea of how they want to handle user storage.

I would want to refactor accounts such that another user could build their own accounts module and replace frame_accounts, and thus build an entire blockchain using their own accounts primitives and APIs.

kianenigma commented 3 years ago

@shawntabrizi putting your last comment next the code snipped that you supposedly sent:

trait FrameAccount<AccountIndex, RefFormat> {
    fn get_nonce() -> AccountIndex;
    fn set_nonce();
    fn get_ref_count() -> RefFormat;
    fn set_ref_count();
}

While I like the notion of a configurable frame_accounts crate that can be replaced with anything else, I don't see how this brings further flexibility than the current AccountData, given that the trait is still enforcing a notion of ref_count and nonce, plus some additional stuff. Current AccountData can also support this by having various combinations of data in its .data field (which goes in the direction of @thiolliere's comment).

So all in all, not having read the offline conversation that happened, by reading this issue I can't figure out if this is about:

  1. a revamp of account structure?
  2. making AccountData.data more flexible to make it easily shared between multiple pallets?
gui1117 commented 3 years ago

At least the current structure constrain you to have everything store in a single storage. From other ppl comments it seems it can be useful to be able to organize in other way.

shawntabrizi commented 3 years ago

Yeah, @thiolliere is exactly right. For example, with things in a single storage, you may run into issues if you are wrapping multiple mutates within one another.

kianenigma commented 3 years ago

I see, thanks for explaining!