Remove deprecated treasury pallet calls

[chungquantin]

ISSUE

• Link to the issue: https://github.com/paritytech/polkadot-sdk/issues/3800

Deliverables

• [x] remove deprecated calls; (https://github.com/paritytech/polkadot-sdk/pull/3820/commits/d579b673672454d0dc7b5049e5cbbe6077da520b)
• [x] set explicit coded indexes for Error and Event enums, remove unused variants and keep the same indexes for the rest; (https://github.com/paritytech/polkadot-sdk/pull/3820/commits/d579b673672454d0dc7b5049e5cbbe6077da520b)
• [x] remove unused Config's type parameters; (https://github.com/paritytech/polkadot-sdk/pull/3820/commits/d579b673672454d0dc7b5049e5cbbe6077da520b)
• [x] remove irrelevant tests and adopt relevant using old api; (https://github.com/paritytech/polkadot-sdk/pull/3820/commits/d579b673672454d0dc7b5049e5cbbe6077da520b)
• [x] remove benchmarks for removed calls; (https://github.com/paritytech/polkadot-sdk/pull/3820/commits/1a3d5f1f96756555ddebd1b898c03464ffffdb25)
• [x] prdoc (https://github.com/paritytech/polkadot-sdk/pull/3820/commits/d579b673672454d0dc7b5049e5cbbe6077da520b)
• [x] remove deprecated methods from the treasury/README.md and add up-to-date dispatchable functions documentation (https://github.com/paritytech/polkadot-sdk/pull/3820/commits/d579b673672454d0dc7b5049e5cbbe6077da520b)
• [x] remove deprecated weight functions (https://github.com/paritytech/polkadot-sdk/pull/3820/commits/8f74134b82df9a6df2824bbbe1555667223f1a83)

Separated to other issues

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

Adjust all treasury pallet instances within polkadot-sdk

• [ ] pallet_bounty, tip, child_bounties: https://github.com/openguild-labs/polkadot-sdk/pull/3
• [ ] Remove deprecated treasury weight functions used in Westend and Rococo runtime collective-westend, collective-rococo

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

• [ ] https://github.com/paritytech/polkadot-sdk/pull/3828

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

[muharem]

migration type for storage removal https://github.com/paritytech/polkadot-sdk/pull/3828

[muharem]

@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 https://github.com/paritytech/polkadot-sdk/pull/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]

@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]

@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]

@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: https://github.com/openguild-labs/polkadot-sdk/pull/1

I have a few concerns:

• As the pallet_bounty low-level implementation still follow the logic of Propose -> Approve, should it follow the new logic declared in the pallet_treasury::spend_local. If yes, new issue can be created to deprecate the treasury pallet as well
• My other concerns are in here: https://github.com/openguild-labs/polkadot-sdk/pull/1/files/e14c706d9520c65ce60037fc727ac5de89a75f9f

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

• Link to issue: https://github.com/openguild-labs/polkadot-sdk/pull/2

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

[muharem]

@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 https://github.com/paritytech/polkadot-sdk/pull/3890, we should basically have those changes in this PR, otherwise you never get CIs green. Same for bounties pallets.

[muharem]

@chungquantin are you still willing to finish this?

[chungquantin]

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

[chungquantin]

@muharem Could you help me to double check this sub-PR if it is good to merge into this PR? https://github.com/openguild-labs/polkadot-sdk/pull/3

I made changes to bounties, child-bounties and tip without removing core logic but only changes in the type parameters. Because those pallets are marked as tightly coupled to the pallet_treasury, I need to update the test cases of these pallets with the new version of the pallet treasury as well.

[muharem]

@chungquantin it introduces breaking changes to more pallets, but makes it a bit cleaner. what would you prefer as a user? if we break, we need a guide for the upgrade in pr doc. if we keep, we need to update docs for those type to reason them for a user.

[chungquantin]

@muharem From the perspective of a user, I think they need to redefine the extra type parameters for child_bounties, tip and bounty if these pallets are used in their parachain which creates duplicate types. I am considering adding the type parameters back to the treasury pallet and only remove deprecated dispatchable functions. What do you think?

[muharem]

@chungquantin I would only remove types from treasury config that are not used anymore in the pallet and it's dependancies. and I wont change anything for dependancies. So we will break the api for treasury pallet only. Plus I would update the docs for those type in treasury config, that are only used by it's extensions.

[chungquantin]

@muharem Get it. So based on your suggestion, I will add back the ApproveOrigin and OnSlash type parameters as those are used by the dependencies. Maybe I should add deprecated flag to the type parameters?

[muharem]

@chungquantin are they deprecated? they still used by the extensions. we can plan some refactoring (if you think we need it) for these pallets. but it should be more comprehensive, with a vision for a final solution and goals, we would need to open an issue, discuss the solution there and execute after. instead of extending the scope of this PR, and introducing some cosmetic changes in a cost of breaking APIs of more pallets. this is my personal opinion. please make all required CIs pass before requesting review.

[chungquantin]

@muharem yes those type parameters are no longer in the treasury pallet but are used in the child pallets like child bounties, bounties and tip.

[muharem]

@chungquantin should we finish this? we only need to fix all integrations for treasury pallet in this repo. CIs should be green.

[chungquantin]

@muharem Ok let me work on it.

[chungquantin]

@muharem I reverted the changes to type parameters because I think it is the cleanest approach to remove deprecated treasury calls at the moment. Failed tests are not related to this. Could you help me to double check if it is good to go?

[muharem]

@chungquantin you did revert the changes in the depended pallets, I agree with it. But you also reverted the changes for the treasury config. We need to remove treasure config parameters that are not used by treasury pallet and not used by it's dependent pallets.

[chungquantin]

@muharem I reverted because those type parameters are used by the dependent pallets.

[muharem]

@muharem I reverted because those type parameters are used by the dependent pallets.

not all of them, ProposalBondMaximum for example used only by treasury pallet.

[paritytech-cicd-pr]

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]

@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 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 - https://github.com/paritytech/polkadot-sdk/pull/3820#discussion_r1626138044 it will fix prdoc and semver CI. not sure what with check-ubmrella, but right prdoc might help

[muharem]

@chungquantin any updates?

[chungquantin]

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