Skip to content

Commit

Permalink
feat!: add error handling to staking hooks (#9571)
Browse files Browse the repository at this point in the history 
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

- Adds error handling for staking hooks

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
  • Loading branch information
@fedekunze
fedekunze committed Jun 30, 2021
1 parent aef416f commit f5b11bc
Show file tree
Hide file tree
Showing 12 changed files with 205 additions and 108 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking Changes

* [\#9571](https://github.com/cosmos/cosmos-sdk/pull/9571) Implemented error handling for staking hooks, which now return an error on failure.
* [\#9427](https://github.com/cosmos/cosmos-sdk/pull/9427) Move simapp `FundAccount` and `FundModuleAccount` to `x/bank/testutil`
* (client/tx) [\#9421](https://github.com/cosmos/cosmos-sdk/pull/9421/) `BuildUnsignedTx`, `BuildSimTx`, `PrintUnsignedStdTx` functions are moved to
the Tx Factory as methods.
Expand Down
4 changes: 3 additions & 1 deletion simapp/export.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,9 @@ func (app *SimApp) prepForZeroHeightGenesis(ctx sdk.Context, jailAllowedAddrs []
feePool.CommunityPool = feePool.CommunityPool.Add(scraps...)
app.DistrKeeper.SetFeePool(ctx, feePool)

app.DistrKeeper.Hooks().AfterValidatorCreated(ctx, val.GetOperator())
if err := app.DistrKeeper.Hooks().AfterValidatorCreated(ctx, val.GetOperator()); err != nil {
panic(err)
}
return false
})

Expand Down
40 changes: 27 additions & 13 deletions x/distribution/keeper/hooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@ var _ stakingtypes.StakingHooks = Hooks{}
func (k Keeper) Hooks() Hooks { return Hooks{k} }

// initialize validator distribution record
func (h Hooks) AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) {
func (h Hooks) AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) error {
val := h.k.stakingKeeper.Validator(ctx, valAddr)
h.k.initializeValidator(ctx, val)
return nil
}

// AfterValidatorRemoved performs clean up after a validator is removed
func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr sdk.ValAddress) {
func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr sdk.ValAddress) error {
// fetch outstanding
outstanding := h.k.GetValidatorOutstandingRewardsCoins(ctx, valAddr)

Expand All @@ -47,7 +48,7 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr
withdrawAddr := h.k.GetDelegatorWithdrawAddr(ctx, accAddr)

if err := h.k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, withdrawAddr, coins); err != nil {
panic(err)
return err
}
}
}
Expand All @@ -73,35 +74,48 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr

// clear current rewards
h.k.DeleteValidatorCurrentRewards(ctx, valAddr)

return nil
}

// increment period
func (h Hooks) BeforeDelegationCreated(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
func (h Hooks) BeforeDelegationCreated(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) error {
val := h.k.stakingKeeper.Validator(ctx, valAddr)
h.k.IncrementValidatorPeriod(ctx, val)
_ = h.k.IncrementValidatorPeriod(ctx, val)
return nil
}

// withdraw delegation rewards (which also increments period)
func (h Hooks) BeforeDelegationSharesModified(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
func (h Hooks) BeforeDelegationSharesModified(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) error {
val := h.k.stakingKeeper.Validator(ctx, valAddr)
del := h.k.stakingKeeper.Delegation(ctx, delAddr, valAddr)

if _, err := h.k.withdrawDelegationRewards(ctx, val, del); err != nil {
panic(err)
return err
}

return nil
}

// create new delegation period record
func (h Hooks) AfterDelegationModified(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) {
func (h Hooks) AfterDelegationModified(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) error {
h.k.initializeDelegation(ctx, valAddr, delAddr)
return nil
}

// record the slash event
func (h Hooks) BeforeValidatorSlashed(ctx sdk.Context, valAddr sdk.ValAddress, fraction sdk.Dec) {
func (h Hooks) BeforeValidatorSlashed(ctx sdk.Context, valAddr sdk.ValAddress, fraction sdk.Dec) error {
h.k.updateValidatorSlashFraction(ctx, valAddr, fraction)
return nil
}

func (h Hooks) BeforeValidatorModified(_ sdk.Context, _ sdk.ValAddress) {}
func (h Hooks) AfterValidatorBonded(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress) {}
func (h Hooks) AfterValidatorBeginUnbonding(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress) {}
func (h Hooks) BeforeDelegationRemoved(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) {}
func (h Hooks) BeforeValidatorModified(_ sdk.Context, _ sdk.ValAddress) error { return nil }
func (h Hooks) AfterValidatorBonded(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress) error {
return nil
}
func (h Hooks) AfterValidatorBeginUnbonding(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress) error {
return nil
}
func (h Hooks) BeforeDelegationRemoved(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error {
return nil
}
46 changes: 29 additions & 17 deletions x/slashing/keeper/hooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"github.com/cosmos/cosmos-sdk/x/slashing/types"
)

func (k Keeper) AfterValidatorBonded(ctx sdk.Context, address sdk.ConsAddress, _ sdk.ValAddress) {
func (k Keeper) AfterValidatorBonded(ctx sdk.Context, address sdk.ConsAddress, _ sdk.ValAddress) error {
// Update the signing info start height or create a new signing info
_, found := k.GetValidatorSigningInfo(ctx, address)
if !found {
Expand All @@ -23,6 +23,8 @@ func (k Keeper) AfterValidatorBonded(ctx sdk.Context, address sdk.ConsAddress, _
)
k.SetValidatorSigningInfo(ctx, address, signingInfo)
}

return nil
}

// AfterValidatorCreated adds the address-pubkey relation when a validator is created.
Expand All @@ -32,14 +34,14 @@ func (k Keeper) AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) e
if err != nil {
return err
}
k.AddPubkey(ctx, consPk)

return nil
return k.AddPubkey(ctx, consPk)
}

// AfterValidatorRemoved deletes the address-pubkey relation when a validator is removed,
func (k Keeper) AfterValidatorRemoved(ctx sdk.Context, address sdk.ConsAddress) {
func (k Keeper) AfterValidatorRemoved(ctx sdk.Context, address sdk.ConsAddress) error {
k.deleteAddrPubkeyRelation(ctx, crypto.Address(address))
return nil
}

// Hooks wrapper struct for slashing keeper
Expand All @@ -55,24 +57,34 @@ func (k Keeper) Hooks() Hooks {
}

// Implements sdk.ValidatorHooks
func (h Hooks) AfterValidatorBonded(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
h.k.AfterValidatorBonded(ctx, consAddr, valAddr)
func (h Hooks) AfterValidatorBonded(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) error {
return h.k.AfterValidatorBonded(ctx, consAddr, valAddr)
}

// Implements sdk.ValidatorHooks
func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, consAddr sdk.ConsAddress, _ sdk.ValAddress) {
h.k.AfterValidatorRemoved(ctx, consAddr)
func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, consAddr sdk.ConsAddress, _ sdk.ValAddress) error {
return h.k.AfterValidatorRemoved(ctx, consAddr)
}

// Implements sdk.ValidatorHooks
func (h Hooks) AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) {
h.k.AfterValidatorCreated(ctx, valAddr)
func (h Hooks) AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) error {
return h.k.AfterValidatorCreated(ctx, valAddr)
}

func (h Hooks) AfterValidatorBeginUnbonding(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress) {}
func (h Hooks) BeforeValidatorModified(_ sdk.Context, _ sdk.ValAddress) {}
func (h Hooks) BeforeDelegationCreated(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) {}
func (h Hooks) BeforeDelegationSharesModified(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) {}
func (h Hooks) BeforeDelegationRemoved(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) {}
func (h Hooks) AfterDelegationModified(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) {}
func (h Hooks) BeforeValidatorSlashed(_ sdk.Context, _ sdk.ValAddress, _ sdk.Dec) {}
func (h Hooks) AfterValidatorBeginUnbonding(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress) error {
return nil
}
func (h Hooks) BeforeValidatorModified(_ sdk.Context, _ sdk.ValAddress) error { return nil }
func (h Hooks) BeforeDelegationCreated(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error {
return nil
}
func (h Hooks) BeforeDelegationSharesModified(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error {
return nil
}
func (h Hooks) BeforeDelegationRemoved(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error {
return nil
}
func (h Hooks) AfterDelegationModified(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error {
return nil
}
func (h Hooks) BeforeValidatorSlashed(_ sdk.Context, _ sdk.ValAddress, _ sdk.Dec) error { return nil }
6 changes: 3 additions & 3 deletions x/slashing/types/expected_keepers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ type StakingKeeper interface {

// StakingHooks event hooks for staking validator object (noalias)
type StakingHooks interface {
AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) // Must be called when a validator is created
AfterValidatorRemoved(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) // Must be called when a validator is deleted
AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) error // Must be called when a validator is created
AfterValidatorRemoved(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) error // Must be called when a validator is deleted

AfterValidatorBonded(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) // Must be called when a validator is bonded
AfterValidatorBonded(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) error // Must be called when a validator is bonded
}
15 changes: 10 additions & 5 deletions x/staking/genesis.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package staking

import (
"fmt"
"log"

abci "github.com/tendermint/tendermint/abci/types"
tmtypes "github.com/tendermint/tendermint/types"
Expand Down Expand Up @@ -44,7 +43,9 @@ func InitGenesis(

// Call the creation hook if not exported
if !data.Exported {
keeper.AfterValidatorCreated(ctx, validator.GetOperator())
if err := keeper.AfterValidatorCreated(ctx, validator.GetOperator()); err != nil {
panic(err)
}
}

// update timeslice if necessary
Expand All @@ -70,13 +71,17 @@ func InitGenesis(

// Call the before-creation hook if not exported
if !data.Exported {
keeper.BeforeDelegationCreated(ctx, delegatorAddress, delegation.GetValidatorAddr())
if err := keeper.BeforeDelegationCreated(ctx, delegatorAddress, delegation.GetValidatorAddr()); err != nil {
panic(err)
}
}

keeper.SetDelegation(ctx, delegation)
// Call the after-modification hook if not exported
if !data.Exported {
keeper.AfterDelegationModified(ctx, delegatorAddress, delegation.GetValidatorAddr())
if err := keeper.AfterDelegationModified(ctx, delegatorAddress, delegation.GetValidatorAddr()); err != nil {
panic(err)
}
}
}

Expand Down Expand Up @@ -149,7 +154,7 @@ func InitGenesis(
var err error
res, err = keeper.ApplyAndReturnValidatorSetUpdates(ctx)
if err != nil {
log.Fatal(err)
panic(err)
}
}

Expand Down
32 changes: 24 additions & 8 deletions x/staking/keeper/delegation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,19 @@ func (k Keeper) SetDelegation(ctx sdk.Context, delegation types.Delegation) {
}

// remove a delegation
func (k Keeper) RemoveDelegation(ctx sdk.Context, delegation types.Delegation) {
func (k Keeper) RemoveDelegation(ctx sdk.Context, delegation types.Delegation) error {
delegatorAddress, err := sdk.AccAddressFromBech32(delegation.DelegatorAddress)
if err != nil {
panic(err)
}
// TODO: Consider calling hooks outside of the store wrapper functions, it's unobvious.
k.BeforeDelegationRemoved(ctx, delegatorAddress, delegation.GetValidatorAddr())
if err := k.BeforeDelegationRemoved(ctx, delegatorAddress, delegation.GetValidatorAddr()); err != nil {
return err
}

store := ctx.KVStore(k.storeKey)
store.Delete(types.GetDelegationKey(delegatorAddress, delegation.GetValidatorAddr()))
return nil
}

// return a given amount of all the delegator unbonding-delegations
Expand Down Expand Up @@ -563,9 +567,13 @@ func (k Keeper) Delegate(

// call the appropriate hook if present
if found {
k.BeforeDelegationSharesModified(ctx, delAddr, validator.GetOperator())
err = k.BeforeDelegationSharesModified(ctx, delAddr, validator.GetOperator())
} else {
k.BeforeDelegationCreated(ctx, delAddr, validator.GetOperator())
err = k.BeforeDelegationCreated(ctx, delAddr, validator.GetOperator())
}

if err != nil {
return sdk.ZeroDec(), err
}

delegatorAddress, err := sdk.AccAddressFromBech32(delegation.DelegatorAddress)
Expand Down Expand Up @@ -621,7 +629,9 @@ func (k Keeper) Delegate(
k.SetDelegation(ctx, delegation)

// Call the after-modification hook
k.AfterDelegationModified(ctx, delegatorAddress, delegation.GetValidatorAddr())
if err := k.AfterDelegationModified(ctx, delegatorAddress, delegation.GetValidatorAddr()); err != nil {
return newShares, err
}

return newShares, nil
}
Expand All @@ -637,7 +647,9 @@ func (k Keeper) Unbond(
}

// call the before-delegation-modified hook
k.BeforeDelegationSharesModified(ctx, delAddr, valAddr)
if err := k.BeforeDelegationSharesModified(ctx, delAddr, valAddr); err != nil {
return amount, err
}

// ensure that we have enough shares to remove
if delegation.Shares.LT(shares) {
Expand Down Expand Up @@ -670,11 +682,15 @@ func (k Keeper) Unbond(

// remove the delegation
if delegation.Shares.IsZero() {
k.RemoveDelegation(ctx, delegation)
err = k.RemoveDelegation(ctx, delegation)
} else {
k.SetDelegation(ctx, delegation)
// call the after delegation modification hook
k.AfterDelegationModified(ctx, delegatorAddress, delegation.GetValidatorAddr())
err = k.AfterDelegationModified(ctx, delegatorAddress, delegation.GetValidatorAddr())
}

if err != nil {
return amount, err
}

// remove the shares and coins from the validator
Expand Down
Loading

0 comments on commit f5b11bc

Please sign in to comment.