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 and configure asset registry pallet #276

Merged
merged 23 commits into from
Jul 27, 2023

Conversation

ebma
Copy link
Member

@ebma ebma commented Jul 14, 2023

Adds the orml-asset-registry pallet to all runtimes. The CustomMetadata struct is defined in the spacewalk primitives and should be changed to something that suits our needs. It is still to be unclear what the CustomMetadata should be, maybe we just keep it empty.

Remaining TODOs

  • Change metadata struct in Spacewalk. This has to happen in another repository though and does not require any other code changes here, besides then using the new commit hashes for spacewalk dependencies. (PR pending).

Notes

The benchmarking for the asset registry was adopted from here. You can find some context on the macros used in the benchmarks here.

The AssetAuthority is configured such that only the root user can register assets. Alternatively we could also give some pallets the permission to register these assets but we don't need this right now.

Closes #275.

@ebma ebma linked an issue Jul 14, 2023 that may be closed by this pull request
@ebma ebma marked this pull request as ready for review July 18, 2023 16:04
@ebma ebma requested a review from a team July 18, 2023 16:08
Copy link
Member

@TorstenStueber TorstenStueber left a comment

Choose a reason for hiding this comment

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

I generally approve this PR, particularly how you define the configuration parameters of the asset registry pallet (especially the AssetProcessor and the AuthorityOrigin).

There is a minor comment I have with the organization of the pallets in construct_runtime.

Another thought: this is the first time (?) that we add our own weights for pallets we use. There are some very small differences in the benchmarking results of these pallets for each runtime. I suppose that these are merely statistical deviations and the weights should actually be exactly the same for each runtime.

Would it be more reasonable to move our estimated weights for this pallet into runtime/common and avoid all the duplication?

This will set the precedent for further additions of our own benchmarked weights.

runtime/amplitude/src/lib.rs Show resolved Hide resolved
@ebma
Copy link
Member Author

ebma commented Jul 24, 2023

Another thought: this is the first time (?) that we add our own weights for pallets we use. There are some very small differences in the benchmarking results of these pallets for each runtime. I suppose that these are merely statistical deviations and the weights should actually be exactly the same for each runtime.

It's actually the second time. As part of the v0.9.40 upgrade, @b-yap recently added custom weights for the XCM pallet, see e.g. here.

Would it be more reasonable to move our estimated weights for this pallet into runtime/common and avoid all the duplication?

While I see your point with the assumption that the weights should be very similar, I think it would be dangerous to have one set of weights for all runtimes. Generally speaking we of course try to have similar configurations for all our parachains, but they are not perfect copies of each other. It might happen that one day, we change some configuration on Amplitude, that we don't change for Pendulum, which would then lead to different requirements/results for the generated weight.
If you look at other parachain projects, most of them seem to have custom weights per runtime. See:

Astar and Moonbeam seem to use the weights that are defined/exposed by the respective pallets and don't have any custom benchmarked weights.

Thus, I would suggest that we generate the weights for each runtime separately.

I think this topic is a little related to #178 although it does not align perfectly since the ticket is speaking about benchmarks for custom pallets, while we also consider benchmarks for non-custom pallets here.

@TorstenStueber
Copy link
Member

Okay, sounds reasonable.

@ebma ebma merged commit f5e9251 into main Jul 27, 2023
1 of 2 checks passed
@ebma ebma deleted the 275-add-and-configure-asset-registry-pallet branch July 27, 2023 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add and configure asset registry pallet
2 participants