From 66d83fb43dd0cbf212bb73b0ab00f214167221bf Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Thu, 29 Nov 2018 10:44:06 -0500 Subject: [PATCH 1/5] Some minor cleanup and reformatting to make things easier to understand --- cmd/gaia/app/app.go | 38 +++++++++++++++-------------- x/distribution/keeper/delegation.go | 14 ++++++----- x/stake/keeper/hooks.go | 1 + 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/cmd/gaia/app/app.go b/cmd/gaia/app/app.go index e88e4c7c3c5c..b88e83e9cebc 100644 --- a/cmd/gaia/app/app.go +++ b/cmd/gaia/app/app.go @@ -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(). @@ -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) } diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 728dfb269f37..dc54a6e4fe5f 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -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 } @@ -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 } diff --git a/x/stake/keeper/hooks.go b/x/stake/keeper/hooks.go index 74e830490647..4ae158786eec 100644 --- a/x/stake/keeper/hooks.go +++ b/x/stake/keeper/hooks.go @@ -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) From f72b7b5738ed9e4fbe6c76c4056b11de5015f1c6 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Mon, 3 Dec 2018 11:19:22 -0500 Subject: [PATCH 2/5] Update onDelegationSharesModified hook --- x/distribution/keeper/hooks.go | 10 +++++++++- x/distribution/keeper/validator.go | 12 ++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index b531a40429a9..b005d286d393 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -1,6 +1,7 @@ package keeper import ( + "bytes" "fmt" sdk "github.com/cosmos/cosmos-sdk/types" @@ -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) + } else { + k.onValidatorModified(ctx, valAddr) + } + if err := k.WithdrawDelegationReward(ctx, delAddr, valAddr); err != nil { panic(err) } @@ -116,7 +125,6 @@ func (h Hooks) OnDelegationCreated(ctx sdk.Context, delAddr sdk.AccAddress, valA h.k.onDelegationCreated(ctx, delAddr, valAddr) } 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) { diff --git a/x/distribution/keeper/validator.go b/x/distribution/keeper/validator.go index 97eb5bc50eb3..e6dc72735edd 100644 --- a/x/distribution/keeper/validator.go +++ b/x/distribution/keeper/validator.go @@ -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) + 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 { From 9753d4f6e428ce98d8fb82a6376394d9e353294b Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Mon, 3 Dec 2018 12:20:54 -0500 Subject: [PATCH 3/5] Add pending log --- PENDING.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/PENDING.md b/PENDING.md index a905c1939ef6..c6c43f22132b 100644 --- a/PENDING.md +++ b/PENDING.md @@ -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 From 8ee8d0a2a1291411f31ba87a4bc90db260900ec2 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 4 Dec 2018 12:08:46 -0500 Subject: [PATCH 4/5] Address PR comments --- x/distribution/keeper/hooks.go | 16 +++++----------- x/distribution/keeper/validator.go | 12 +++++++++--- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index b005d286d393..260ee2e77cdb 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -1,7 +1,6 @@ package keeper import ( - "bytes" "fmt" sdk "github.com/cosmos/cosmos-sdk/types" @@ -29,9 +28,11 @@ func (k Keeper) onValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) { // Withdraw all validator rewards func (k Keeper) onValidatorModified(ctx sdk.Context, valAddr sdk.ValAddress) { - // This doesn't need to be run at genesis + // Move the validator's rewards from the global pool to the validator's pools + // (dist info), but without actually withdrawing the rewards. This does not + // need to happen during the genesis block. if ctx.BlockHeight() > 0 { - if err := k.WithdrawValidatorRewardsAll(ctx, valAddr); err != nil { + if err := k.updateValidatorDistInfoFromPool(ctx, valAddr); err != nil { panic(err) } } @@ -78,14 +79,6 @@ 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) - } else { - k.onValidatorModified(ctx, valAddr) - } - if err := k.WithdrawDelegationReward(ctx, delAddr, valAddr); err != nil { panic(err) } @@ -125,6 +118,7 @@ func (h Hooks) OnDelegationCreated(ctx sdk.Context, delAddr sdk.AccAddress, valA h.k.onDelegationCreated(ctx, delAddr, valAddr) } 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) { diff --git a/x/distribution/keeper/validator.go b/x/distribution/keeper/validator.go index e6dc72735edd..20ad86ca1e28 100644 --- a/x/distribution/keeper/validator.go +++ b/x/distribution/keeper/validator.go @@ -91,16 +91,22 @@ func (k Keeper) GetValidatorAccum(ctx sdk.Context, operatorAddr sdk.ValAddress) return accum, nil } -// UpdateValidatorDistInfoFromPool updates the validator's distribution info +// 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) { +// from a onValidatorModified hook. +func (k Keeper) updateValidatorDistInfoFromPool(ctx sdk.Context, operatorAddr sdk.ValAddress) sdk.Error { + if !k.HasValidatorDistInfo(ctx, operatorAddr) { + types.ErrNoValidatorDistInfo(k.codespace) + } + valInfo := k.GetValidatorDistInfo(ctx, operatorAddr) wc := k.GetWithdrawContext(ctx, operatorAddr) valInfo, feePool := valInfo.TakeFeePoolRewards(wc) k.SetFeePool(ctx, feePool) k.SetValidatorDistInfo(ctx, valInfo) + + return nil } // withdrawal all the validator rewards including the commission From e16c2031215f13f3d4b12ecabea82a79490bc7ff Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 4 Dec 2018 12:13:48 -0500 Subject: [PATCH 5/5] Fix linting --- x/distribution/keeper/validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/distribution/keeper/validator.go b/x/distribution/keeper/validator.go index 20ad86ca1e28..951ba280a584 100644 --- a/x/distribution/keeper/validator.go +++ b/x/distribution/keeper/validator.go @@ -96,7 +96,7 @@ func (k Keeper) GetValidatorAccum(ctx sdk.Context, operatorAddr sdk.ValAddress) // from a onValidatorModified hook. func (k Keeper) updateValidatorDistInfoFromPool(ctx sdk.Context, operatorAddr sdk.ValAddress) sdk.Error { if !k.HasValidatorDistInfo(ctx, operatorAddr) { - types.ErrNoValidatorDistInfo(k.codespace) + return types.ErrNoValidatorDistInfo(k.codespace) } valInfo := k.GetValidatorDistInfo(ctx, operatorAddr)