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

Make pallet Assets instantiable #8483

Merged
3 commits merged into from
Apr 16, 2021
Merged

Conversation

olanod
Copy link
Contributor

@olanod olanod commented Mar 29, 2021

As mentioned in paritytech/polkadot-sdk#327, making the assets pallet instantiable allows one to better group sets of assets that share something in common instead of putting everything in a single big group that might require some sort of filtering on the pallets that depend on some fungibles.

@olanod olanod marked this pull request as ready for review March 29, 2021 13:59
@shawntabrizi
Copy link
Member

@olanod asset creation is permissionless, so i don't see how such organization you are striving for will be enforced.

Do you have a more concrete idea on how you want to use this stuff?

@olanod
Copy link
Contributor Author

olanod commented Mar 29, 2021

But it also can be permissioned only right? At least some of the assets in our chain will be created only by an authority i.e. a set of valid collaterals for a given stable coin. Also I'm looking into assets pallet mostly as an implementor of the fungibles::* traits and not much for its own dispatchable calls.

@shawntabrizi
Copy link
Member

@olanod no, it is current not possible to make it permissioned without some sort of wrapper on what functions can be called:

https://github.com/paritytech/substrate/blob/master/frame/assets/src/lib.rs#L368

@olanod
Copy link
Contributor Author

olanod commented Mar 29, 2021

@shawntabrizi but that's fine no? I can have that wrapper as an extra pallet or a proxy that filters calls and still benefit from the instantiability of Assets pallet. I would say it's even more compelling to have different instances in this case, say I have one instance with some access control filter for cool people only and another one that is general purpose.

@shawntabrizi
Copy link
Member

shawntabrizi commented Apr 2, 2021

@olanod please take a look at the diff that github provides. We do not use spaces in Substrate, so please update them to tabs. Otherwise the changes look reasonable.

@shawntabrizi shawntabrizi added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit A0-please_review Pull request needs code review. labels Apr 2, 2021
@olanod
Copy link
Contributor Author

olanod commented Apr 2, 2021

My bad ... will have to disable the format on save 😅

@shawntabrizi
Copy link
Member

@thiolliere can you take a quick look here?

@olanod
Copy link
Contributor Author

olanod commented Apr 2, 2021

The easiest way I found to revert the spaces formatting was to format again overriding the hard_tabs setting of rustfmt, rustmt has a lot of opinions on how the code should look like but as its not the scope of the PR I tried to leave out as much of the extra formatting that was not directly related to the feature as possible splitting chunks as far as git add --patch --interactive allowed me but there might be a bit of formatted code left here and there that I hope you wont mind(and might be for the best).

frame/assets/src/lib.rs Outdated Show resolved Hide resolved
frame/assets/src/lib.rs Outdated Show resolved Hide resolved
frame/assets/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

ah I forgot, benchmarks need to be updated too, right now benchmark are only generated for default instance not for other ones.

diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs
index 227d45623d..d02fe7b74b 100644
--- a/frame/assets/src/benchmarking.rs
+++ b/frame/assets/src/benchmarking.rs
@@ -135,7 +135,7 @@ fn assert_event<T: Config>(generic_event: <T as Config>::Event) {
        }));
 }
 
-benchmarks! {
+benchmarks_instance_pallet! {
        create {
                let caller: T::AccountId = whitelisted_caller();
                let caller_lookup = T::Lookup::unlookup(caller.clone());

But there will be other changes inside the file

@shawntabrizi
Copy link
Member

@thiolliere check again?

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

looks good to me

@gui1117
Copy link
Contributor

gui1117 commented Apr 16, 2021

bot merge

@ghost
Copy link

ghost commented Apr 16, 2021

Trying merge.

@ghost ghost merged commit 0ce623a into paritytech:master Apr 16, 2021
@olanod
Copy link
Contributor Author

olanod commented Apr 16, 2021

Awesome! thanks @shawntabrizi was delaying that for the weekend 👍🏽

This pull request was closed.
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. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants