feat(pallet-assets): define and implement sufficients traits for pallet-assets

[pandres95]

This pull request supersedes #1768, and implements methods to access and mutate is_sufficient value of an asset.

---

Description

This Pull Request introduces two traits under pallet-asset: a trait called sufficients::Inspect, and a trait called sufficients::Mutate. These methods are useful since pallet implementors can benefit from having this (so far missing) piece of information, either for testing checks or for handling accounts touching on behalf of others.

Use Cases

Use Case: Checking whether an asset was appropriately created as sufficient

A pallet might have a rule that allows creating assets, but only marking one of these (i.e. the first) as sufficient. A typical test for this would resemble something like this

#[test]
fn it_works() {
    new_test_ext().execute_with(|| {
        setup();

        // Can register a new asset
        assert_ok!(
            Communities::create_asset(RuntimeOrigin::signed(COMMUNITY_ADMIN), COMMUNITY, ASSET_B, 1)
        );

        // Can register additional assets
        assert_ok!(
            Communities::create_asset(RuntimeOrigin::signed(COMMUNITY_ADMIN), COMMUNITY, ASSET_C, 1)
        );

        // First asset owned by the community is sufficient by default
        assert_sufficiency(COMMUNITY, ASSET_B, 1, true);

        // Additional assets owned by the community are not sufficient
        // by default
        assert_sufficiency(COMMUNITY, ASSET_C, 1, false);
    });
}

Without this getter, implementing assert_sufficiency would require (as shown here) the following steps:

1. Forcing access to the storage via sp_io::storage::get.
2. Encoding a AssetDetails struct that resembles the expected one, since is_sufficient is a private field.
3. Asserting equality for both structs.

With the getter, it would be something as simple as this, no further illegal accesses required:

        // First asset owned by the community is sufficient by default
        assert_eq(Assets::is_sufficient(ASSET_B), true);

        // Additional assets owned by the community are not sufficient
        // by default
        assert_eq(Assets::is_sufficient(ASSET_C), false);

Use Case: Checking sufficiency for an existing asset

Some pallets might see it useful to check whether an asset is sufficient, for some cases where it might be useful (e.g. some restricted versions of DEXes where pool implementors would like to check that those assets are sufficient).

Without the getter, this would require building the pallet as an extension of the Assets pallet. Some tricks like the one explained in the use case above would be impractical.

Implementation

The implementation is actually really simple.

1. Define the traits under substrate/frame/pallets/assets/src/traits.rs: sufficients::Inspect and sufficients::Mutate. The method signature returns bool, defaults to false when the asset does not exist.
2. Implement on the impl_sufficients.rs file, getting the asset's details, and mapping the response as asset.is_sufficient.

Checklist

• My PR includes a detailed description as outlined in the "Description" section above
• My PR follows the labeling requirements of this project (at minimum one label for T
  required)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[pandres95]

Per @muharem's suggestion, I decided to close #1768 and continue here. Ready to gather feedback (I know this PR might come with some controversy).

[joepetrowski]

sufficients isn't really a good name because the mod has nothing to do with the behavior of sufficient assets, but merely setting the sufficient property to true or false.

I would therefore call it sufficiency.

I also question whether this should be in the Assets pallet at all and not frame-support like the Account Touch trait.

[joepetrowski]

@muharem and @liamaharon have done some recent work on this pallet, maybe they can comment.

[pandres95]

I get it. Just resolved all the other changes.

As for whether sufficiency should be on Assets pallet and not on frame-support, my hint was this @muharem's comment:

not sure if this concept is general enough, I cannot really say.

This is because this concept might not be general enough, but is somewhat specific to Assets. Instead, the most general concept, actually, is the AccountTouch trait.

but an alternative can be a separate trait on Assets pallet level, which lets you inspect some additional characteristics of an asset.

That's what I based on to close #1768 and open this PR instead. 😁

[muharem]

I clearly see use cases for a trait to read the is_sufficient flag. But I am not fully agree with the use case you giving in the description - "Checking whether an asset was appropriately created as sufficient". When it is tests within the package, I think it is fine to access the internals of a package (e.g. storage). And you should not need to test it as in your example from outside.

I also would like to question if you have a real use case, where you need to alter is_sufficient flag. if you look to the public api of the pallet, the altering api - force_asset_status has more parameters then just one flag. I can easily imagine someone coming and introducing new function with all those parameters. What I am trying to say, it is good to have real use cases for introducing such contracts. And not trying to say that one better than another.

[muharem]

maybe traits and impls? otherwise the list of files will grow here pretty quick.
I also have only strong opinion on keeping our folders/files/modules style consistent, since there is no yet agreement on it, I would just try to keep it close to what we have now, and now we tend to have less files than many.

[pandres95]

Would this mean also moving impl_fungibles and impl_stored_map onto an impls file/folder? I see the point on making the file structure lighter, but seeing that other impls are not that light (i.e. impl_fungibles is 311 lines) a bit of not-flatness might be good for the sake of readibility from time to time.

My suggestion:

...
impls/
├─ fungibles.rs
├─ stored_maps.rs
├─ sufficiency.rs
├─ mod.rs
...
traits.rs
...

[pandres95]

Reply to #2872 (comment)

also I see the possibility for these traits moving support module

I see it's possible to come up with a more general solution, by considering a more general trait that works alongside CanTouch and would be CanProvide/SetProvider (a hook).

This would general enough, and would enable every possible entity out there to be a provider or set that possibility. Now, I'm not sure of whether that would be achievable without some ambiguity (on entity ID, for example) so I think sticking to sufficiency for now would help @muharem. Maybe if we get enough feedback on a way around, I could move forward with a more general solution that can go on frame-support in the future.

[pandres95]

Since I've already implemented the last set of requested changes, it'd be nice to have some sort of feedback regarding this PR, before I can move forward with a further proposal on generalizing this concept. I'd appreciate that.

cc/ @muharem @joepetrowski

[pandres95]

Closing due to inactivity.

If someone is interested about this proposal in the future, just ping me.