Allow retract_tip on tip_new (#6511)

* Allow `retract_tip` on `tip_new`

* initial migration code

* test migration

* make pub

* bump spec

Author: shawntabrizi

Cargo.lock

@@ -97,8 +97,8 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// and set impl_version to 0. If only runtime
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
-	spec_version: 254,
-	impl_version: 1,
+	spec_version: 255,
+	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 	transaction_version: 1,
 };

bin/node/runtime/src/lib.rs

@@ -25,6 +25,7 @@ frame-benchmarking = { version = "2.0.0-rc4", default-features = false, path = "
 [dev-dependencies]
 sp-io ={ version = "2.0.0-rc4", path = "../../primitives/io" }
 sp-core = { version = "2.0.0-rc4", path = "../../primitives/core" }
+sp-storage = { version = "2.0.0-rc4", path = "../../primitives/storage" }
 
 [features]
 default = ["std"]

frame/treasury/Cargo.toml

@@ -192,13 +192,17 @@ pub struct OpenTip<
 	reason: Hash,
 	/// The account to be tipped.
 	who: AccountId,
-	/// The account who began this tip and the amount held on deposit.
-	finder: Option<(AccountId, Balance)>,
+	/// The account who began this tip.
+	finder: AccountId,
+	/// The amount held on deposit for this tip.
+	deposit: Balance,
 	/// The block number at which this tip will close if `Some`. If `None`, then no closing is
 	/// scheduled.
 	closes: Option<BlockNumber>,
 	/// The members who have voted for this tip. Sorted by AccountId.
 	tips: Vec<(AccountId, Balance)>,
+	/// Whether this tip should result in the finder taking a fee.
+	finders_fee: bool,
 }
 
 decl_storage! {
@@ -428,8 +432,15 @@ decl_module! {
 			T::Currency::reserve(&finder, deposit)?;
 
 			Reasons::<T>::insert(&reason_hash, &reason);
-			let finder = Some((finder, deposit));
-			let tip = OpenTip { reason: reason_hash, who, finder, closes: None, tips: vec![] };
+			let tip = OpenTip {
+				reason: reason_hash,
+				who,
+				finder,
+				deposit,
+				closes: None,
+				tips: vec![],
+				finders_fee: true
+			};
 			Tips::<T>::insert(&hash, tip);
 			Self::deposit_event(RawEvent::NewTip(hash));
 		}
@@ -457,12 +468,13 @@ decl_module! {
 		fn retract_tip(origin, hash: T::Hash) {
 			let who = ensure_signed(origin)?;
 			let tip = Tips::<T>::get(&hash).ok_or(Error::<T>::UnknownTip)?;
-			let (finder, deposit) = tip.finder.ok_or(Error::<T>::NotFinder)?;
-			ensure!(finder == who, Error::<T>::NotFinder);
+			ensure!(tip.finder == who, Error::<T>::NotFinder);
 
 			Reasons::<T>::remove(&tip.reason);
 			Tips::<T>::remove(&hash);
-			let _ = T::Currency::unreserve(&who, deposit);
+			if !tip.deposit.is_zero() {
+				let _ = T::Currency::unreserve(&who, tip.deposit);
+			}
 			Self::deposit_event(RawEvent::TipRetracted(hash));
 		}
 
@@ -501,8 +513,16 @@ decl_module! {
 
 			Reasons::<T>::insert(&reason_hash, &reason);
 			Self::deposit_event(RawEvent::NewTip(hash.clone()));
-			let tips = vec![(tipper, tip_value)];
-			let tip = OpenTip { reason: reason_hash, who, finder: None, closes: None, tips };
+			let tips = vec![(tipper.clone(), tip_value)];
+			let tip = OpenTip {
+				reason: reason_hash,
+				who,
+				finder: tipper,
+				deposit: Zero::zero(),
+				closes: None,
+				tips,
+				finders_fee: false,
+			};
 			Tips::<T>::insert(&hash, tip);
 		}
 
@@ -667,15 +687,17 @@ impl<T: Trait> Module<T> {
 		let treasury = Self::account_id();
 		let max_payout = Self::pot();
 		let mut payout = tips[tips.len() / 2].1.min(max_payout);
-		if let Some((finder, deposit)) = tip.finder {
-			let _ = T::Currency::unreserve(&finder, deposit);
-			if finder != tip.who {
+		if !tip.deposit.is_zero() {
+			let _ = T::Currency::unreserve(&tip.finder, tip.deposit);
+		}
+		if tip.finders_fee {
+			if tip.finder != tip.who {
 				// pay out the finder's fee.
 				let finders_fee = T::TipFindersFee::get() * payout;
 				payout -= finders_fee;
 				// this should go through given we checked it's at most the free balance, but still
 				// we only make a best-effort.
-				let _ = T::Currency::transfer(&treasury, &finder, finders_fee, KeepAlive);
+				let _ = T::Currency::transfer(&treasury, &tip.finder, finders_fee, KeepAlive);
 			}
 		}
 		// same as above: best-effort only.
@@ -753,6 +775,55 @@ impl<T: Trait> Module<T> {
 			// Must never be less than 0 but better be safe.
 			.saturating_sub(T::Currency::minimum_balance())
 	}
+
+	pub fn migrate_retract_tip_for_tip_new() {
+		/// An open tipping "motion". Retains all details of a tip including information on the finder
+		/// and the members who have voted.
+		#[derive(Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug)]
+		pub struct OldOpenTip<
+			AccountId: Parameter,
+			Balance: Parameter,
+			BlockNumber: Parameter,
+			Hash: Parameter,
+		> {
+			/// The hash of the reason for the tip. The reason should be a human-readable UTF-8 encoded string. A URL would be
+			/// sensible.
+			reason: Hash,
+			/// The account to be tipped.
+			who: AccountId,
+			/// The account who began this tip and the amount held on deposit.
+			finder: Option<(AccountId, Balance)>,
+			/// The block number at which this tip will close if `Some`. If `None`, then no closing is
+			/// scheduled.
+			closes: Option<BlockNumber>,
+			/// The members who have voted for this tip. Sorted by AccountId.
+			tips: Vec<(AccountId, Balance)>,
+		}
+
+		use frame_support::{Twox64Concat, migration::StorageKeyIterator};
+
+		for (hash, old_tip) in StorageKeyIterator::<
+			T::Hash,
+			OldOpenTip<T::AccountId, BalanceOf<T>, T::BlockNumber, T::Hash>,
+			Twox64Concat,
+		>::new(b"Treasury", b"Tips").drain()
+		{
+			let (finder, deposit, finders_fee) = match old_tip.finder {
+				Some((finder, deposit)) => (finder, deposit, true),
+				None => (T::AccountId::default(), Zero::zero(), false),
+			};
+			let new_tip = OpenTip {
+				reason: old_tip.reason,
+				who: old_tip.who,
+				finder,
+				deposit,
+				closes: old_tip.closes,
+				tips: old_tip.tips,
+				finders_fee
+			};
+			Tips::<T>::insert(hash, new_tip)
+		}
+	}
 }
 
 impl<T: Trait> OnUnbalanced<NegativeImbalanceOf<T>> for Module<T> {

frame/treasury/src/lib.rs

@@ -293,6 +293,7 @@ fn close_tip_works() {
 #[test]
 fn retract_tip_works() {
 	new_test_ext().execute_with(|| {
+		// with report awesome
 		Balances::make_free_balance_be(&Treasury::account_id(), 101);
 		assert_ok!(Treasury::report_awesome(Origin::signed(0), b"awesome.dot".to_vec(), 3));
 		let h = tip_hash();
@@ -303,6 +304,17 @@ fn retract_tip_works() {
 		assert_ok!(Treasury::retract_tip(Origin::signed(0), h.clone()));
 		System::set_block_number(2);
 		assert_noop!(Treasury::close_tip(Origin::signed(0), h.into()), Error::<Test>::UnknownTip);
+
+		// with tip new
+		Balances::make_free_balance_be(&Treasury::account_id(), 101);
+		assert_ok!(Treasury::tip_new(Origin::signed(10), b"awesome.dot".to_vec(), 3, 10));
+		let h = tip_hash();
+		assert_ok!(Treasury::tip(Origin::signed(11), h.clone(), 10));
+		assert_ok!(Treasury::tip(Origin::signed(12), h.clone(), 10));
+		assert_noop!(Treasury::retract_tip(Origin::signed(0), h.clone()), Error::<Test>::NotFinder);
+		assert_ok!(Treasury::retract_tip(Origin::signed(10), h.clone()));
+		System::set_block_number(2);
+		assert_noop!(Treasury::close_tip(Origin::signed(10), h.into()), Error::<Test>::UnknownTip);
 	});
 }
 
@@ -544,3 +556,97 @@ fn inexistent_account_works() {
 		assert_eq!(Balances::free_balance(3), 99); // Balance of `3` has changed
 	});
 }
+
+#[test]
+fn test_last_reward_migration() {
+	use sp_storage::Storage;
+
+	let mut s = Storage::default();
+
+	#[derive(Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug)]
+	pub struct OldOpenTip<
+		AccountId: Parameter,
+		Balance: Parameter,
+		BlockNumber: Parameter,
+		Hash: Parameter,
+	> {
+		/// The hash of the reason for the tip. The reason should be a human-readable UTF-8 encoded string. A URL would be
+		/// sensible.
+		reason: Hash,
+		/// The account to be tipped.
+		who: AccountId,
+		/// The account who began this tip and the amount held on deposit.
+		finder: Option<(AccountId, Balance)>,
+		/// The block number at which this tip will close if `Some`. If `None`, then no closing is
+		/// scheduled.
+		closes: Option<BlockNumber>,
+		/// The members who have voted for this tip. Sorted by AccountId.
+		tips: Vec<(AccountId, Balance)>,
+	}
+
+	let reason1 = BlakeTwo256::hash(b"reason1");
+	let hash1 = BlakeTwo256::hash_of(&(reason1, 10u64));
+
+	let old_tip_finder = OldOpenTip::<u64, u64, u64, H256> {
+		reason: reason1,
+		who: 10,
+		finder: Some((20, 30)),
+		closes: Some(13),
+		tips: vec![(40, 50), (60, 70)]
+	};
+
+	let reason2 = BlakeTwo256::hash(b"reason2");
+	let hash2 = BlakeTwo256::hash_of(&(reason2, 20u64));
+
+	let old_tip_no_finder = OldOpenTip::<u64, u64, u64, H256> {
+		reason: reason2,
+		who: 20,
+		finder: None,
+		closes: Some(13),
+		tips: vec![(40, 50), (60, 70)]
+	};
+
+	let data = vec![
+		(
+			Tips::<Test>::hashed_key_for(hash1),
+			old_tip_finder.encode().to_vec()
+		),
+		(
+			Tips::<Test>::hashed_key_for(hash2),
+			old_tip_no_finder.encode().to_vec()
+		),
+	];
+
+	s.top = data.into_iter().collect();
+	sp_io::TestExternalities::new(s).execute_with(|| {
+		Treasury::migrate_retract_tip_for_tip_new();
+
+		// Test w/ finder
+		assert_eq!(
+			Tips::<Test>::get(hash1),
+			Some(OpenTip {
+				reason: reason1,
+				who: 10,
+				finder: 20,
+				deposit: 30,
+				closes: Some(13),
+				tips: vec![(40, 50), (60, 70)],
+				finders_fee: true,
+			})
+		);
+
+		// Test w/o finder
+		assert_eq!(
+			Tips::<Test>::get(hash2),
+			Some(OpenTip {
+				reason: reason2,
+				who: 20,
+				finder: Default::default(),
+				deposit: 0,
+				closes: Some(13),
+				tips: vec![(40, 50), (60, 70)],
+				finders_fee: false,
+			})
+		);
+	});
+}