Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Pallets: Treasury deprecate propose_spend dispatchable #14538

Merged
merged 7 commits into from
Aug 10, 2023
Merged

Pallets: Treasury deprecate propose_spend dispatchable #14538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f18509b
treasury deprecate dispatchables
muharem Jul 8, 2023
4edf3a3
allow deprecated
muharem Jul 10, 2023
8a2dab8
allow deprecated for benchmarks
muharem Jul 11, 2023
80647f7
allow deprecated in tests
muharem Aug 1, 2023
2d45c1b
allow deprecated for bounties tests
muharem Aug 8, 2023
25cc143
Merge remote-tracking branch 'origin/master' into muharem-treasury-de…
muharem Aug 8, 2023
703213d
deprecation month
muharem Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 108 additions & 27 deletions frame/bounties/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,10 @@ fn minting_works() {
#[test]
fn spend_proposal_takes_min_deposit() {
new_test_ext().execute_with(|| {
assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), 1, 3));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), 1, 3)
});
assert_eq!(Balances::free_balance(0), 99);
assert_eq!(Balances::reserved_balance(0), 1);
});
Expand All @@ -237,7 +240,10 @@ fn spend_proposal_takes_min_deposit() {
#[test]
fn spend_proposal_takes_proportional_deposit() {
new_test_ext().execute_with(|| {
assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3)
});
assert_eq!(Balances::free_balance(0), 95);
assert_eq!(Balances::reserved_balance(0), 5);
});
Expand All @@ -247,7 +253,10 @@ fn spend_proposal_takes_proportional_deposit() {
fn spend_proposal_fails_when_proposer_poor() {
new_test_ext().execute_with(|| {
assert_noop!(
Treasury::propose_spend(RuntimeOrigin::signed(2), 100, 3),
{
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(2), 100, 3)
},
TreasuryError::InsufficientProposersBalance,
);
});
Expand All @@ -258,8 +267,14 @@ fn accepted_spend_proposal_ignored_outside_spend_period() {
new_test_ext().execute_with(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);

assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3));
assert_ok!(Treasury::approve_proposal(RuntimeOrigin::root(), 0));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3)
});
assert_ok!({
#[allow(deprecated)]
Treasury::approve_proposal(RuntimeOrigin::root(), 0)
});

<Treasury as OnInitialize<u64>>::on_initialize(1);
assert_eq!(Balances::free_balance(3), 0);
Expand All @@ -285,8 +300,14 @@ fn rejected_spend_proposal_ignored_on_spend_period() {
new_test_ext().execute_with(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);

assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3));
assert_ok!(Treasury::reject_proposal(RuntimeOrigin::root(), 0));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3)
});
assert_ok!({
#[allow(deprecated)]
Treasury::reject_proposal(RuntimeOrigin::root(), 0)
});

<Treasury as OnInitialize<u64>>::on_initialize(2);
assert_eq!(Balances::free_balance(3), 0);
Expand All @@ -299,10 +320,19 @@ fn reject_already_rejected_spend_proposal_fails() {
new_test_ext().execute_with(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);

assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3));
assert_ok!(Treasury::reject_proposal(RuntimeOrigin::root(), 0));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3)
});
assert_ok!({
#[allow(deprecated)]
Treasury::reject_proposal(RuntimeOrigin::root(), 0)
});
assert_noop!(
Treasury::reject_proposal(RuntimeOrigin::root(), 0),
{
#[allow(deprecated)]
Treasury::reject_proposal(RuntimeOrigin::root(), 0)
},
TreasuryError::InvalidIndex
);
});
Expand All @@ -312,7 +342,10 @@ fn reject_already_rejected_spend_proposal_fails() {
fn reject_non_existent_spend_proposal_fails() {
new_test_ext().execute_with(|| {
assert_noop!(
Treasury::reject_proposal(RuntimeOrigin::root(), 0),
{
#[allow(deprecated)]
Treasury::reject_proposal(RuntimeOrigin::root(), 0)
},
pallet_treasury::Error::<Test>::InvalidIndex
);
});
Expand All @@ -322,7 +355,10 @@ fn reject_non_existent_spend_proposal_fails() {
fn accept_non_existent_spend_proposal_fails() {
new_test_ext().execute_with(|| {
assert_noop!(
Treasury::approve_proposal(RuntimeOrigin::root(), 0),
{
#[allow(deprecated)]
Treasury::approve_proposal(RuntimeOrigin::root(), 0)
},
TreasuryError::InvalidIndex
);
});
Expand All @@ -333,10 +369,19 @@ fn accept_already_rejected_spend_proposal_fails() {
new_test_ext().execute_with(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);

assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3));
assert_ok!(Treasury::reject_proposal(RuntimeOrigin::root(), 0));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3)
});
assert_ok!({
#[allow(deprecated)]
Treasury::reject_proposal(RuntimeOrigin::root(), 0)
});
assert_noop!(
Treasury::approve_proposal(RuntimeOrigin::root(), 0),
{
#[allow(deprecated)]
Treasury::approve_proposal(RuntimeOrigin::root(), 0)
},
TreasuryError::InvalidIndex
);
});
Expand All @@ -348,8 +393,14 @@ fn accepted_spend_proposal_enacted_on_spend_period() {
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);

assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3));
assert_ok!(Treasury::approve_proposal(RuntimeOrigin::root(), 0));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), 100, 3)
});
assert_ok!({
#[allow(deprecated)]
Treasury::approve_proposal(RuntimeOrigin::root(), 0)
});

<Treasury as OnInitialize<u64>>::on_initialize(2);
assert_eq!(Balances::free_balance(3), 100);
Expand All @@ -363,8 +414,14 @@ fn pot_underflow_should_not_diminish() {
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);

assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), 150, 3));
assert_ok!(Treasury::approve_proposal(RuntimeOrigin::root(), 0));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), 150, 3)
});
assert_ok!({
#[allow(deprecated)]
Treasury::approve_proposal(RuntimeOrigin::root(), 0)
});

<Treasury as OnInitialize<u64>>::on_initialize(2);
assert_eq!(Treasury::pot(), 100); // Pot hasn't changed
Expand All @@ -385,14 +442,26 @@ fn treasury_account_doesnt_get_deleted() {
assert_eq!(Treasury::pot(), 100);
let treasury_balance = Balances::free_balance(&Treasury::account_id());

assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), treasury_balance, 3));
assert_ok!(Treasury::approve_proposal(RuntimeOrigin::root(), 0));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), treasury_balance, 3)
});
assert_ok!({
#[allow(deprecated)]
Treasury::approve_proposal(RuntimeOrigin::root(), 0)
});

<Treasury as OnInitialize<u64>>::on_initialize(2);
assert_eq!(Treasury::pot(), 100); // Pot hasn't changed

assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), Treasury::pot(), 3));
assert_ok!(Treasury::approve_proposal(RuntimeOrigin::root(), 1));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), Treasury::pot(), 3)
});
assert_ok!({
#[allow(deprecated)]
Treasury::approve_proposal(RuntimeOrigin::root(), 1)
});

<Treasury as OnInitialize<u64>>::on_initialize(4);
assert_eq!(Treasury::pot(), 0); // Pot is emptied
Expand All @@ -415,10 +484,22 @@ fn inexistent_account_works() {
assert_eq!(Balances::free_balance(Treasury::account_id()), 0); // Account does not exist
assert_eq!(Treasury::pot(), 0); // Pot is empty

assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), 99, 3));
assert_ok!(Treasury::approve_proposal(RuntimeOrigin::root(), 0));
assert_ok!(Treasury::propose_spend(RuntimeOrigin::signed(0), 1, 3));
assert_ok!(Treasury::approve_proposal(RuntimeOrigin::root(), 1));
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), 99, 3)
});
assert_ok!({
#[allow(deprecated)]
Treasury::approve_proposal(RuntimeOrigin::root(), 0)
});
assert_ok!({
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(0), 1, 3)
});
assert_ok!({
#[allow(deprecated)]
Treasury::approve_proposal(RuntimeOrigin::root(), 1)
});
<Treasury as OnInitialize<u64>>::on_initialize(2);
assert_eq!(Treasury::pot(), 0); // Pot hasn't changed
assert_eq!(Balances::free_balance(3), 0); // Balance of `3` hasn't changed
Expand Down
6 changes: 6 additions & 0 deletions frame/treasury/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,10 @@ fn setup_proposal<T: Config<I>, I: 'static>(
fn create_approved_proposals<T: Config<I>, I: 'static>(n: u32) -> Result<(), &'static str> {
for i in 0..n {
let (caller, value, lookup) = setup_proposal::<T, I>(i);
#[allow(deprecated)]
Treasury::<T, I>::propose_spend(RawOrigin::Signed(caller).into(), value, lookup)?;
let proposal_id = <ProposalCount<T, I>>::get() - 1;
#[allow(deprecated)]
Treasury::<T, I>::approve_proposal(RawOrigin::Root.into(), proposal_id)?;
}
ensure!(<Approvals<T, I>>::get().len() == n as usize, "Not all approved");
Expand Down Expand Up @@ -93,6 +95,7 @@ benchmarks_instance_pallet! {

reject_proposal {
let (caller, value, beneficiary_lookup) = setup_proposal::<T, _>(SEED);
#[allow(deprecated)]
Treasury::<T, _>::propose_spend(
RawOrigin::Signed(caller).into(),
value,
Expand All @@ -107,6 +110,7 @@ benchmarks_instance_pallet! {
let p in 0 .. T::MaxApprovals::get() - 1;
create_approved_proposals::<T, _>(p)?;
let (caller, value, beneficiary_lookup) = setup_proposal::<T, _>(SEED);
#[allow(deprecated)]
Treasury::<T, _>::propose_spend(
RawOrigin::Signed(caller).into(),
value,
Expand All @@ -119,12 +123,14 @@ benchmarks_instance_pallet! {

remove_approval {
let (caller, value, beneficiary_lookup) = setup_proposal::<T, _>(SEED);
#[allow(deprecated)]
Treasury::<T, _>::propose_spend(
RawOrigin::Signed(caller).into(),
value,
beneficiary_lookup
)?;
let proposal_id = Treasury::<T, _>::proposal_count() - 1;
#[allow(deprecated)]
Treasury::<T, I>::approve_proposal(RawOrigin::Root.into(), proposal_id)?;
let reject_origin =
T::RejectOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Expand Down
12 changes: 12 additions & 0 deletions frame/treasury/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,10 @@ pub mod pallet {
/// - O(1)
#[pallet::call_index(0)]
#[pallet::weight(T::WeightInfo::propose_spend())]
#[allow(deprecated)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wonder if developers will notice these deprecations, may be there is a better way to deprecate dispatchables?
this at least will be included in release notes

Copy link
Member

Choose a reason for hiding this comment

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

You mean the pr will be included? Otherwise no one really will see this from the upper layers.

Copy link
Member

Choose a reason for hiding this comment

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

The metadata does not carry deprecation info, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bkchr yes, which is a good part.
the combination of #[allow(deprecated)] and #[deprecated(...)] for one item looks hacky.
may be whitelisting those warnings could be a solution.

@ggwpez just checked, it does not. you can see it in rococo contracts, the contracts pallet has deprecated calls.
but it does include docs, so we could include it into docs.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/paritytech/substrate/issues/13976#issuecomment-1570081315

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a deprecation issue for it - https://github.com/paritytech/substrate/issues/14732
Can you please review the PR
I did not add anything to metadata since there is no agreement on it yet.

#[deprecated(
note = "`propose_spend` will be removed in February 2024. Use `spend` instead."
)]
pub fn propose_spend(
origin: OriginFor<T>,
#[pallet::compact] value: BalanceOf<T, I>,
Expand Down Expand Up @@ -364,6 +368,10 @@ pub mod pallet {
/// - O(1)
#[pallet::call_index(1)]
#[pallet::weight((T::WeightInfo::reject_proposal(), DispatchClass::Operational))]
#[allow(deprecated)]
#[deprecated(
note = "`reject_proposal` will be removed in February 2024. Use `spend` instead."
)]
pub fn reject_proposal(
Copy link
Member

Choose a reason for hiding this comment

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

You could rename them to deprecated_reject_proposal. Then the info will be in the metadata and visible to downstream devs/users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it can be a solution actually, but I would first agree on it and have it as a standard to not create multiple ways of deprecating

origin: OriginFor<T>,
#[pallet::compact] proposal_id: ProposalIndex,
Expand Down Expand Up @@ -392,6 +400,10 @@ pub mod pallet {
/// - O(1).
#[pallet::call_index(2)]
#[pallet::weight((T::WeightInfo::approve_proposal(T::MaxApprovals::get()), DispatchClass::Operational))]
#[allow(deprecated)]
#[deprecated(
note = "`approve_proposal` will be removed in February 2024. Use `spend` instead."
)]
pub fn approve_proposal(
origin: OriginFor<T>,
#[pallet::compact] proposal_id: ProposalIndex,
Expand Down
Loading