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.

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

R4R: Removal of Mandatory Self-Delegation Reward #2984

Merged
merged 10 commits into from
Dec 4, 2018
5 changes: 3 additions & 2 deletions PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ IMPROVEMENTS
* Gaia

* SDK
- \#1277 Complete bank module specification

* \#1277 Complete bank module specification
* \#2914 No longer withdraw validator rewards on a self bond/unbond

* Tendermint


Expand Down
38 changes: 20 additions & 18 deletions cmd/gaia/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,11 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, baseAppOptio
)

// register the staking hooks
// NOTE: stakeKeeper above are passed by reference,
// so that it can be modified like below:
// NOTE: The stakeKeeper above is passed by reference, so that it can be
// modified like below:
app.stakeKeeper = *stakeKeeper.SetHooks(
NewHooks(app.distrKeeper.Hooks(), app.slashingKeeper.Hooks()))
NewStakingHooks(app.distrKeeper.Hooks(), app.slashingKeeper.Hooks()),
)

// register message routes
app.Router().
Expand Down Expand Up @@ -318,52 +319,53 @@ func (app *GaiaApp) LoadHeight(height int64) error {

//______________________________________________________________________________________________

// Combined Staking Hooks
type Hooks struct {
var _ sdk.StakingHooks = StakingHooks{}

// StakingHooks contains combined distribution and slashing hooks needed for the
// staking module.
type StakingHooks struct {
dh distr.Hooks
sh slashing.Hooks
}

func NewHooks(dh distr.Hooks, sh slashing.Hooks) Hooks {
return Hooks{dh, sh}
func NewStakingHooks(dh distr.Hooks, sh slashing.Hooks) StakingHooks {
return StakingHooks{dh, sh}
}

var _ sdk.StakingHooks = Hooks{}

// nolint
func (h Hooks) OnValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) {
func (h StakingHooks) OnValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) {
h.dh.OnValidatorCreated(ctx, valAddr)
h.sh.OnValidatorCreated(ctx, valAddr)
}
func (h Hooks) OnValidatorModified(ctx sdk.Context, valAddr sdk.ValAddress) {
func (h StakingHooks) OnValidatorModified(ctx sdk.Context, valAddr sdk.ValAddress) {
h.dh.OnValidatorModified(ctx, valAddr)
h.sh.OnValidatorModified(ctx, valAddr)
}
func (h Hooks) OnValidatorRemoved(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
func (h StakingHooks) OnValidatorRemoved(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
h.dh.OnValidatorRemoved(ctx, consAddr, valAddr)
h.sh.OnValidatorRemoved(ctx, consAddr, valAddr)
}
func (h Hooks) OnValidatorBonded(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
func (h StakingHooks) OnValidatorBonded(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
h.dh.OnValidatorBonded(ctx, consAddr, valAddr)
h.sh.OnValidatorBonded(ctx, consAddr, valAddr)
}
func (h Hooks) OnValidatorPowerDidChange(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
func (h StakingHooks) OnValidatorPowerDidChange(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
h.dh.OnValidatorPowerDidChange(ctx, consAddr, valAddr)
h.sh.OnValidatorPowerDidChange(ctx, consAddr, valAddr)
}
func (h Hooks) OnValidatorBeginUnbonding(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
func (h StakingHooks) OnValidatorBeginUnbonding(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
h.dh.OnValidatorBeginUnbonding(ctx, consAddr, valAddr)
h.sh.OnValidatorBeginUnbonding(ctx, consAddr, valAddr)
}
func (h Hooks) OnDelegationCreated(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
func (h StakingHooks) OnDelegationCreated(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
h.dh.OnDelegationCreated(ctx, delAddr, valAddr)
h.sh.OnDelegationCreated(ctx, delAddr, valAddr)
}
func (h Hooks) OnDelegationSharesModified(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
func (h StakingHooks) OnDelegationSharesModified(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
h.dh.OnDelegationSharesModified(ctx, delAddr, valAddr)
h.sh.OnDelegationSharesModified(ctx, delAddr, valAddr)
}
func (h Hooks) OnDelegationRemoved(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
func (h StakingHooks) OnDelegationRemoved(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
h.dh.OnDelegationRemoved(ctx, delAddr, valAddr)
h.sh.OnDelegationRemoved(ctx, delAddr, valAddr)
}
14 changes: 8 additions & 6 deletions x/distribution/keeper/delegation.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,12 @@ func (k Keeper) WithdrawDelegationReward(ctx sdk.Context, delAddr sdk.AccAddress
return types.ErrNoDelegationDistInfo(k.codespace)
}

feePool, valInfo, delInfo, withdraw :=
k.withdrawDelegationReward(ctx, delAddr, valAddr)
feePool, valInfo, delInfo, withdraw := k.withdrawDelegationReward(ctx, delAddr, valAddr)

k.SetValidatorDistInfo(ctx, valInfo)
k.SetDelegationDistInfo(ctx, delInfo)
k.WithdrawToDelegator(ctx, feePool, delAddr, withdraw)

return nil
}

Expand Down Expand Up @@ -194,19 +194,21 @@ func (k Keeper) WithdrawDelegationRewardsAll(ctx sdk.Context, delAddr sdk.AccAdd
func (k Keeper) withdrawDelegationRewardsAll(ctx sdk.Context,
delAddr sdk.AccAddress) types.DecCoins {

// iterate over all the delegations
withdraw := types.DecCoins{}
operationAtDelegation := func(_ int64, del sdk.Delegation) (stop bool) {

// iterate over all the delegations
operationAtDelegation := func(_ int64, del sdk.Delegation) (stop bool) {
valAddr := del.GetValidatorAddr()
feePool, valInfo, delInfo, diWithdraw :=
k.withdrawDelegationReward(ctx, delAddr, valAddr)
feePool, valInfo, delInfo, diWithdraw := k.withdrawDelegationReward(ctx, delAddr, valAddr)
withdraw = withdraw.Plus(diWithdraw)

k.SetFeePool(ctx, feePool)
k.SetValidatorDistInfo(ctx, valInfo)
k.SetDelegationDistInfo(ctx, delInfo)

return false
}

k.stakeKeeper.IterateDelegations(ctx, delAddr, operationAtDelegation)
return withdraw
}
Expand Down
10 changes: 9 additions & 1 deletion x/distribution/keeper/hooks.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper

import (
"bytes"
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -77,6 +78,14 @@ func (k Keeper) onDelegationCreated(ctx sdk.Context, delAddr sdk.AccAddress,
func (k Keeper) onDelegationSharesModified(ctx sdk.Context, delAddr sdk.AccAddress,
valAddr sdk.ValAddress) {

if bytes.Equal(delAddr.Bytes(), valAddr.Bytes()) {
// On updates to a self bond/unbond, we must update the validator's dist
// info without withdrawing any rewards.
k.UpdateValidatorDistInfoFromPool(ctx, valAddr)
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't need to be exposed (should keep consistency)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Updated.

But consistency with what exactly? Everything in x/distribution/keeper/validator.go is exposed (which is where this lives). I wasn't able to find a consistent pattern for these methods. I think a large bulk of them should be unexposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also if you look at what's happening within onValidatorModified you'll see we skip running this at height of 0 - don't see why we wouldn't replicate this for the new function too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto: I don't think there is a need now as I just moved the new call into onValidatorModified.

} else {
k.onValidatorModified(ctx, valAddr)
}

if err := k.WithdrawDelegationReward(ctx, delAddr, valAddr); err != nil {
panic(err)
}
Expand Down Expand Up @@ -116,7 +125,6 @@ func (h Hooks) OnDelegationCreated(ctx sdk.Context, delAddr sdk.AccAddress, valA
h.k.onDelegationCreated(ctx, delAddr, valAddr)
Copy link
Contributor

Choose a reason for hiding this comment

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

The new hook route should also be called within the OnDelegationCreated hook here

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 don't think there is a need now as I just moved the new call into onValidatorModified.

}
func (h Hooks) OnDelegationSharesModified(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
h.k.onValidatorModified(ctx, valAddr)
h.k.onDelegationSharesModified(ctx, delAddr, valAddr)
}
func (h Hooks) OnDelegationRemoved(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
Expand Down
12 changes: 12 additions & 0 deletions x/distribution/keeper/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,18 @@ func (k Keeper) GetValidatorAccum(ctx sdk.Context, operatorAddr sdk.ValAddress)
return accum, nil
}

// UpdateValidatorDistInfoFromPool updates the validator's distribution info
// from the global fee pool without withdrawing any rewards. This will be called
// from a onDelegationSharesModified hook on a non self bond/unbond call.
func (k Keeper) UpdateValidatorDistInfoFromPool(ctx sdk.Context, operatorAddr sdk.ValAddress) {
valInfo := k.GetValidatorDistInfo(ctx, operatorAddr)
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
wc := k.GetWithdrawContext(ctx, operatorAddr)
valInfo, feePool := valInfo.TakeFeePoolRewards(wc)

k.SetFeePool(ctx, feePool)
k.SetValidatorDistInfo(ctx, valInfo)
}

// withdrawal all the validator rewards including the commission
func (k Keeper) WithdrawValidatorRewardsAll(ctx sdk.Context, operatorAddr sdk.ValAddress) sdk.Error {

Expand Down
1 change: 1 addition & 0 deletions x/stake/keeper/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ func (k Keeper) OnValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) {
k.hooks.OnValidatorCreated(ctx, valAddr)
}
}

func (k Keeper) OnValidatorModified(ctx sdk.Context, valAddr sdk.ValAddress) {
if k.hooks != nil {
k.hooks.OnValidatorModified(ctx, valAddr)
Expand Down