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

Allow council to slash treasury tip #7753

Merged
19 commits merged into from
Dec 29, 2020
Merged

Allow council to slash treasury tip #7753

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
5efbfee
wk2051 | D4 |Allow council to slash treasury tip | p1
shamb0 Dec 17, 2020
b9a938c
Update frame/tips/src/lib.rs
shamb0 Dec 18, 2020
170f288
wk2051 | D5 |Allow council to slash treasury tip | p2
shamb0 Dec 18, 2020
5a62c97
wk2051 | D5 |Allow council to slash treasury tip | p3
shamb0 Dec 18, 2020
b157188
wk2051 | D5 |Allow council to slash treasury tip | p4
shamb0 Dec 19, 2020
26478ea
wk2051 | D5 |Allow council to slash treasury tip | p5
shamb0 Dec 20, 2020
04f41a6
Merge branch 'master' into pr/7753
shawntabrizi Dec 20, 2020
d03d70b
random change
shawntabrizi Dec 20, 2020
b18bdcd
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Dec 20, 2020
e34b421
Merge pull request #2 from paritytech/shawntabrizi-slash-tip-weight
shamb0 Dec 20, 2020
7088c3d
fix typo
shawntabrizi Dec 21, 2020
d22e8b6
Update frame/tips/src/lib.rs
shamb0 Dec 21, 2020
3b8bb56
Update frame/tips/src/lib.rs
shamb0 Dec 21, 2020
875257f
Update frame/tips/src/lib.rs
shamb0 Dec 21, 2020
ddd1ead
Update frame/tips/src/lib.rs
shamb0 Dec 21, 2020
f7261dc
Update frame/tips/src/tests.rs
shamb0 Dec 21, 2020
0aa051a
wk2052 | D1 | Allow council to slash treasury tip | p6
shamb0 Dec 21, 2020
287269b
Merge branch 'master' into pr/7753
shawntabrizi Dec 29, 2020
f9c865c
Merge branch 'issue-7057-allow-council-to-slash-treasury-tip' of http…
shawntabrizi Dec 29, 2020
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
33 changes: 31 additions & 2 deletions frame/tips/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ use frame_support::traits::{
use sp_runtime::{ Percent, RuntimeDebug, traits::{
Zero, AccountIdConversion, Hash, BadOrigin
}};

use frame_support::traits::{Contains, ContainsLengthBound};
use frame_support::traits::{
Contains, ContainsLengthBound,
OnUnbalanced, EnsureOrigin,
};
use codec::{Encode, Decode};
use frame_system::{self as system, ensure_signed};
pub use weights::WeightInfo;
Expand Down Expand Up @@ -168,6 +170,8 @@ decl_event!(
TipClosing(Hash),
/// A tip suggestion has been closed. \[tip_hash, who, payout\]
TipClosed(Hash, AccountId, Balance),
/// A tip suggestion has been slashed. \[tip_hash, finder, deposit\]
Copy link
Member

Choose a reason for hiding this comment

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

Probably best to keep this at the bottom of the list.

You have to think about how these structures and enums are encoded. At some point, there will be an event emitted with index 5 for example. If you place a new item at 4, then all other items shift forward by 1, and now 5 means something different. This isn't always important, and if you are using metadata correctly it shouldnt matter, but here, i dont see an advantage to introduce a breaking encoding change.

Copy link
Contributor Author

@shamb0 shamb0 Dec 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks to make me aware about encoding logic behind this enums, moved the new one to end of the list. Done

TipSlashed(Hash, AccountId, Balance),
/// A tip suggestion has been retracted. \[tip_hash\]
TipRetracted(Hash),
}
Expand Down Expand Up @@ -379,6 +383,31 @@ decl_module! {
Tips::<T>::insert(&hash, tip);
}

/// Remove and slash an already-open tip.
///
/// May only be called from `T::RejectOrigin`.
///
/// As a result, API will slash the finder and the deposits are lost.
///
/// Emits `TipSlashed` if successful.
#[weight = 10_000]
Copy link
Member

Choose a reason for hiding this comment

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

You need to write a new benchmark for this, and use the T::WeightInfo trait like all the other functions.

fn slash_tip(origin, hash: T::Hash) {
T::RejectOrigin::ensure_origin(origin)?;

let tip = Tips::<T>::get(hash).ok_or(Error::<T>::UnknownTip)?;

Copy link
Member

Choose a reason for hiding this comment

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

This on the other hand is VERY important to put at the bottom.

This would be a breaking change for the transaction ordering which would actually be a breaking change for most chains.

Specifically, things like Hardware wallets (ledgers etc) depend on the order of the extrinsics in order to correctly encode and sign transactions.

When you submit a transaction to a chain, you are sending only 2 numbers to identify the extrinsic: the module, and the extrinsic number.

If you add a new dispatchable function in the middle of the other dispatchable function, then the order and number of the extrinsics will change, and this is not good.

Instead, if you place new things at the bottom of the list, then the ordering will always be backwards compatible...

So please place this at the bottom of the extrinsics list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it Shawn, moved to the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very much agreeing. An additional note for when you have to break the transaction version is to check if the next polkadot release is already breaking it or not. It would be sensible to postpone non-urgent transaction-breaking PRs to a release for which hardware developers have been notified of the change and whatnot.

if !tip.deposit.is_zero() {
let deposit = tip.deposit;
let imbalance = T::Currency::slash_reserved(&tip.finder, deposit).0;
T::OnSlash::on_unbalanced(imbalance);
}

Reasons::<T>::remove(&tip.reason);
Tips::<T>::remove(hash);

Self::deposit_event(RawEvent::TipSlashed(hash, tip.finder, tip.deposit));
}

/// Close and payout a tip.
///
/// The dispatch origin for this call must be _Signed_.
Expand Down
18 changes: 18 additions & 0 deletions frame/tips/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,24 @@ fn close_tip_works() {
});
}

#[test]
fn slash_tip_works() {
new_test_ext().execute_with(|| {
System::set_block_number(1);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);
assert_ok!(TipsModTestInst::report_awesome(Origin::signed(0), b"awesome.dot".to_vec(), 3));
let h = tip_hash();
assert_eq!(last_event(), RawEvent::NewTip(h));
assert_noop!(
TipsModTestInst::slash_tip(Origin::signed(0), h.clone()),
BadOrigin,
);
assert_ok!(TipsModTestInst::slash_tip(Origin::root(), h.clone()));
assert_eq!(last_event(), RawEvent::TipSlashed(h, 0, 12));
Copy link
Member

Choose a reason for hiding this comment

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

I want to see balance changes here. So show that a reserve balance is taken for creating a tip, and that it is completely removed from the user after the slash event.

assert_eq!(Balances::reserved_balance(...), ...)

etc..

});
}

#[test]
fn retract_tip_works() {
new_test_ext().execute_with(|| {
Expand Down