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

Implement pallet_registrar hooks #325

Merged
merged 20 commits into from
Nov 27, 2023
Merged

Implement pallet_registrar hooks #325

merged 20 commits into from
Nov 27, 2023

Conversation

tmpolaczyk
Copy link
Contributor

@tmpolaczyk tmpolaczyk commented Nov 16, 2023

Allows to execute code on container chain register and deregister. This PR only includes the deregister hooks:

  • Clear Registrar storage: BootNodes and ParaGenesisData, and return parachain deposit (this used to be done immediately, now it will be done after 2 sessions)
  • Call AuthorNoting::kill_author_data
  • Call ServicesPayment::set_credits(0)

These hooks are executed 2 sessions after calling deregister, which is when the container chain won't have any collators anymore. Unless the para id was never marked as valid_for_collating, in that case the hooks are executed immediately.

Also, change the pausing mechanism. Add a new extrinsic to unpause: unpause_container_chain. Previously mark_valid_as_collating was used to unpause, now that won't work anymore. This change was needed because otherwise pausing and unpausing could trigger the implemented hooks when it should not, so now paused container chains are stored in a new storage item Paused instead of being in PendingVerification. Container chains that were paused before this PR will need to be unpaused using mark_valid_for_collating.

Alows to execute code on register and deregsiter

Also, change the pausing mechanism, now we have a separate `Paused`
storage item. This was needed because otherwise pausing and unpausing
could trigger hooks when it should not.
@tmpolaczyk tmpolaczyk added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes labels Nov 16, 2023
Copy link
Contributor

github-actions bot commented Nov 16, 2023

Coverage Report

(master)

@@                    Coverage Diff                     @@
##           master   tomasz-registrar-hooks      +/-   ##
==========================================================
+ Coverage   71.37%                   71.39%   +0.02%     
+ Files          86                       89       +3     
+ Lines       19709                    19791      +82     
==========================================================
+ Hits        14067                    14128      +61     
+ Misses       5642                     5663      +21     
Files Changed Coverage
/container-chains/templates/frontier/node/src/chain_spec.rs 71.74% (-3.02%) 🔽
/container-chains/templates/frontier/node/src/command.rs 20.59% (-0.12%) 🔽
/container-chains/templates/frontier/node/src/service.rs 76.77% (-1.18%) 🔽
/node/src/command.rs 15.51% (-0.06%) 🔽
/node/src/service.rs 17.72% (-14.28%) 🔽
/pallets/registrar/src/lib.rs 92.68% (-1.71%) 🔽
/runtime/dancebox/src/lib.rs 79.48% (-0.27%) 🔽

Coverage generated Fri Nov 24 17:37:58 UTC 2023

tools/benchmarking.sh Outdated Show resolved Hide resolved
@girazoki
Copy link
Collaborator

Ideally I would like to see tests that capture the following cases:

  • a parachain is pending to be a registered para and we deregister it. In this case we should not call the hook immediately
  • a prachain is pending to be paused and we deregister it. In this case, we should not call the hook immediately

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
#[cfg(feature = "std")]
fn integrity_test() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
fn integrity_test() {
fn try_state(_block_number: u32) {

The integrity_test hook doesn't have access to the storage, despite docs saying otherwise.

Copy link
Contributor

@fgamundi fgamundi left a comment

Choose a reason for hiding this comment

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

LGTM

@tmpolaczyk tmpolaczyk merged commit f990d4e into master Nov 27, 2023
24 checks passed
@tmpolaczyk tmpolaczyk deleted the tomasz-registrar-hooks branch November 27, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants