feat: unfork cosmos sdk

[kien6034]

Closes #5

• [x]Closes Config cache size #14
• [x]Closes Move blockHeightMiddleware to core #13
• [x]Closes [cosmos-sdk](x/staking/delegation) limit voting power logic #19
• [x]Closes [cosmos-sdk] client/prunning #20

[StrathCole]

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?

[kien6034]

The reason why i don't use those hooks is because the hooks only provide us with this context (ctx sdk.Context, _ sdk.AccAddress, valAddr sdk.ValAddress), which is not-sufficient to perform the pre-check behavior like in the current forks. (we need the types.Msg... to get the required information)

[StrathCole]

Thanks for the clarification.

[StrathCole]

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.

[kien6034]

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

[StrathCole]

Do you also need to handle the event of creating a new validator?

[kien6034]

@StrathCole the hook AfterDelegationModified is already embedded in the internal keeper.Delegate function, which is called by the CreateValidator function

[StrathCole]

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.

[StrathCole]

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()),
)

[kien6034]

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

[StrathCole]

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.

[StrathCole]

given the nature of the hook, it might make sense to put it first (unless you would merge it with the distrhooks anyway).

[kien6034]

agree on this

[StrathCole]

You're having the skip-mev repo as dependencies in this PR

[kien6034]

i think because this branch is base off the feat/unfork-cosmos-sdk that @hoank101 and @TropicalDog17 is working on rightnow.
@hoank101 @TropicalDog17 can you check this for me?

[hoank101]

hi @kien6034 can you resolve conflict