Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add asset_sufficient getter for fungibles #1768

Conversation

pandres95
Copy link
Contributor

@pandres95 pandres95 commented Oct 2, 2023

Description

This Pull Request introduces a method inside the fungibles::Inspect trait called asset_sufficient. This method is 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::asset_sufficient(ASSET_B), Some(true));

        // Additional assets owned by the community are not sufficient
        // by default
        assert_eq(Assets::asset_sufficient(ASSET_C), Some(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 method under the fungibles::Inspect trait. The method signature conveniently returns Option<bool>: to be None implies that the asset does not exist, so it's also a shortcut for asset_exists(asset) && asset_sufficient(asset), reducing code.
  2. Implement on the impl_fungibles.rs file, getting the asset's details, and mapping the response whenever Some(d) to d.is_sufficient.
  3. It was also necessary to implement the call for LocalAndForeignAssets.

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)
    • Note: Since I've checked similar getter methods are not included in tests, I left the tests intact (but I still modified them locally to ensure the implementation works).

Optional Test Case

diff --git a/substrate/frame/assets/src/tests.rs b/substrate/frame/assets/src/tests.rs
index 06d4ec1211..d9c1e5b71a 100644
--- a/substrate/frame/assets/src/tests.rs
+++ b/substrate/frame/assets/src/tests.rs
@@ -22,7 +22,11 @@ use crate::{mock::*, Error};
 use frame_support::{
        assert_noop, assert_ok,
        dispatch::GetDispatchInfo,
-       traits::{fungibles::InspectEnumerable, tokens::Preservation::Protect, Currency},
+       traits::{
+               fungibles::{Inspect, InspectEnumerable},
+               tokens::Preservation::Protect,
+               Currency,
+       },
 };
 use pallet_balances::Error as BalancesError;
 use sp_io::storage;
@@ -44,6 +48,8 @@ fn asset_account_counts(asset_id: u32) -> (u32, u32) {
 fn transfer_should_never_burn() {
        new_test_ext().execute_with(|| {
                assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, false, 1));
+               assert_eq!(Assets::asset_sufficient(0), Some(false));
+
                Balances::make_free_balance_be(&1, 100);
                Balances::make_free_balance_be(&2, 100);

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Oct 2, 2023

User @pandres95, please sign the CLA here.

@pandres95 pandres95 marked this pull request as ready for review October 2, 2023 01:06
@pandres95 pandres95 requested review from a team October 2, 2023 01:06
@pandres95 pandres95 marked this pull request as draft October 2, 2023 01:10
@pandres95 pandres95 marked this pull request as ready for review October 2, 2023 04:32
@pandres95 pandres95 force-pushed the pandres95-fungibles-inspect-asset_sufficient branch from e5dd40e to fb0d325 Compare October 3, 2023 17:24
@juangirini juangirini added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 5, 2023
@@ -117,6 +117,9 @@ pub trait Inspect<AccountId>: Sized {

/// Returns `true` if an `asset` exists.
fn asset_exists(asset: Self::AssetId) -> bool;

/// Returns `Some(true)` if an `asset` exists and is sufficient.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it return None if the asset does not exist? And Some(false) if it exists and is not sufficient?
A better doc would be useful here

Copy link
Contributor Author

@pandres95 pandres95 Oct 5, 2023

Choose a reason for hiding this comment

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

You are right. It's done.

The new doc states the following:

/// Returns `Some(true)` if an `asset` exists and is sufficient. Returns `Some(false)`
/// if an `asset` exists, but is not sufficient. Returns `None` if an `asset` does not exist.

It's already ready for review again @juangirini 😊

@paritytech-ci paritytech-ci requested a review from a team October 5, 2023 08:55
@pandres95 pandres95 force-pushed the pandres95-fungibles-inspect-asset_sufficient branch from e5e5ca0 to cb58dae Compare October 5, 2023 16:17
@pandres95
Copy link
Contributor Author

pandres95 commented Oct 17, 2023

It'd be good to know if this proposal is somewhat useful for the current scope of the project or otherwise, I should close it. Also, would be awesome to gather some feedback.


Sorry in advance if I'm not fully aware about how the reviews work for PRs.


/// Returns `Some(true)` if an `asset` exists and is sufficient. Returns `Some(false)`
/// if an `asset` exists, but is not sufficient. Returns `None` if an `asset` does not exist.
fn asset_sufficient(asset: Self::AssetId) -> Option<bool>;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this concept is general enough, I cannot really say.
but an alternative can be a separate trait on assets pallet level, which lets you inspect some additional characteristics of an asset.
if you decide to continue in this direction, we need a good explanation here what sufficient means.
also the returned value can be just bool here, since you have asset_exists to check it is existence.

@pandres95
Copy link
Contributor Author

pandres95 commented Jan 7, 2024

Per @muharem's suggestion, I decided to close this PR and continue on #2872.

@pandres95 pandres95 closed this Jan 7, 2024
@pandres95 pandres95 deleted the pandres95-fungibles-inspect-asset_sufficient branch January 7, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants