feat: unfork cosmos sdk #17
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| package staking | ||
|
There was a problem hiding this comment. I am aware it is "clean" to create a full hook file. Given that only one function is used, would it make sense to create a simple hook file that inherits the distribution hooks file and in the function you alter first calls the inherited hook and then the additional code? This would remove one hook from the multihook list in the stakingkeeper. There was a problem hiding this comment. I meant like this: type TerraStakingHooks struct {
distrkeeper.Hooks
sk stakingkeeper.Keeper
}
func NewTerraStakingHooks(sk stakingkeeper.Keeper, distrHooks distrkeeper.Hooks) *TerraStakingHooks {
return &TerraStakingHooks{
Hooks: distrHooks,
sk: sk,
}
}
func (h TerraStakingHooks) AfterDelegationModified(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) error {
if err := h.Hooks.AfterDelegationModified(ctx, delAddr, valAddr); err != nil {
return err
}
/* your code from the power check in here */
return nil
}And in the app when setting up the hooks something like: appKeepers.StakingKeeper.SetHooks(
stakingtypes.NewMultiStakingHooks(customstaking.NewTerraStakingHooks(appKeepers.StakingKeeper, appKeepers.DistrKeeper.Hooks()), appKeepers.SlashingKeeper.Hooks()),
)There was a problem hiding this comment. the distribution hooks is used for supporting rewards operation stuff, and the hook that i used already have the implementation. Imo, i think keeping it as the separate file though make the code looks "long", but will be cleaner and easier to maintain in the future There was a problem hiding this comment. My only thought was to remove the small overhead of calling all the hook functions that aren't used anyway. But it's nothing I would insist in. |
||
|
|
||
| import ( | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" | ||
| stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" | ||
| ) | ||
|
|
||
| const ( | ||
| ColumbusChainID = "columbus-5" | ||
| ) | ||
|
|
||
| var _ stakingtypes.StakingHooks = &TerraStakingHooks{} | ||
|
|
||
| // TerraStakingHooks implements staking hooks to enforce validator power limit | ||
| type TerraStakingHooks struct { | ||
| sk stakingkeeper.Keeper | ||
| } | ||
|
|
||
| func NewTerraStakingHooks(sk stakingkeeper.Keeper) *TerraStakingHooks { | ||
| return &TerraStakingHooks{sk: sk} | ||
| } | ||
|
|
||
| // Implement required staking hooks interface methods | ||
| func (h TerraStakingHooks) BeforeDelegationCreated(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (h TerraStakingHooks) BeforeDelegationSharesModified(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error { | ||
| return nil | ||
| } | ||
|
|
||
| // Other required hook methods with empty implementations | ||
| func (h TerraStakingHooks) AfterDelegationModified(ctx sdk.Context, _ sdk.AccAddress, valAddr sdk.ValAddress) error { | ||
|
There was a problem hiding this comment. Would it make sense to modify and put the logic into the "BeforeDelegationCreated" and "BeforeDelegationSharesModified" functions using a helper function? This would allow an early exit like the current fork change does. Or is there a good reason to rather do it after? There was a problem hiding this comment. The reason why i don't use those hooks is because the hooks only provide us with this context |
||
| if ctx.ChainID() != ColumbusChainID { | ||
| return nil | ||
| } | ||
|
|
||
| validator, found := h.sk.GetValidator(ctx, valAddr) | ||
| if !found { | ||
| return nil | ||
| } | ||
|
|
||
| // Get validator's current power (after delegation modified) | ||
| validatorPower := sdk.TokensToConsensusPower(validator.Tokens, h.sk.PowerReduction(ctx)) | ||
|
|
||
| // Get the total power of the validator set | ||
| totalPower := h.sk.GetLastTotalPower(ctx) | ||
|
|
||
| // Get validator delegation percent | ||
| validatorDelegationPercent := sdk.NewDec(validatorPower).QuoInt64(totalPower.Int64()) | ||
|
|
||
| if validatorDelegationPercent.GT(sdk.NewDecWithPrec(20, 2)) { | ||
| panic("validator power is over the allowed limit") | ||
|
There was a problem hiding this comment. Would it be possible to use a normal error instead of a panic here? I am not 100% sure why a panic was used in the original code, maybe you can clarify on that although you didn't write it. There was a problem hiding this comment. if subtractAccount {
if tokenSrc == types.Bonded {
panic("delegation token source cannot be bonded")
}
var sendName string
switch {
case validator.IsBonded():
sendName = types.BondedPoolName
case validator.IsUnbonding(), validator.IsUnbonded():
sendName = types.NotBondedPoolName
default:
panic("invalid validator status")
}i think it align with the current checks logic. I think we should keep it as it is, but lets me find out if it is better to return err here There was a problem hiding this comment. Do you also need to handle the event of creating a new validator? There was a problem hiding this comment. @StrathCole the hook |
||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (h TerraStakingHooks) BeforeValidatorSlashed(_ sdk.Context, _ sdk.ValAddress, _ sdk.Dec) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (h TerraStakingHooks) BeforeValidatorModified(_ sdk.Context, _ sdk.ValAddress) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (h TerraStakingHooks) AfterValidatorBonded(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (h TerraStakingHooks) AfterValidatorBeginUnbonding(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (h TerraStakingHooks) AfterValidatorRemoved(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (h TerraStakingHooks) AfterUnbondingInitiated(_ sdk.Context, _ uint64) error { | ||
| return nil | ||
| } | ||
|
|
||
| // Add this method to TerraStakingHooks | ||
| func (h TerraStakingHooks) AfterValidatorCreated(_ sdk.Context, _ sdk.ValAddress) error { | ||
| return nil | ||
| } | ||
|
|
||
| // Add the missing method | ||
| func (h TerraStakingHooks) BeforeDelegationRemoved(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error { | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| package server | ||
|
|
||
| import ( | ||
| "net/http" | ||
| "strconv" | ||
|
|
||
| "github.com/cosmos/cosmos-sdk/codec/legacy" | ||
| grpctypes "github.com/cosmos/cosmos-sdk/types/grpc" | ||
| ) | ||
|
|
||
| // BlockHeightMiddleware parses height query parameter and sets GRPCBlockHeightHeader | ||
| func BlockHeightMiddleware(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| heightStr := r.FormValue("height") | ||
| if heightStr != "" { | ||
| height, err := strconv.ParseInt(heightStr, 10, 64) | ||
| if err != nil { | ||
| writeErrorResponse(w, http.StatusBadRequest, "syntax error") | ||
| return | ||
| } | ||
|
|
||
| if height < 0 { | ||
| writeErrorResponse(w, http.StatusBadRequest, "height must be equal or greater than zero") | ||
| return | ||
| } | ||
|
|
||
| if height > 0 { | ||
| r.Header.Set(grpctypes.GRPCBlockHeightHeader, heightStr) | ||
| } | ||
| } | ||
|
|
||
| next.ServeHTTP(w, r) | ||
| }) | ||
| } | ||
|
|
||
| // writeErrorResponse prepares and writes an HTTP error | ||
| func writeErrorResponse(w http.ResponseWriter, status int, err string) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(status) | ||
| _, _ = w.Write(legacy.Cdc.MustMarshalJSON(newErrorResponse(0, err))) | ||
| } | ||
|
|
||
| type errorResponse struct { | ||
| Code int `json:"code,omitempty"` | ||
| Error string `json:"error"` | ||
| } | ||
|
|
||
| func newErrorResponse(code int, err string) errorResponse { | ||
| return errorResponse{Code: code, Error: err} | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given the nature of the hook, it might make sense to put it first (unless you would merge it with the distrhooks anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree on this