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

feat: add utility pallet #131

Merged
merged 3 commits into from
Sep 5, 2022

Conversation

cuteolaf
Copy link
Member

No description provided.

@cuteolaf cuteolaf changed the title feat: add utility paleet feat: add utility pallet Aug 31, 2022
@cuteolaf cuteolaf force-pushed the issue-110-add-utility-pallet branch from 17a660b to 6daa8a9 Compare August 31, 2022 12:28
@cuteolaf cuteolaf mentioned this pull request Aug 31, 2022
@ilhanu ilhanu requested a review from ndkazu August 31, 2022 15:52
@@ -141,4 +147,4 @@ try-runtime = [
"pallet-onboarding/try-runtime",
Copy link
Contributor

@ndkazu ndkazu Sep 1, 2022

Choose a reason for hiding this comment

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

Is there a reason for not adding "pallet-utility/try-runtime" here?
and there is also the missing "pallet-utility/runtime-benchmarks" in the runtime-benchmark section.
Usually I refer to the following Files/folder to add pallets:
https://github.com/paritytech/substrate/blob/polkadot-v0.9.26/bin/node/runtime/Cargo.toml

I also realise that this rule (adding the pallet in try-runtime and runtime-benchmark) was not always respected in implementation of previous pallets. Let's learn & improve together!!

@@ -595,6 +602,7 @@ construct_runtime!(
Preimage: pallet_preimage,
Council: pallet_collective::<Instance1>,
Democracy: pallet_democracy,
Utility: pallet_utility,
// flag add pallet runtime
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here also, missing [pallet_utility, Utility] in the define_benchmark section.

@letodunc
Copy link
Contributor

letodunc commented Sep 1, 2022

To prevent missing things when adding a new pallet, there is a script that automatize it for you. It's add_new_pallet.sh in the scripts folder. You execute it from the root folder of the project. In your case, a use could be scripts/add_new_pallet.sh utility Utility. Everything will be correctly configured for the basic add in the project. Then you can customize it with what you need.

@ilhanu
Copy link
Member

ilhanu commented Sep 1, 2022

No sure why the coverage tests are failing, might have to do with the that it's an external PR

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2022

Codecov Report

Merging #131 (56526b1) into main (fb5a4e8) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main     #131   +/-   ##
=======================================
  Coverage   73.13%   73.13%           
=======================================
  Files          40       40           
  Lines        2922     2922           
=======================================
  Hits         2137     2137           
  Misses        785      785           
Impacted Files Coverage Δ
runtime/src/lib.rs 2.38% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ilhanu
Copy link
Member

ilhanu commented Sep 4, 2022

@cuteolaf can you please check the logs for the benchmarking and provide a fix.

@ndkazu
Copy link
Contributor

ndkazu commented Sep 5, 2022

A few things I do before sending a commit:

  1. cargo check to make sure that it compiles (you probably did it already).
  2. cargo test to be sure that all the tests are correctly configured and pass.
  3. cargo build --release --features runtime-benchmarks to be sure that all the benchmarks are correctly configured and pass.
    sometimes you will also have additionnal tests in your benchmarks (it is the case for some FairSquares pallets), in this case you also want to run the following:
  4. cargo test --features runtime-benchmarks

In my experience, If all the checks above pass on your local node, your commit will checkout without ANY problem.

@ilhanu ilhanu merged commit b99da68 into Fair-Squares:main Sep 5, 2022
@cuteolaf cuteolaf deleted the issue-110-add-utility-pallet branch September 10, 2022 15:06
cuteolaf added a commit to cuteolaf/fair-squares that referenced this pull request Oct 7, 2022
* feat: add utility pallet
* add: benchmark, try-runtime
* fix: error in macro
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.

5 participants