paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.78k stars 641 forks source link

FRAME: Move pallets over to use `fungible` traits #226

Open gavofyork opened 1 year ago

gavofyork commented 1 year ago

Eventually, we will also want to deprecate and remove Balances dispatchables:

Pallet migrations needed

Here's a list of pallets, turning this into a tracking issue of the migration process.

shawntabrizi commented 1 year ago

I made a comment here, but will bring it here too.

While changing from Currency to Fungible trait, I think you should also update the name of the Associated type.

For example, this is weird to me:

/// Currency type that this works on.
type Currency: FunInspect<Self::AccountId, Balance = Self::CurrencyBalance>
    + FunMutate<Self::AccountId>
    + FunBalanced<Self::AccountId>
    + FunHoldMutate<Self::AccountId, Reason = Self::RuntimeHoldReason>;

Perhaps we should use the name NativeBalance for the native token balance (Balances pallet), and TokenBalance for use of the Assets pallet and stuff.

bkontur commented 10 months ago

We can observe various approaches to this refactoring. I think it does not provide the best developer experience to see the "same" stuff implemented in multiple ways within a single repository:

// 1.
type Currency: Mutate<Self::AccountId> + Balanced<Self::AccountId>;
// 2.
type Currency: frame_support::traits::fungible::Inspect<Self::AccountId>;
// 3.
/// This is kind of unclear `Fn` as Function? or `Fn` as Fungible? What about Fungibles? Fns?
type Currency: FnMutate<Self::AccountId>
            + FnMutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason>
            + FnBalanced<Self::AccountId>;
// 4.
/// Also `Fun` as Function? or `Fun` as Fungible? What about Fungibles? Funs?
type Currency: FunMutate<Self::AccountId>
            + FunMutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason>
            + FunBalanced<Self::AccountId>;
// 5.
type Currency: InspectFungible<Self::AccountId>
            + MutateFungible<Self::AccountId>
            + HoldMutateFungible<Self::AccountId, Reason = Self::RuntimeHoldReason>;

If we reach a consensus, I have no problem completing the refactoring according to the chosen approach (at least for already merged code).

I would vote for //5. as it appears to be the most readable and avoids potential discrepancies between fungible and fungibles in the same context.

// 5.
InspectFungible
MutateFungible
HoldMutateFungible
BalancedFungible

Any thoughts?

liamaharon commented 10 months ago

Yeah I also really dislike 2,3,4. Thanks for this, good that we standardise.

I also don't really like 5. Instead I would suggest

Imo it's cleaner to differentiate traits using our existing module structure rather than making up a new pattern and writing as everywhere which imo is tedious to implement.

Let me know what you think.

bkontur commented 10 months ago

Yeah I also really dislike 2,3,4. Thanks for this, good that we standardise.

I also don't really like 5. Instead I would suggest

* If we are using only one of `fungible` or `fungibles` in the scope, approach 1.

* If we are using both of `fungible` and `fungibles` in the scope, just use `fungible::Inspect`, `fungibles::Balanced`, etc to differentiate them instead of doing a bunch of `as` namings.

Imo it's cleaner to differentiate traits using our existing module structure rather than making up a new pattern and writing as everywhere which imo is tedious to implement.

Let me know what you think.

cool, I am with you, yes, lets avoid those as, and simplify that, I will prepare PR (probably based on https://github.com/paritytech/polkadot-sdk/pull/1801) and lets see

Polkadot-Forum commented 9 months ago

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-4-0/5152/1