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

Better error for missing index in CRV2 #4643

Merged
merged 5 commits into from
Jun 2, 2024
Merged

Conversation

gupnik
Copy link
Contributor

@gupnik gupnik commented May 30, 2024

Fixes #4552

@gupnik gupnik added R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework. labels May 30, 2024
@gupnik gupnik requested a review from a team as a code owner May 30, 2024 05:21
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Great to have a test!

@gupnik
Copy link
Contributor Author

gupnik commented May 31, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented May 31, 2024

@gupnik https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6358853 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-9059039b-a554-42f1-8bdd-4a045a8d13a9 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented May 31, 2024

@gupnik Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6358853 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6358853/artifacts/download.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6358869

@@ -187,6 +187,13 @@ impl Def {
}
}

if pallet_index.is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if pallet_index.is_none() {
let Some(pallet_index) = pallet_index else {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pallet_index can be None for all items other than type aliases.

@@ -187,6 +187,13 @@ impl Def {
}
}

if pallet_index.is_none() {
if let syn::Item::Type(item) = item {
Copy link
Member

Choose a reason for hiding this comment

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

This if doesn't make sense? I mean the index can only be set when the item is a type, at least judging based on the code above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This if doesn't make sense? I mean the index can only be set when the item is a type, at least judging based on the code above.

So, the module marked with #[frame_support::runtime] can have arbitrary items. For example,

#[frame_support::runtime]
mod runtime {
    use super::*;
    
    pub type System = frame_system;
}

In this case, you only want this check for pub type System = frame_system; and not for use super::*;.

Basically, this change requires all type aliases within the runtime macro to be annotated with #[runtime::pallet_index] while the other items are preserved.

@bkchr bkchr enabled auto-merge June 2, 2024 18:34
@bkchr bkchr added this pull request to the merge queue Jun 2, 2024
Merged via the queue into master with commit f81751e Jun 2, 2024
153 of 156 checks passed
@bkchr bkchr deleted the gupnik/crv2-missing-index branch June 2, 2024 19:09
ordian added a commit that referenced this pull request Jun 4, 2024
* master: (106 commits)
  [ci] Delete unused flow (#4676)
  Fix umbrella CI check and fix the C&P message (#4670)
  Add Dockerfiles to the templates (#4637)
  Revamp the Readme of the minimal template (#4649)
  Add chain-spec-builder docker image (#4655)
  Update Amforc bootnodes for Kusama and Polkadot (#4668)
  make all storage items in parachain-system public (#4645)
  [Pools] Refactors and runtime apis for DelegateStake (#4537)
  update amforc westend and its parachain bootnodes (#4641)
  Better error for missing index in CRV2 (#4643)
  Implement `XcmPaymentApi` and `DryRunApi` on all system parachains (#4634)
  Use Unlicense for templates (#4628)
  collator-protocol: remove `elastic-scaling-experimental` feature (#4595)
  Update `runtime_type` ref doc with the new "Associated Type Bounds" (#4624)
  Adds ability to specify chain type in chain-spec-builder (#4542)
  Fix broken windows build (#4636)
  Beefy client generic on aduthority Id (#1816)
  pallet-staking: Put tests behind `cfg(debug_assertions)` (#4620)
  Broker new price adapter (#4521)
  Change `XcmDryRunApi::dry_run_extrinsic` to take a call instead (#4621)
  ...
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
Fixes paritytech#4552

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <info@kchr.de>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
Fixes paritytech#4552

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <info@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes 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.

CRV2: Missing pallet index causes misleading error message
5 participants