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

Remove crowdloan pallets #1834

Merged
merged 2 commits into from
May 13, 2024
Merged

Remove crowdloan pallets #1834

merged 2 commits into from
May 13, 2024

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented May 10, 2024

Description

Remove:

  • pallet-croddloan-claim
  • pallet-crowdloan-reward
  • libs/proofs (used only by them)
  • Unused trait
  • Storage from altair and centrifuge (Tech-debt)

@lemunozm lemunozm added P5-soon Issue should be addressed soon. I11-cleaning No mandatory issue that leave the repo more readable/organized labels May 10, 2024
@lemunozm lemunozm self-assigned this May 10, 2024
@lemunozm
Copy link
Contributor Author

I think we also need to create a migration to remove any storage from them

@wischli
Copy link
Contributor

wischli commented May 10, 2024

I think we also need to create a migration to remove any storage from them

Yes, we can use the runtime_common::migrations::KillPallet for that!

Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.47%. Comparing base (23d432c) to head (3683b6b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1834      +/-   ##
==========================================
- Coverage   48.54%   48.47%   -0.08%     
==========================================
  Files         169      165       -4     
  Lines       13370    12961     -409     
==========================================
- Hits         6491     6283     -208     
+ Misses       6879     6678     -201     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lemunozm
Copy link
Contributor Author

lemunozm commented May 10, 2024

We have an issue removing the storage:

[2024-05-10T12:11:20Z INFO  runtime_common::migrations::nuke] Clear pallet "CrowdloanClaim": iteration result. backend: 10349 unique: 10349 loops: 10349

It's 259% heavier than a block, so we need to split this, which is not trivial I guess. Maybe we can leave the migration for another PR and start cleaning the repo now for the upgrade

@lemunozm
Copy link
Contributor Author

@gpmayorga the PR still appears as failed when it shouldn't 🤔. Something missing?

@mustermeiszer
Copy link
Collaborator

mustermeiszer commented May 13, 2024

It's 259% heavier than a block, so we need to split this, which is not trivial I guess. Maybe we can leave the migration for another PR and start cleaning the repo now for the upgrade

Multiblock migrations are maybe already available: paritytech/polkadot-sdk#1554

@wischli
Copy link
Contributor

wischli commented May 13, 2024

It's 259% heavier than a block, so we need to split this, which is not trivial I guess. Maybe we can leave the migration for another PR and start cleaning the repo now for the upgrade

Multiblock migrations are maybe already available: paritytech/polkadot-sdk#1554

Unfortunately, multiblock migrations are part of SDK v1.9.0.

IMO, it's absolutely feasible to keep the dead storage until then because it doesn't impose any risk. It just needs to be cleaned at some point.

@lemunozm
Copy link
Contributor Author

Then, we can merge this!

Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

I created a follow-up task: #1836

@lemunozm
Copy link
Contributor Author

I already did it, but I close mine and use yours 👍🏻

@lemunozm lemunozm merged commit 5adac81 into main May 13, 2024
11 of 12 checks passed
@lemunozm lemunozm deleted the clean/crowdloan branch May 13, 2024 09:04
@wischli wischli added this to the Centrifuge 1029 milestone Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I11-cleaning No mandatory issue that leave the repo more readable/organized P5-soon Issue should be addressed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants