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.

Sign up for GitHub

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

Jump to bottom

refactor: enforce no runtime in pallet #349

Merged
merged 12 commits into from
Apr 6, 2022
Merged

refactor: enforce no runtime in pallet #349

merged 12 commits into from
Apr 6, 2022

Conversation

weichweich
Copy link
Contributor

@weichweich weichweich commented Apr 5, 2022 •
edited

No Ticket

This PR enforces that no pallet* has dependencies to any runtime code. Which boils down to:

  • All creates in pallet/* have no dependency to any crates in nodes/ and runtimes/*
  • runtimes/* has dependencies to /pallets/* but not to /nodes*
  • /nodes/* has dependencies to everything.

Why would we do that!? It makes the test for each pallet more redundant!

In a pallet: depending on runtime-common prevents us from having common configuration for that pallet.

If a pallet has a dependency to runtime-common, we can't have a dependency the other way. So if we need to have a configuration struct in a pallet, this struct can't get set inside the runtime common pallet. Or in more general if we have a custom type in a pallet, that depends on runtime-common we can't use this type in the runtime-common crate.

Pallets should not depend on runtime code in general

A pallet should have no dependency on runtime specifics anyways.
Pallets are usable in what ever runtime you want.
Using runtime code in a pallet would mean that we bind the pallet to this specific runtime (even if it's only used in tests).

*I didn't touch the staking pallet since this is a bit more work and it wasn't needed for the pallet I was working on.

Checklist:

  • I have verified that the code works
    • No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

@weichweich weichweich self-assigned this Apr 5, 2022
@weichweich weichweich requested review from wischli, ntn-x2 and trusch and removed request for wischli, ntn-x2 and trusch April 5, 2022 10:52
weichweich
weichweich commented Apr 5, 2022
View reviewed changes
pallets/did/src/did_details.rs
@@ -177,7 +177,7 @@ pub trait DidVerifiableIdentifier {
) -> Result<DidVerificationKey, SignatureError>;
}

impl DidVerifiableIdentifier for runtime_common::DidIdentifier {
impl<I: AsRef<[u8; 32]>> DidVerifiableIdentifier for I {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm up for better solution. This was the first thing that came to my mind. But it's definitely not the best solution since not every 32 Byte array is a DidVerifiableIdentifier. 🤔 On the other hand why not? You could try it on anything that is somehow a 32 Byte array.

Copy link
Member

Choose a reason for hiding this comment

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

Can't we use the same trait requirements that the pallet requires?

type DidIdentifier: Parameter + DidVerifiableIdentifier + MaxEncodedLen;

Or if we want to be even more general, just taking whatever is needed from the Parameter type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the implementation the requirement I: AsRef<[u8; 32]> should be enough. But I would like to limit it to the actual type that is the DidIdentifier and not just all things that can be referenced as a [u8; 32]. Which is also an AccountId. But I think there is no way around that. 🤷 Adding more constraints maybe reduce the number of types this is implemented for, but I'm not sure if it's worth it? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I would not spend too much on this, but if that's the only trait required right now in the method implementation and it's compiling, I would go on with that.

ntn-x2
ntn-x2 reviewed Apr 5, 2022
View reviewed changes
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

As mentioned, there's still a dependency in the lock file from the launch and the staking pallets towards the runtime-common crate. There are also few other comments, but the cleanup is very nice to have!

pallets/did/src/did_details.rs
@@ -177,7 +177,7 @@ pub trait DidVerifiableIdentifier {
) -> Result<DidVerificationKey, SignatureError>;
}

impl DidVerifiableIdentifier for runtime_common::DidIdentifier {
impl<I: AsRef<[u8; 32]>> DidVerifiableIdentifier for I {
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use the same trait requirements that the pallet requires?

type DidIdentifier: Parameter + DidVerifiableIdentifier + MaxEncodedLen;

Or if we want to be even more general, just taking whatever is needed from the Parameter type.

pallets/did/src/mock.rs Outdated Show resolved Hide resolved
pallets/did/src/mock.rs Show resolved Hide resolved
wischli
wischli approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

LGTM

runtimes/common/src/constants.rs Outdated Show resolved Hide resolved
@weichweich weichweich merged commit c724119 into develop Apr 6, 2022
@weichweich weichweich deleted the aw-enforce-no-runtime-in-pallet branch April 6, 2022 10:56
ntn-x2 pushed a commit that referenced this pull request Jun 23, 2022
@weichweich @ntn-x2
refactor: enforce no runtime in pallet (#349)
f9738f8 
* refactor: clean inflation pallet of runtime code

* refactor: clean pallet did lookup of runtime code

* refactor: move parameter to common

* refactor: clean pallet did of runtime references

* refactor: clean kilt launch of runtime code

* refactor: clean parachain-staking of runtime code

(cherry picked from commit c724119)
ntn-x2 added a commit that referenced this pull request Jun 24, 2022
@ntn-x2
feat: release 1.7.0 (#373)
aa39f8d 
* Adds two more relaychain bootnodes for staging environment  (#334)

* chore: reset peregrine stg (#335)

* ci: use custom ci image (#336)

* Optimizes docker layer (#337)

* fix: add did lookup pallet to DID authorization logic + reverse lookup index (#343)

* chore: update toolchain version to nightly 1.59 (#339)

* feat: proxy type for disableling deposit claiming (#341)

* fix: rococo protocol id (#369)

* feat: generic access control (#316)

* Updates toolchain version (#345)

* refactor: enforce no runtime in pallet (#349)

* fix: features (#353)

* feat: add tips pallet (#352)

* feat: upgrade to Polkadot v0.9.19 (#357)

* chore: upgrade and clean up (#360)

* Adds the new rococo chainspec (#363)

* feat: add launch pallet removal migration (#359)

* refactor: update rilt para id from 2015 to 2108 (#364)

* fix: rilt para id (#365)

* feat: upgrade to Polkadot v0.9.23 (#366)

* use ci-linx:production base image (#368)

* feat: upgrade to Polkadot v0.9.24 (#370)

* fix: fix CI builders compilation errors and pin to a specific hash (#372)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntn-x2 ntn-x2 ntn-x2 left review comments

@wischli wischli wischli approved these changes

@trusch trusch Awaiting requested review from trusch

Assignees

@weichweich weichweich

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@weichweich @ntn-x2 @wischli