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.

Sign up for GitHub

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

Update relevant code to Treasury pallet in the pallet_bounty #1

Closed
wants to merge 1 commit into from
Closed

Update relevant code to Treasury pallet in the pallet_bounty #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 0 additions & 21 deletions substrate/frame/bounties/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,27 +171,6 @@ benchmarks_instance_pallet! {
ensure!(!T::Currency::free_balance(&beneficiary_account).is_zero(), "Beneficiary didn't get paid");
}

close_bounty_proposed {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This benchmark case relies on the parameter type ApproveOrigin which is deprecated in the pallet_treasury. Hence, I simply remove the benchmark case. I wonder if we need a workaround to cover this case or not.

setup_pot_account::<T, I>();
let (caller, curator, fee, value, reason) = setup_bounty::<T, I>(0, 0);
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin =
T::ApproveOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
}: close_bounty<T::RuntimeOrigin>(approve_origin, bounty_id)

close_bounty_active {
setup_pot_account::<T, I>();
let (curator_lookup, bounty_id) = create_bounty::<T, I>()?;
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin =
T::ApproveOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
}: close_bounty<T::RuntimeOrigin>(approve_origin, bounty_id)
verify {
assert_last_event::<T, I>(Event::BountyCanceled { index: bounty_id }.into())
}

extend_bounty_expiry {
setup_pot_account::<T, I>();
let (curator_lookup, bounty_id) = create_bounty::<T, I>()?;
Expand Down
6 changes: 2 additions & 4 deletions substrate/frame/bounties/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,8 +460,7 @@ pub mod pallet {

let slash_curator = |curator: &T::AccountId,
curator_deposit: &mut BalanceOf<T, I>| {
let imbalance = T::Currency::slash_reserved(curator, *curator_deposit).0;
T::OnSlash::on_unbalanced(imbalance);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pallet_bounty still rely on the old low-level implementation Propose > Approve so I wonder if removing this on_unbalanced when slash_reserved affects the existing use of the pallet.

T::Currency::slash_reserved(curator, *curator_deposit).0;
*curator_deposit = Zero::zero();
};

Expand Down Expand Up @@ -713,8 +712,7 @@ pub mod pallet {
// The reject origin would like to cancel a proposed bounty.
BountyDescriptions::<T, I>::remove(bounty_id);
let value = bounty.bond;
let imbalance = T::Currency::slash_reserved(&bounty.proposer, value).0;
T::OnSlash::on_unbalanced(imbalance);
T::Currency::slash_reserved(&bounty.proposer, value).0;
*maybe_bounty = None;

Self::deposit_event(Event::<T, I>::BountyRejected {
Expand Down
203 changes: 7 additions & 196 deletions substrate/frame/bounties/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ impl pallet_balances::Config for Test {
type RuntimeFreezeReason = ();
}
parameter_types! {
pub const ProposalBond: Permill = Permill::from_percent(5);
pub static Burn: Permill = Permill::from_percent(50);
pub const TreasuryPalletId: PalletId = PalletId(*b"py/trsry");
pub const TreasuryPalletId2: PalletId = PalletId(*b"py/trsr2");
Expand All @@ -95,13 +94,8 @@ parameter_types! {
impl pallet_treasury::Config for Test {
type PalletId = TreasuryPalletId;
type Currency = pallet_balances::Pallet<Test>;
type ApproveOrigin = frame_system::EnsureRoot<u128>;
type RejectOrigin = frame_system::EnsureRoot<u128>;
type RuntimeEvent = RuntimeEvent;
type OnSlash = ();
type ProposalBond = ProposalBond;
type ProposalBondMinimum = ConstU64<1>;
type ProposalBondMaximum = ();
type SpendPeriod = ConstU64<2>;
type Burn = Burn;
type BurnDestination = (); // Just gets burned.
Expand All @@ -122,13 +116,8 @@ impl pallet_treasury::Config for Test {
impl pallet_treasury::Config<Instance1> for Test {
type PalletId = TreasuryPalletId2;
type Currency = pallet_balances::Pallet<Test>;
type ApproveOrigin = frame_system::EnsureRoot<u128>;
type RejectOrigin = frame_system::EnsureRoot<u128>;
type RuntimeEvent = RuntimeEvent;
type OnSlash = ();
type ProposalBond = ProposalBond;
type ProposalBondMinimum = ConstU64<1>;
type ProposalBondMaximum = ();
type SpendPeriod = ConstU64<2>;
type Burn = Burn;
type BurnDestination = (); // Just gets burned.
Expand Down Expand Up @@ -227,56 +216,12 @@ fn minting_works() {
});
}

#[test]
fn spend_proposal_takes_min_deposit() {
new_test_ext().execute_with(|| {
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);
});
}

#[test]
fn spend_proposal_takes_proportional_deposit() {
new_test_ext().execute_with(|| {
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);
});
}

#[test]
fn spend_proposal_fails_when_proposer_poor() {
new_test_ext().execute_with(|| {
assert_noop!(
{
#[allow(deprecated)]
Treasury::propose_spend(RuntimeOrigin::signed(2), 100, 3)
},
TreasuryError::InsufficientProposersBalance,
);
});
}

#[test]
fn accepted_spend_proposal_ignored_outside_spend_period() {
new_test_ext().execute_with(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);

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

<Treasury as OnInitialize<u64>>::on_initialize(1);
assert_eq!(Balances::free_balance(3), 0);
Expand All @@ -297,112 +242,13 @@ fn unused_pot_should_diminish() {
});
}

#[test]
fn rejected_spend_proposal_ignored_on_spend_period() {
new_test_ext().execute_with(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);

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);
assert_eq!(Treasury::pot(), 50);
});
}

#[test]
fn reject_already_rejected_spend_proposal_fails() {
new_test_ext().execute_with(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);

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

#[test]
fn reject_non_existent_spend_proposal_fails() {
new_test_ext().execute_with(|| {
assert_noop!(
{
#[allow(deprecated)]
Treasury::reject_proposal(RuntimeOrigin::root(), 0)
},
pallet_treasury::Error::<Test>::InvalidIndex
);
});
}

#[test]
fn accept_non_existent_spend_proposal_fails() {
new_test_ext().execute_with(|| {
assert_noop!(
{
#[allow(deprecated)]
Treasury::approve_proposal(RuntimeOrigin::root(), 0)
},
TreasuryError::InvalidIndex
);
});
}

#[test]
fn accept_already_rejected_spend_proposal_fails() {
new_test_ext().execute_with(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);

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

#[test]
fn accepted_spend_proposal_enacted_on_spend_period() {
new_test_ext().execute_with(|| {
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);

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

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

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

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

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

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

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

<Treasury as OnInitialize<u64>>::on_initialize(4);
assert_eq!(Treasury::pot(), 0); // Pot is emptied
Expand All @@ -486,22 +311,8 @@ 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!({
#[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)
});
assert_ok!(Treasury::spend_local(RuntimeOrigin::root(), 99, 3));
assert_ok!(Treasury::spend_local(RuntimeOrigin::root(), 1, 3));
<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