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 deprecated treasury pallet calls #3820

Conversation

chungquantin
Copy link
Contributor

@chungquantin chungquantin commented Mar 25, 2024

ISSUE

Deliverables

  • remove deprecated calls; (d579b67)
  • set explicit coded indexes for Error and Event enums, remove unused variants and keep the same indexes for the rest; (d579b67)
  • remove unused Config's type parameters; (d579b67)
  • remove irrelevant tests and adopt relevant using old api; (d579b67)
  • remove benchmarks for removed calls; (1a3d5f1)
  • prdoc (d579b67)
  • remove deprecated methods from the treasury/README.md and add up-to-date dispatchable functions documentation (d579b67)
  • remove deprecated weight functions (8f74134)

Separated to other issues

  • remove storage items like Proposals and ProposalCount, that are not used anymore

Adjust all treasury pallet instances within polkadot-sdk

Add migration for westend and rococo to clean the data from removed storage items

Test Outcomes

Successful tests by running cargo test --features runtime-benchmarks

running 38 tests
test tests::__construct_runtime_integrity_test::runtime_integrity_tests ... ok
test benchmarking::benchmarks::bench_check_status ... ok
test benchmarking::benchmarks::bench_payout ... ok
test benchmarking::benchmarks::bench_spend_local ... ok
test tests::accepted_spend_proposal_enacted_on_spend_period ... ok
test benchmarking::benchmarks::bench_spend ... ok
test tests::accepted_spend_proposal_ignored_outside_spend_period ... ok
test benchmarking::benchmarks::bench_void_spend ... ok
test benchmarking::benchmarks::bench_remove_approval ... ok
test tests::genesis_funding_works ... ok
test tests::genesis_config_works ... ok
test tests::inexistent_account_works ... ok
test tests::minting_works ... ok
test tests::check_status_works ... ok
test tests::payout_retry_works ... ok
test tests::pot_underflow_should_not_diminish ... ok
test tests::remove_already_removed_approval_fails ... ok
test tests::spend_local_origin_permissioning_works ... ok
test tests::spend_valid_from_works ... ok
test tests::spend_expires ... ok
test tests::spend_works ... ok
test tests::test_genesis_config_builds ... ok
test tests::spend_payout_works ... ok
test tests::spend_local_origin_works ... ok
test tests::spend_origin_works ... ok
test tests::spending_local_in_batch_respects_max_total ... ok
test tests::spending_in_batch_respects_max_total ... ok
test tests::try_state_proposals_invariant_2_works ... ok
test tests::try_state_proposals_invariant_1_works ... ok
test tests::try_state_spends_invariant_2_works ... ok
test tests::try_state_spends_invariant_1_works ... ok
test tests::treasury_account_doesnt_get_deleted ... ok
test tests::try_state_spends_invariant_3_works ... ok
test tests::unused_pot_should_diminish ... ok
test tests::void_spend_works ... ok
test tests::try_state_proposals_invariant_3_works ... ok
test tests::max_approvals_limited ... ok
test benchmarking::benchmarks::bench_on_initialize_proposals ... ok

test result: ok. 38 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.08s

   Doc-tests pallet_treasury

running 2 tests
test substrate/frame/treasury/src/lib.rs - (line 52) ... ignored
test substrate/frame/treasury/src/lib.rs - (line 79) ... ignored

test result: ok. 0 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 0.00s

polkadot address: 19nSqFQorfF2HxD3oBzWM3oCh4SaCRKWt1yvmgaPYGCo71J

@chungquantin chungquantin requested a review from a team as a code owner March 25, 2024 09:18
Copy link
Contributor

@muharem muharem left a comment

Choose a reason for hiding this comment

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

we also need to remove storage items and update treasury pallet setups for the runtimes within this repo. for those runtimes we need to include storage migration to clean up the data for those removed storage items. there is probably some general migration type that allows to remove the whole storage item, I need to check this.
but if we do not have it, I can create an issue for it and we can work on it.

substrate/frame/treasury/src/lib.rs Show resolved Hide resolved
substrate/frame/treasury/src/tests.rs Outdated Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team March 25, 2024 10:59
@muharem
Copy link
Contributor

muharem commented Mar 25, 2024

migration type for storage removal #3828

@muharem
Copy link
Contributor

muharem commented Mar 26, 2024

@chungquantin I think what is left

  • remove storage items like Proposals and ProposalCount, that are not used anymore
  • adjust all treasury pallet instances within polkadot-sdk
  • add migration for westend and rococo to clean the data from removed storage items (I am about to merge this PR [FRAME] Remove storage migration type #3828). We can even check which removed storage items for westend and rococo has any data (the proposal count should be there)

also please make sure we do not break anything for dependant from this pallet the bounties and child bounties.

@chungquantin
Copy link
Contributor Author

@muharem For Proposals and ProposalCount storage items, the current implementation of the spend_local and spend_fund method rely heavily on those item. Would you mean we do a refractor on those method to remove those storage items completely?

@muharem
Copy link
Contributor

muharem commented Mar 26, 2024

@muharem For Proposals and ProposalCount storage items, the current implementation of the spend_local and spend_fund method rely heavily on those item. Would you mean we do a refractor on those method to remove those storage items completely?

missed that. we can do it, but that is not what we want to achieve with this PR. lets just focus on deprecation. so we keep those storage items.

@chungquantin
Copy link
Contributor Author

chungquantin commented Mar 27, 2024

@muharem

Update pallet_bounty deprecated calls

Thank you for your feedback. I have added this sub PR to update the deprecated use of Treasury pallet in the pallet_bounty: openguild-labs#1

I have a few concerns:

Update pallet_treasury relevant code in other runtimes Westend, Rococo and Collective

Could you please help me to review the sub PR if it is good to merge into this parent PR?

@muharem
Copy link
Contributor

muharem commented Apr 2, 2024

@chungquantin we should not change anything for child_bounties and bounties pallet. but only ensure that changes we introduce for treasury pallet do no break anything for those pallet. The OnSlash type for example, should not be removed, or the same type should be introduced for bounties and maybe child-bounties pallets.

I've seen #3890, we should basically have those changes in this PR, otherwise you never get CIs green. Same for bounties pallets.

prdoc/pr_3800.prdoc Outdated Show resolved Hide resolved
@muharem
Copy link
Contributor

muharem commented Apr 15, 2024

@chungquantin are you still willing to finish this?

@chungquantin
Copy link
Contributor Author

@muharem Yes I am working on it. Let me ship the code around tomorrow

@paritytech-review-bot paritytech-review-bot bot requested a review from a team April 15, 2024 12:48
@paritytech-cicd-pr
Copy link

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

@chungquantin
Copy link
Contributor Author

@muharem Thank you. I have managed to fix the code. I checked the CI tests and seems like the failed tests are not relevant to this PR. Could you help me to double check if this PR is good to go? Thanks.

@muharem muharem added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Jun 4, 2024
- Remove deprecated parameter types: ProposalBond, ProposalBondMaximum, ProposalBondMinimum

crates:
- name: pallet-treasury
Copy link
Contributor

Choose a reason for hiding this comment

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

we need all altered crates here listed with bump attribute. check more info here - https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/prdoc.md#record-semver-changes

@muharem
Copy link
Contributor

muharem commented Jun 4, 2024

@muharem Thank you. I have managed to fix the code. I checked the CI tests and seems like the failed tests are not relevant to this PR. Could you help me to double check if this PR is good to go? Thanks.

check this comment - #3820 (comment)
it will fix prdoc and semver CI. not sure what with check-ubmrella, but right prdoc might help

@muharem
Copy link
Contributor

muharem commented Jun 11, 2024

@chungquantin any updates?

@paritytech-review-bot paritytech-review-bot bot requested a review from a team June 14, 2024 02:38
@chungquantin
Copy link
Contributor Author

@muharem It is good to go now. Could you please check?

Copy link
Contributor

@muharem muharem left a comment

Choose a reason for hiding this comment

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

almost there

prdoc/pr_3820.prdoc Outdated Show resolved Hide resolved
substrate/frame/treasury/src/benchmarking.rs Outdated Show resolved Hide resolved
substrate/frame/treasury/src/benchmarking.rs Show resolved Hide resolved
substrate/frame/treasury/src/lib.rs Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team June 16, 2024 03:58
@paritytech-review-bot paritytech-review-bot bot requested a review from a team June 17, 2024 14:17
@muharem muharem added this pull request to the merge queue Jun 18, 2024
Merged via the queue into paritytech:master with commit 40677b6 Jun 18, 2024
158 of 160 checks passed
@chungquantin chungquantin deleted the chungquantin/remove_treasury_deprecated_calls branch June 18, 2024 15:49
github-merge-queue bot pushed a commit that referenced this pull request Jun 26, 2024
Introduce migration type to remove data associated with a specific
storage of a pallet.

Based on existing `RemovePallet` migration type.

Required for #3820

---------

Co-authored-by: Liam Aharon <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants