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

Treasury rewards should pay the remainder of the 10% #4026

Merged
merged 5 commits into from
Nov 6, 2019
Merged

Treasury rewards should pay the remainder of the 10% #4026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
90d3e01
Treasury rewards should pay the remainder of the 10%..
gavofyork Nov 5, 2019
f4c6824
Event for deposits coming into treasury
gavofyork Nov 6, 2019
0688fce
Fix tests
gavofyork Nov 6, 2019
412e62a
Merge remote-tracking branch 'origin/master' into gav-treasury-rewards
gavofyork Nov 6, 2019
b15f69f
Remove OnDilution
gavofyork Nov 6, 2019
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
17 changes: 16 additions & 1 deletion node/executor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,13 +466,18 @@ mod tests {
event: Event::system(system::Event::ExtrinsicSuccess),
topics: vec![],
},
EventRecord {
phase: Phase::ApplyExtrinsic(1),
event: Event::treasury(treasury::RawEvent::Deposit(1984800000000)),
topics: vec![],
},
EventRecord {
phase: Phase::ApplyExtrinsic(1),
event: Event::balances(balances::RawEvent::Transfer(
alice().into(),
bob().into(),
69 * DOLLARS,
1 * CENTS
1 * CENTS,
)),
topics: vec![],
},
Expand Down Expand Up @@ -510,6 +515,11 @@ mod tests {
event: Event::system(system::Event::ExtrinsicSuccess),
topics: vec![],
},
EventRecord {
phase: Phase::ApplyExtrinsic(1),
event: Event::treasury(treasury::RawEvent::Deposit(1984780231392)),
topics: vec![],
},
EventRecord {
phase: Phase::ApplyExtrinsic(1),
event: Event::balances(
Expand All @@ -527,6 +537,11 @@ mod tests {
event: Event::system(system::Event::ExtrinsicSuccess),
topics: vec![],
},
EventRecord {
phase: Phase::ApplyExtrinsic(2),
event: Event::treasury(treasury::RawEvent::Deposit(1984780231392)),
topics: vec![],
},
EventRecord {
phase: Phase::ApplyExtrinsic(2),
event: Event::balances(
Expand Down
7 changes: 5 additions & 2 deletions node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ impl session::historical::Trait for Runtime {
srml_staking_reward_curve::build! {
const REWARD_CURVE: PiecewiseLinear<'static> = curve!(
min_inflation: 0_025_000,
max_inflation: 0_100_000,
max_inflation: 0_100_000, // 10% - must be equal to MaxReward below.
ideal_stake: 0_500_000,
falloff: 0_050_000,
max_piece_count: 40,
Expand All @@ -253,20 +253,23 @@ parameter_types! {
pub const SessionsPerEra: sr_staking_primitives::SessionIndex = 6;
pub const BondingDuration: staking::EraIndex = 24 * 28;
pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE;
pub const MaxReward: Perbill = Perbill::from_percent(10);
// ^^^ 10% - must be equal to max_inflation, above.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is annoyingly the same value in two different places; the curve! macro requires a literal and we can't extract the parameters once it's created.

Copy link
Contributor

@gui1117 gui1117 Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The only way I see to remove this duplication is:

  • make max_reward perbill an output of curve macro
  • make the curve a struct with different fields
  • (or wait for more const in rust: all arithmetic operation needed by curve could potentially be const)
  • (or make the curve build at runtime)

}

impl staking::Trait for Runtime {
type Currency = Balances;
type Time = Timestamp;
type CurrencyToVote = CurrencyToVoteHandler;
type OnRewardMinted = Treasury;
type RewardRemainder = Treasury;
type Event = Event;
type Slash = Treasury; // send the slashed funds to the treasury.
type Reward = (); // rewards are minted from the void
type SessionsPerEra = SessionsPerEra;
type BondingDuration = BondingDuration;
type SessionInterface = Self;
type RewardCurve = RewardCurve;
type MaxPossibleReward = MaxReward;
}

parameter_types! {
Expand Down
23 changes: 16 additions & 7 deletions srml/staking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ use codec::{HasCompact, Encode, Decode};
use support::{
decl_module, decl_event, decl_storage, ensure,
traits::{
Currency, OnFreeBalanceZero, OnDilution, LockIdentifier, LockableCurrency,
Currency, OnFreeBalanceZero, LockIdentifier, LockableCurrency,
WithdrawReasons, OnUnbalanced, Imbalance, Get, Time
}
};
Expand Down Expand Up @@ -501,8 +501,8 @@ pub trait Trait: system::Trait {
/// The post-processing needs it but will be moved to off-chain. TODO: #2908
type CurrencyToVote: Convert<BalanceOf<Self>, u64> + Convert<u128, BalanceOf<Self>>;

/// Some tokens minted.
type OnRewardMinted: OnDilution<BalanceOf<Self>>;
/// Tokens have been minted and are unused for validator-reward.
type RewardRemainder: OnUnbalanced<NegativeImbalanceOf<Self>>;

/// The overarching event type.
type Event: From<Event<Self>> + Into<<Self as system::Trait>::Event>;
Expand All @@ -524,6 +524,10 @@ pub trait Trait: system::Trait {

/// The NPoS reward curve to use.
type RewardCurve: Get<&'static PiecewiseLinear<'static>>;

/// The maximum possible reward (in proportion of total issued tokens) that can be paid in one
/// reward cycle.
type MaxPossibleReward: Get<Perbill>;
}

/// Mode of era-forcing.
Expand Down Expand Up @@ -652,8 +656,9 @@ decl_storage! {

decl_event!(
pub enum Event<T> where Balance = BalanceOf<T>, <T as system::Trait>::AccountId {
/// All validators have been rewarded by the given balance.
Reward(Balance),
/// All validators have been rewarded by the first balance; the second is the remainder
/// from the maximum amount of reward.
Reward(Balance, Balance),
/// One validator (and its nominators) has been slashed by the given amount.
Slash(AccountId, Balance),
/// An old slashing report from a prior era was discarded because it could
Expand Down Expand Up @@ -1218,9 +1223,13 @@ impl<T: Trait> Module<T> {
let total_reward = total_imbalance.peek();
// assert!(total_reward <= total_payout)

Self::deposit_event(RawEvent::Reward(total_reward));
let max_reward = T::MaxPossibleReward::get() * T::Currency::total_issuance();
let rest_reward = max_reward.saturating_sub(total_reward);

Self::deposit_event(RawEvent::Reward(total_reward, rest_reward));

T::Reward::on_unbalanced(total_imbalance);
T::OnRewardMinted::on_dilution(total_reward, total_rewarded_stake);
T::RewardRemainder::on_unbalanced(T::Currency::issue(rest_reward));
}

// Increment current era.
Expand Down
4 changes: 3 additions & 1 deletion srml/staking/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,19 +192,21 @@ parameter_types! {
pub const SessionsPerEra: SessionIndex = 3;
pub const BondingDuration: EraIndex = 3;
pub const RewardCurve: &'static PiecewiseLinear<'static> = &I_NPOS;
pub const MaxReward: Perbill = Perbill::from_percent(10);
}
impl Trait for Test {
type Currency = balances::Module<Self>;
type Time = timestamp::Module<Self>;
type CurrencyToVote = CurrencyToVoteHandler;
type OnRewardMinted = ();
type RewardRemainder = ();
type Event = ();
type Slash = ();
type Reward = ();
type SessionsPerEra = SessionsPerEra;
type BondingDuration = BondingDuration;
type SessionInterface = Self;
type RewardCurve = RewardCurve;
type MaxPossibleReward = MaxReward;
}

pub struct ExtBuilder {
Expand Down
11 changes: 0 additions & 11 deletions srml/support/src/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,17 +69,6 @@ pub trait OnFreeBalanceZero<AccountId> {
fn on_free_balance_zero(who: &AccountId);
}

/// Trait for a hook to get called when some balance has been minted, causing dilution.
pub trait OnDilution<Balance> {
/// Some `portion` of the total balance just "grew" by `minted`. `portion` is the pre-growth
/// amount (it doesn't take account of the recent growth).
fn on_dilution(minted: Balance, portion: Balance);
}

impl<Balance> OnDilution<Balance> for () {
fn on_dilution(_minted: Balance, _portion: Balance) {}
}

/// Outcome of a balance update.
pub enum UpdateBalanceOutcome {
/// Account balance was simply updated.
Expand Down
93 changes: 21 additions & 72 deletions srml/treasury/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,6 @@
//! respectively.
//! - **Pot:** Unspent funds accumulated by the treasury module.
//!
//! ### Implementations
//!
//! The treasury module provides an implementation for the following trait:
//!
//! - `OnDilution` - When new funds are minted to reward the deployment of other existing funds,
//! a corresponding amount of tokens are minted into the treasury so that the tokens being rewarded
//! do not represent a higher portion of total supply. For example, in the default substrate node,
//! when validators are rewarded new tokens for staking, they do not hold a higher portion of total
//! tokens. Rather, tokens are added to the treasury to keep the portion of tokens staked constant.
//!
//! ## Interface
//!
//! ### Dispatchable Functions
Expand All @@ -72,12 +62,12 @@ use serde::{Serialize, Deserialize};
use rstd::prelude::*;
use support::{decl_module, decl_storage, decl_event, ensure, print};
use support::traits::{
Currency, ExistenceRequirement, Get, Imbalance, OnDilution, OnUnbalanced,
Currency, ExistenceRequirement, Get, Imbalance, OnUnbalanced,
ReservableCurrency, WithdrawReason
};
use sr_primitives::{Permill, Perbill, ModuleId};
use sr_primitives::{Permill, ModuleId};
use sr_primitives::traits::{
Zero, EnsureOrigin, StaticLookup, AccountIdConversion, CheckedSub, Saturating
Zero, EnsureOrigin, StaticLookup, AccountIdConversion, Saturating
};
use sr_primitives::weights::SimpleDispatchInfo;
use codec::{Encode, Decode};
Expand Down Expand Up @@ -257,6 +247,8 @@ decl_event!(
Burnt(Balance),
/// Spending has finished; this is the amount that rolls over until next spend.
Rollover(Balance),
/// Some funds have been deposited.
Deposit(Balance),
}
);

Expand Down Expand Up @@ -346,29 +338,12 @@ impl<T: Trait> Module<T> {

impl<T: Trait> OnUnbalanced<NegativeImbalanceOf<T>> for Module<T> {
fn on_unbalanced(amount: NegativeImbalanceOf<T>) {
let numeric_amount = amount.peek();

// Must resolve into existing but better to be safe.
let _ = T::Currency::resolve_creating(&Self::account_id(), amount);
}
}

/// Mint extra funds for the treasury to keep the ratio of portion to total_issuance equal
/// pre dilution and post-dilution.
///
/// i.e.
/// ```nocompile
/// portion / total_issuance_before_dilution ==
/// (portion + minted) / (total_issuance_before_dilution + minted_to_treasury + minted)
/// ```
impl<T: Trait> OnDilution<BalanceOf<T>> for Module<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are to not have this trait and just use imbalances for it (as it seems here), I think it can be removed as a trait (have not done a clone build but should already give some warnings afaiu)

fn on_dilution(minted: BalanceOf<T>, portion: BalanceOf<T>) {
if !minted.is_zero() && !portion.is_zero() {
let total_issuance = T::Currency::total_issuance();
if let Some(funding) = total_issuance.checked_sub(&portion) {
let increase_ratio = Perbill::from_rational_approximation(minted, portion);
let funding = increase_ratio * funding;
Self::on_unbalanced(T::Currency::issue(funding));
}
}
Self::deposit_event(RawEvent::Deposit(numeric_amount));
}
}

Expand All @@ -379,7 +354,7 @@ mod tests {
use support::{assert_noop, assert_ok, impl_outer_origin, parameter_types};
use primitives::H256;
use sr_primitives::{
traits::{BlakeTwo256, OnFinalize, IdentityLookup}, testing::Header, assert_eq_error_rate,
traits::{BlakeTwo256, OnFinalize, IdentityLookup}, testing::Header, Perbill
};

impl_outer_origin! {
Expand Down Expand Up @@ -470,37 +445,11 @@ mod tests {
fn minting_works() {
new_test_ext().execute_with(|| {
// Check that accumulate works when we have Some value in Dummy already.
Treasury::on_dilution(100, 100);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);
});
}

#[test]
fn minting_works_2() {
let tests = [(1, 10), (1, 20), (40, 130), (2, 66), (2, 67), (2, 100), (2, 101), (2, 134)];
for &(minted, portion) in &tests {
new_test_ext().execute_with(|| {
let init_total_issuance = Balances::total_issuance();
Treasury::on_dilution(minted, portion);

assert_eq!(
Treasury::pot(),
(((init_total_issuance - portion) * minted) as f32 / portion as f32)
.round() as u64
);

// Assert:
// portion / init_total_issuance
// == (portion + minted) / (init_total_issuance + Treasury::pot() + minted),
assert_eq_error_rate!(
portion * 1_000 / init_total_issuance,
(portion + minted) * 1_000 / (init_total_issuance + Treasury::pot() + minted),
2,
);
});
}
}

#[test]
fn spend_proposal_takes_min_deposit() {
new_test_ext().execute_with(|| {
Expand Down Expand Up @@ -529,7 +478,7 @@ mod tests {
#[test]
fn accepted_spend_proposal_ignored_outside_spend_period() {
new_test_ext().execute_with(|| {
Treasury::on_dilution(100, 100);
Balances::make_free_balance_be(&Treasury::account_id(), 101);

assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3));
assert_ok!(Treasury::approve_proposal(Origin::ROOT, 0));
Expand All @@ -544,7 +493,7 @@ mod tests {
fn unused_pot_should_diminish() {
new_test_ext().execute_with(|| {
let init_total_issuance = Balances::total_issuance();
Treasury::on_dilution(100, 100);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Balances::total_issuance(), init_total_issuance + 100);

<Treasury as OnFinalize<u64>>::on_finalize(2);
Expand All @@ -556,7 +505,7 @@ mod tests {
#[test]
fn rejected_spend_proposal_ignored_on_spend_period() {
new_test_ext().execute_with(|| {
Treasury::on_dilution(100, 100);
Balances::make_free_balance_be(&Treasury::account_id(), 101);

assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3));
assert_ok!(Treasury::reject_proposal(Origin::ROOT, 0));
Expand All @@ -570,7 +519,7 @@ mod tests {
#[test]
fn reject_already_rejected_spend_proposal_fails() {
new_test_ext().execute_with(|| {
Treasury::on_dilution(100, 100);
Balances::make_free_balance_be(&Treasury::account_id(), 101);

assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3));
assert_ok!(Treasury::reject_proposal(Origin::ROOT, 0));
Expand All @@ -595,7 +544,7 @@ mod tests {
#[test]
fn accept_already_rejected_spend_proposal_fails() {
new_test_ext().execute_with(|| {
Treasury::on_dilution(100, 100);
Balances::make_free_balance_be(&Treasury::account_id(), 101);

assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3));
assert_ok!(Treasury::reject_proposal(Origin::ROOT, 0));
Expand All @@ -606,7 +555,7 @@ mod tests {
#[test]
fn accepted_spend_proposal_enacted_on_spend_period() {
new_test_ext().execute_with(|| {
Treasury::on_dilution(100, 100);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);

assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3));
Expand All @@ -621,7 +570,7 @@ mod tests {
#[test]
fn pot_underflow_should_not_diminish() {
new_test_ext().execute_with(|| {
Treasury::on_dilution(100, 100);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);

assert_ok!(Treasury::propose_spend(Origin::signed(0), 150, 3));
Expand All @@ -630,10 +579,10 @@ mod tests {
<Treasury as OnFinalize<u64>>::on_finalize(2);
assert_eq!(Treasury::pot(), 100); // Pot hasn't changed

Treasury::on_dilution(100, 100);
Balances::deposit_into_existing(&Treasury::account_id(), 100);
<Treasury as OnFinalize<u64>>::on_finalize(4);
assert_eq!(Balances::free_balance(&3), 150); // Fund has been spent
assert_eq!(Treasury::pot(), 75); // Pot has finally changed
assert_eq!(Treasury::pot(), 25); // Pot has finally changed
});
}

Expand All @@ -642,7 +591,7 @@ mod tests {
#[test]
fn treasury_account_doesnt_get_deleted() {
new_test_ext().execute_with(|| {
Treasury::on_dilution(100, 100);
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Treasury::pot(), 100);
let treasury_balance = Balances::free_balance(&Treasury::account_id());

Expand Down Expand Up @@ -685,7 +634,7 @@ mod tests {
assert_eq!(Treasury::pot(), 0); // Pot hasn't changed
assert_eq!(Balances::free_balance(&3), 0); // Balance of `3` hasn't changed

Treasury::on_dilution(100, 100);
Balances::make_free_balance_be(&Treasury::account_id(), 100);
assert_eq!(Treasury::pot(), 99); // Pot now contains funds
assert_eq!(Balances::free_balance(&Treasury::account_id()), 100); // Account does exist

Expand Down