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

require explicit pallet indexes #821

Merged
merged 53 commits into from
Sep 23, 2024
Merged

require explicit pallet indexes #821

merged 53 commits into from
Sep 23, 2024

Conversation

sam0x17
Copy link
Contributor

@sam0x17 sam0x17 commented Sep 18, 2024

fixes #787

also fixes CI bug that was causing custom lint check to not catch lint failures

Adds a custom lint that completely disallows invocations of construct_runtime! that do not use explicit indexes for all mentioned pallets.

This was extremely tricky to get right. After many attempts to re-implement construct_runtime! parsing from scratch, I decided to fork frame-support-procedural from upstream and modify it so that instead of a proc-macro crate, it exports all parsing logic so it can be used from other crates. This can be fully updated to whatever tag we want in polkadot-sdk using the provided update.sh script by modifying this line:

POLKADOT_SDK_TAG="v1.10.0-rc3"

If polkadot-sdk ever adds a public way of accessing the parsing logic for frame internals, we can remove this fork, however that is unlikely to happen any time soon. Maintaining this fork should be super easy since it can be updated with the script whenever we want, and the structure of that crate is likely never going to change (I used to maintain it).

Notes for Reviewers / TLDR:

  • the contents of procedural-fork OTHER THAN the lib.rs, update.sh and Cargo.toml are copied verbatim from polkadot-sdk 1.10., so those 3 files are all you need to review from that directory
  • otherwise all the relevant code changes are in support/linting/pallet_index.rs and build.rs

@sam0x17 sam0x17 self-assigned this Sep 18, 2024
@sam0x17 sam0x17 changed the base branch from main to devnet-ready September 18, 2024 22:06
@sam0x17 sam0x17 marked this pull request as draft September 18, 2024 22:29
orriin
orriin previously approved these changes Sep 19, 2024
Cargo.toml Outdated Show resolved Hide resolved
JohnReedV
JohnReedV previously approved these changes Sep 20, 2024
orriin
orriin previously approved these changes Sep 20, 2024
gztensor
gztensor previously approved these changes Sep 20, 2024
support/linting/src/pallet_index.rs Show resolved Hide resolved
@sam0x17 sam0x17 dismissed stale reviews from gztensor, orriin, and JohnReedV via c368ce3 September 23, 2024 18:16
@sam0x17
Copy link
Contributor Author

sam0x17 commented Sep 23, 2024

had to turn off cargo test for procedural-fork because the tests rely on a bunch of janky features to pass and no need for that, if it compiles we are good / we just care about parsing

@sam0x17 sam0x17 requested a review from a team September 23, 2024 19:32
@unconst unconst merged commit cdbac35 into devnet-ready Sep 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-spec-version-bump PR does not contain changes that requires bumping the spec version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lint check to disallow implicit pallet indexing
6 participants