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

migrate alliance, fast-unstake and bags list to use derive-impl #1636

Merged
merged 16 commits into from
Oct 1, 2023

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Sep 19, 2023

Moving a few pallets to the latest and greatest derive_impl to give it a try.

Part of #171

@kianenigma kianenigma requested review from a team September 19, 2023 15:17
@kianenigma kianenigma added the R0-silent Changes should not be mentioned in any release notes label Sep 19, 2023
@kianenigma kianenigma added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Sep 19, 2023
Co-authored-by: Adrian Catangiu <adrian@parity.io>
@kianenigma kianenigma requested a review from a team as a code owner September 20, 2023 13:41
@@ -877,6 +877,8 @@ pub fn inject_runtime_type(_: TokenStream, tokens: TokenStream) -> TokenStream {
if item.ident != "RuntimeCall" &&
item.ident != "RuntimeEvent" &&
item.ident != "RuntimeOrigin" &&
item.ident != "RuntimeHoldReason" &&
item.ident != "RuntimeFreezeReason" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to modify the error message as well.

type SS58Prefix = ();
type OnSetCode = ();
type MaxConsumers = frame_support::traits::ConstU32<16>;
// we use U128 account id in this pallet for a good reason.
Copy link
Contributor

@liamaharon liamaharon Sep 21, 2023

Choose a reason for hiding this comment

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

what is the 'good reason'? should we mention it in this comment?

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 had mixed this in nomination-pools, where we do use u128 such that the process of converting a pool id into account id would work and yield different numbers. I actually don't know the reason for this, and will try and move it to u32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to have something to do with iteration order -- with u128, if I insert 1 and 2 into a map and then iter on it, I get it back in the same order, but not with u32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the right behavior is actually to be agnostic to the order, as the pallet makes no guarantees there.

type AccountData = pallet_balances::AccountData<u64>;
// This pallet wishes to overwrite this.
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

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 am not aware tbh, but somehow in the test setu they wishes to on;y allow balances and remark transactions. Probably to test that any call filter here cannot be bipassed by being wrapped in a multisig.

@@ -877,6 +877,8 @@ pub fn inject_runtime_type(_: TokenStream, tokens: TokenStream) -> TokenStream {
if item.ident != "RuntimeCall" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love to see docs on these macros

Copy link
Contributor

Choose a reason for hiding this comment

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

Docs are available here:

pub use frame_support_procedural::inject_runtime_type;

Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Love inject_runtime_type ❤️

@paritytech-cicd-pr
Copy link

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

@@ -227,12 +227,17 @@ pub mod pallet {
impl DefaultConfig for TestDefaultConfig {
#[inject_runtime_type]
type RuntimeEvent = ();
#[inject_runtime_type]
type RuntimeHoldReason = ();
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, when we have #[inject_runtime_type] here, does it actually mean that the RHS here can be anything we want, and it wouldn't matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be any type (as long as it is a valid type identifier) and any type is valid because in DefaultConfig it is merely a type RuntimeHoldReason with no bounds.

@kianenigma kianenigma merged commit 2ed66a0 into master Oct 1, 2023
7 of 8 checks passed
@kianenigma kianenigma deleted the kiz-migrat-some-pallet-to-default-config branch October 1, 2023 14:16
ordian added a commit that referenced this pull request Oct 3, 2023
* master: (24 commits)
  Init System Parachain storage versions and add migration check jobs to CI (#1344)
  no-bound derives: Use absolute path for `core` (#1763)
  migrate alliance, fast-unstake and bags list to use derive-impl (#1636)
  Tvl pool staking (#1322)
  improve service error (#1734)
  frame-support: `RuntimeDebug\Eq\PartialEq` impls for `Imbalance` (#1717)
  Point documentation links to monorepo (#1741)
  [NPoS] Fix for Reward Deficit in the pool (#1255)
  Move import queue from `ChainSync` to `SyncingEngine` (#1736)
  Enable mocking contracts (#1331)
  Revert "fix(review-bot): pull secrets from `master` environment" (#1748)
  Remove kusama and polkadot runtime crates (#1731)
  Use `Extensions` to register offchain worker custom extensions (#1719)
  [RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format (#1666)
  fix(review-bot): pull secrets from `master` environment (#1745)
  Fix `subkey inspect` output text padding (#1744)
  archive: Implement height, hashByHeight and call (#1582)
  rpc/client: Propagate `rpc_methods` method to reported methods (#1713)
  rococo-runtime: `RococoGenesisExt` removed (#1490)
  PVF: more filesystem sandboxing (#1373)
  ...
ordian added a commit that referenced this pull request Oct 10, 2023
* tsv-disabling-node-side: (24 commits)
  Init System Parachain storage versions and add migration check jobs to CI (#1344)
  no-bound derives: Use absolute path for `core` (#1763)
  migrate alliance, fast-unstake and bags list to use derive-impl (#1636)
  Tvl pool staking (#1322)
  improve service error (#1734)
  frame-support: `RuntimeDebug\Eq\PartialEq` impls for `Imbalance` (#1717)
  Point documentation links to monorepo (#1741)
  [NPoS] Fix for Reward Deficit in the pool (#1255)
  Move import queue from `ChainSync` to `SyncingEngine` (#1736)
  Enable mocking contracts (#1331)
  Revert "fix(review-bot): pull secrets from `master` environment" (#1748)
  Remove kusama and polkadot runtime crates (#1731)
  Use `Extensions` to register offchain worker custom extensions (#1719)
  [RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format (#1666)
  fix(review-bot): pull secrets from `master` environment (#1745)
  Fix `subkey inspect` output text padding (#1744)
  archive: Implement height, hashByHeight and call (#1582)
  rpc/client: Propagate `rpc_methods` method to reported methods (#1713)
  rococo-runtime: `RococoGenesisExt` removed (#1490)
  PVF: more filesystem sandboxing (#1373)
  ...
github-merge-queue bot pushed a commit that referenced this pull request Feb 19, 2024
… frame_system (#3317)

# Description

This PR removes redundant type definition from test definition config
implementations like
```
#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
impl frame_system::Config for Test {
    type A = A;
    ...
}
```

This changes avoid redundancies in the code as the macro `derive_impl`
defines the relevant types. To implement the changes, it was a simple
fact of running tests and making sure that the tests would still run
while the definition would be removed.

Closes #3237

As a note, here is a brief account of things done from the Issue's
description statement
```
alliance migrate alliance, fast-unstake and bags list to use derive-impl #1636
asset-conversion                                                                                            DONE
asset-rate                                                                                                  DONE
assets                                                                                                      DONE
atomic-swap                                                                                                 DONE
aura                                                                                                        DONE
authority-discovery                                                                                         DONE                                                                     
authorship  migrate babe and authorship to use derive-impl #1790
babe  migrate babe and authorship to use derive-impl #1790
bags-list migrate alliance, fast-unstake and bags list to use derive-impl #1636
balances                                                                                                    DONE
beefy                                                                                                       NOTHING TO DO --- also noted this error without failing tests Feb 13 13:49:08.941 ERROR runtime::timestamp: `pallet_timestamp::UnixTime::now` is called at genesis, invalid value returned: 0
beefy-mmr                                                                                                   NOTHING TO DO
bounties                                                                                                    DONE
child-bounties                                                                                              DONE
collective                                                                                                  DONE
contracts                                                                                                   DONE
conviction-voting                                                                                           DONE
core-fellowship                                                                                             NOTHING TO DO
democracy                                                                                                   DONE
election-provider-multi-phase                                                                               NOTHING TO DO
elections-phragmen                                                                                          DONE
executive                                                                                                   NOTHING TO DO
fast-unstake migrate alliance, fast-unstake and bags list to use derive-impl #1636
glutton                                                                                                     DONE
grandpa                                                                                                     DONE
identity                                                                                                    DONE
im-online                                                                                                   NOTHING TO DO
indices Refactor indices pallet #1789
insecure-randomness-collective-flip                                                                         DONE
lottery                                                                                                     DONE
membership                                                                                                  DONE
merkle-mountain-range                                                                                       NOTHING TO DO
message-queue                                                                                               DONE
multisig add frame_system::DefaultConfig to individual pallet DefaultConfigs substrate#14453
nft-fractionalization                                                                                       DONE
nfts                                                                                                        DONE
nicks Refactor pallet-state-trie-migration to fungible::* traits #1801                                      NOT IN REPO
nis                                                                                                         DONE
node-authorization                                                                                          DONE
nomination-pools                                                                                            NOTHING TO DO -- ONLY impl for Runtime
offences                                                                                                    DELETED EVERYTHING -- IS THAT CORRECT??
preimage                                                                                                    DONE
proxy add frame_system::DefaultConfig to individual pallet DefaultConfigs substrate#14453
ranked-collective                                                                                           NOTHING TO DO
recovery                                                                                                    DONE
referenda                                                                                                   DONE
remark                                                                                                      DONE
root-offences                                                                                               DONE
root-testing                                                                                                NOTHING TO DO
salary                                                                                                      NOTHING TO DO
scheduler                                                                                                   DONE
scored-pool                                                                                                 DONE
session                                                                                                     DONE -- substrate/frame/session/benchmarking/src/mock.rs untouched
society                                                                                                     NOTHING TO DO
staking                                                                                                     DONE
staking-bags-benchmarks                                                                                     NOT IN REPO
state-trie-migration                                                                                        NOTHING TO DO
statement                                                                                                   DONE
sudo                                                                                                        DONE
system                                                                                                      DONE
timestamp                                                                                                   DONE
tips                                                                                                        DONE
transaction-payment                                                                                         NOTHING TO DO
transaction-storage                                                                                         NOTHING TO DO
treasury                                                                                                    DONE
try-runtime                                                                                                 NOTHING TO DO -- no specific mention of 'for Test'
uniques                                                                                                     DONE
utility                                                                                                     DONE
vesting                                                                                                     DONE
whitelist                                                                                                   DONE
```

---------

Co-authored-by: command-bot <>
Co-authored-by: gupnik <nikhilgupta.iitk@gmail.com>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…tytech#1636)

Moving a few pallets to the latest and greatest `derive_impl` to give it
a try.

Part of paritytech#171

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
… frame_system (paritytech#3317)

# Description

This PR removes redundant type definition from test definition config
implementations like
```
#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
impl frame_system::Config for Test {
    type A = A;
    ...
}
```

This changes avoid redundancies in the code as the macro `derive_impl`
defines the relevant types. To implement the changes, it was a simple
fact of running tests and making sure that the tests would still run
while the definition would be removed.

Closes paritytech#3237

As a note, here is a brief account of things done from the Issue's
description statement
```
alliance migrate alliance, fast-unstake and bags list to use derive-impl paritytech#1636
asset-conversion                                                                                            DONE
asset-rate                                                                                                  DONE
assets                                                                                                      DONE
atomic-swap                                                                                                 DONE
aura                                                                                                        DONE
authority-discovery                                                                                         DONE                                                                     
authorship  migrate babe and authorship to use derive-impl paritytech#1790
babe  migrate babe and authorship to use derive-impl paritytech#1790
bags-list migrate alliance, fast-unstake and bags list to use derive-impl paritytech#1636
balances                                                                                                    DONE
beefy                                                                                                       NOTHING TO DO --- also noted this error without failing tests Feb 13 13:49:08.941 ERROR runtime::timestamp: `pallet_timestamp::UnixTime::now` is called at genesis, invalid value returned: 0
beefy-mmr                                                                                                   NOTHING TO DO
bounties                                                                                                    DONE
child-bounties                                                                                              DONE
collective                                                                                                  DONE
contracts                                                                                                   DONE
conviction-voting                                                                                           DONE
core-fellowship                                                                                             NOTHING TO DO
democracy                                                                                                   DONE
election-provider-multi-phase                                                                               NOTHING TO DO
elections-phragmen                                                                                          DONE
executive                                                                                                   NOTHING TO DO
fast-unstake migrate alliance, fast-unstake and bags list to use derive-impl paritytech#1636
glutton                                                                                                     DONE
grandpa                                                                                                     DONE
identity                                                                                                    DONE
im-online                                                                                                   NOTHING TO DO
indices Refactor indices pallet paritytech#1789
insecure-randomness-collective-flip                                                                         DONE
lottery                                                                                                     DONE
membership                                                                                                  DONE
merkle-mountain-range                                                                                       NOTHING TO DO
message-queue                                                                                               DONE
multisig add frame_system::DefaultConfig to individual pallet DefaultConfigs substrate#14453
nft-fractionalization                                                                                       DONE
nfts                                                                                                        DONE
nicks Refactor pallet-state-trie-migration to fungible::* traits paritytech#1801                                      NOT IN REPO
nis                                                                                                         DONE
node-authorization                                                                                          DONE
nomination-pools                                                                                            NOTHING TO DO -- ONLY impl for Runtime
offences                                                                                                    DELETED EVERYTHING -- IS THAT CORRECT??
preimage                                                                                                    DONE
proxy add frame_system::DefaultConfig to individual pallet DefaultConfigs substrate#14453
ranked-collective                                                                                           NOTHING TO DO
recovery                                                                                                    DONE
referenda                                                                                                   DONE
remark                                                                                                      DONE
root-offences                                                                                               DONE
root-testing                                                                                                NOTHING TO DO
salary                                                                                                      NOTHING TO DO
scheduler                                                                                                   DONE
scored-pool                                                                                                 DONE
session                                                                                                     DONE -- substrate/frame/session/benchmarking/src/mock.rs untouched
society                                                                                                     NOTHING TO DO
staking                                                                                                     DONE
staking-bags-benchmarks                                                                                     NOT IN REPO
state-trie-migration                                                                                        NOTHING TO DO
statement                                                                                                   DONE
sudo                                                                                                        DONE
system                                                                                                      DONE
timestamp                                                                                                   DONE
tips                                                                                                        DONE
transaction-payment                                                                                         NOTHING TO DO
transaction-storage                                                                                         NOTHING TO DO
treasury                                                                                                    DONE
try-runtime                                                                                                 NOTHING TO DO -- no specific mention of 'for Test'
uniques                                                                                                     DONE
utility                                                                                                     DONE
vesting                                                                                                     DONE
whitelist                                                                                                   DONE
```

---------

Co-authored-by: command-bot <>
Co-authored-by: gupnik <nikhilgupta.iitk@gmail.com>
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
Status: Done
Development

Successfully merging this pull request may close these issues.