Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[FRAME Core] remove unnecessary overrides while using derive_impl for frame_system #3237

Closed
7 of 71 tasks
gupnik opened this issue Feb 7, 2024 · 9 comments · Fixed by #3317
Closed
7 of 71 tasks

[FRAME Core] remove unnecessary overrides while using derive_impl for frame_system #3237

gupnik opened this issue Feb 7, 2024 · 9 comments · Fixed by #3317
Assignees
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T10-tests This PR/Issue is related to tests.

Comments

@gupnik
Copy link
Contributor

gupnik commented Feb 7, 2024

#2409 moved all test runtimes to use derive_impl for frame_system by simply adding an attribute to all impls like so:

+#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
impl frame_system::Config for Runtime {
..
}

This enabled the introduction of new features like RuntimeTask without the need to update the configs everywhere.

However, the existing impls still continue to override the defaults for a number of types that are essentially the same as the ones provided by default config.

#1790 provides a sample of how the overridden defaults could simply be removed. This issue tracks the same for all pallets:

Here's a tracking list:

@gupnik gupnik added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T10-tests This PR/Issue is related to tests. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. labels Feb 7, 2024
@Gilt0
Copy link
Contributor

Gilt0 commented Feb 11, 2024

Hi @gupnik ,
I would be curious to help on this issue, however, I have to admit that I am a complete and absolute beginner on this repo. So I may take more time to complete...
Would you be keen to help me fix the issues?
Best wishes,
Gilt0

@gupnik
Copy link
Contributor Author

gupnik commented Feb 11, 2024

Hi @gupnik , I would be curious to help on this issue, however, I have to admit that I am a complete and absolute beginner on this repo. So I may take more time to complete... Would you be keen to help me fix the issues? Best wishes, Gilt0

Sure, assigned it to you.

@Gilt0
Copy link
Contributor

Gilt0 commented Feb 11, 2024

Great thank you.

@Gilt0
Copy link
Contributor

Gilt0 commented Feb 14, 2024

hi @gupnik ,

I created pull request: Gilt0:issue-3237.

Also:

  • I have not figured out how to add the PR to the Security Audit (PRs) - SRLabs.
  • I did not squash so that you'd see all the steps (maybe naming of commits was a little short sighted - apologies).
  • for some reason the actions fail at Review Bot and I am not sure what I need to do to fix it.
    EDIT: it seems that the bot actually communicates with me and it has to do with clippy.

Best wishes,
Gilt0

@gupnik
Copy link
Contributor Author

gupnik commented Feb 14, 2024

hi @gupnik ,

I created pull request: Gilt0:issue-3237.

Also:

  • I have not figured out how to add the PR to the Security Audit (PRs) - SRLabs.
  • I did not squash so that you'd see all the steps (maybe naming of commits was a little short sighted - apologies).
  • for some reason the actions fail at Review Bot and I am not sure what I need to do to fix it.
    EDIT: it seems that the bot actually communicates with me and it has to do with clippy.

Best wishes, Gilt0

Great @Gilt0.

  1. I have added the labels.
  2. Works.
  3. CI points to unused imports as the reason. https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5209825
Screenshot 2024-02-14 at 2 27 28 PM

@Gilt0
Copy link
Contributor

Gilt0 commented Feb 14, 2024

Thank you for your reply @gupnik
Currently fixing the actions error (mainly unused imports that escaped my vigilance).

@Gilt0
Copy link
Contributor

Gilt0 commented Feb 14, 2024

Hey @gupnik

I am stalling a little. There is an error I don't understand. I tried reverting changes on identity pallet, but it did not do anything (shooting blind as I don't really understand what is going on).

Would you have any pointers on what could be happening?

[2024-02-14T22:59:57Z INFO  runtime::frame-support] 🩺 Running "Beefy" try-state checks
[2024-02-14T22:59:57Z INFO  runtime::frame-support] 🩺 Running "Mmr" try-state checks
[2024-02-14T22:59:57Z INFO  runtime::frame-support] 🩺 Running "BeefyMmrLeaf" try-state checks
[2024-02-14T22:59:57Z INFO  runtime::frame-support] 🩺 Running "IdentityMigrator" try-state checks
[2024-02-14T22:59:57Z ERROR runtime] panicked at /builds/parity/mirrors/polkadot-sdk/polkadot/runtime/westend/src/lib.rs:2227:65:
    called `Result::unwrap()` on an `Err` value: Other("Detected errors while executing try_state checks. See logs for more info.")
thread 'main' panicked at cli/main.rs:325:6:
called `Result::unwrap()` on an `Err` value: Input("failed to execute TryRuntime_on_runtime_upgrade: Execution aborted due to trap: wasm trap: wasm `unreachable` instruction executed\nWASM backtrace:\nerror while executing at wasm backtrace:\n    0: 0x73d60 - <unknown>!rust_begin_unwind\n    1: 0x11dbf - <unknown>!core::panicking::panic_fmt::he07f4fcfc0e8e78f\n    2: 0xfdb0 - <unknown>!core::result::unwrap_failed::h601a124147f65f04\n    3: 0x116ace - <unknown>!<westend_runtime::Runtime as frame_try_runtime::runtime_decl_for_try_runtime::TryRuntimeV1<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32,sp_runtime::traits::BlakeTwo256>,sp_runtime::generic::unchecked_extrinsic::UncheckedExtrinsic<sp_runtime::multiaddress::MultiAddress<<<sp_runtime::MultiSignature as sp_runtime::traits::Verify>::Signer as sp_runtime::traits::IdentifyAccount>::AccountId,()>,westend_runtime::RuntimeCall,sp_runtime::MultiSignature,(frame_system::extensions::check_non_zero_sender::CheckNonZeroSender<westend_runtime::Runtime>,frame_system::extensions::check_spec_version::CheckSpecVersion<westend_runtime::Runtime>,frame_system::extensions::check_tx_version::CheckTxVersion<westend_runtime::Runtime>,frame_system::extensions::check_genesis::CheckGenesis<westend_runtime::Runtime>,frame_system::extensions::check_mortality::CheckMortality<westend_runtime::Runtime>,frame_system::extensions::check_nonce::CheckNonce<westend_runtime::Runtime>,frame_system::extensions::check_weight::CheckWeight<westend_runtime::Runtime>,pallet_transaction_payment::ChargeTransactionPayment<westend_runtime::Runtime>)>>>>::on_runtime_upgrade::h3d77c7b947511727\n    4: 0x20ff92 - <unknown>!TryRuntime_on_runtime_upgrade")
stack backtrace:
   0: rust_begin_unwind
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:597:5
   1: core::panicking::panic_fmt
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:72:14
   2: core::result::unwrap_failed
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/result.rs:1652:5
   3: tokio::runtime::park::CachedParkThread::block_on
   4: try_runtime::main

@gupnik
Copy link
Contributor Author

gupnik commented Feb 15, 2024

Commented on the PR @Gilt0

@Gilt0
Copy link
Contributor

Gilt0 commented Feb 15, 2024

@gupnik Ah great thank you.

github-merge-queue bot pushed a commit that referenced this issue 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 issue 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>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T10-tests This PR/Issue is related to tests.
Projects
None yet
2 participants