Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions

x/pse/keeper/hooks.go line 22 at r1 (raw file):

var _ stakingtypes.StakingHooks = Hooks{}

// Hooks Create new distribution hooks.

nit: staking/pse hooks

x/pse/keeper/hooks.go line 74 at r1 (raw file):

// BeforeDelegationRemoved implements the staking hooks interface.
func (h Hooks) BeforeDelegationRemoved(_ context.Context, _ sdk.AccAddress, _ sdk.ValAddress) error {
	return nil

If a user undeleagte/unbond a previously staked value, don't we need to deduct the unbonded value's corresponding score?
This is crucial for redelegation as well, otherwise the score will be given Twice (for a certain user, The first delegation score, and if we do not finalize the score in the removal, we will add the redelegated amount again in the `AfterDelegationModified.)

x/pse/keeper/hooks.go line 103 at r1 (raw file):

// BeforeValidatorSlashed implements the staking hooks interface.
func (h Hooks) BeforeValidatorSlashed(ctx context.Context, valAddr sdk.ValAddress, fraction sdkmath.LegacyDec) error {
	return nil

In case of validators slashing, the score remains the same and the user won't get any loss, is it the current design? if it is the case, we need to track the scores per-user-per-validator to be able to normalize the scores after slashing.

@ysv reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @masihyeganeh and @miladz68)

x/pse/keeper/hooks.go line 28 at r1 (raw file):

// AfterDelegationModified implements the staking hooks interface.
func (h Hooks) AfterDelegationModified(ctx context.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) error {

to make sure that logic is correct would be nice to understand in which cases all these hooks are triggered.
If AfterDelegationModified is called for any delegation change then impl is correct.

Is it called also when: delegation is created, deleted ?

x/pse/keeper/hooks.go line 50 at r1 (raw file):

	}

	oldScore, err := h.k.AccountScore.Get(ctx, delAddr)

I'm thinking if AccountScore is the best name here.
Because I would expect AccountScore query to return live score so maybe it makes sense to differentiated these 2 inside naming also.

My proposals:

• AccountScoreSnapshot
• AccountCheckpointScore
• AccountCommitedScore

And then full formula is:
AccountScore = AccountScoreSnapshot + SUM(Score(DelegationTimeEntry))

Because currently we probably use AccountScore name for both AccountScore & AccountScoreSnapshot

x/pse/keeper/hooks.go line 50 at r1 (raw file):

	}

	oldScore, err := h.k.AccountScore.Get(ctx, delAddr)

nit: I would change old -> last/previous

x/pse/keeper/hooks.go line 59 at r1 (raw file):

	oldDelegatedTokens := val.TokensFromShares(delegationTimeEntry.Shares).TruncateInt()
	delegationDuration := blockTimeUnixSeconds - delegationTimeEntry.LastChangedUnixSec
	addedScore := oldDelegatedTokens.MulRaw(delegationDuration)

nit: not critical but ideas for better name: delegationScore, currentDelegationScore, scoreFromDelegation

x/pse/keeper/hooks.go line 64 at r1 (raw file):

	if err := h.k.SetDelegationTimeEntry(ctx, valAddr, delAddr, types.DelegationTimeEntry{
		LastChangedUnixSec: blockTimeUnixSeconds,
		Shares:             delegation.Shares,

what if delegation.Shares == 0, shouldn't we skip SetDelegationTimeEntry in this case ?

x/pse/keeper/hooks.go line 102 at r1 (raw file):

// BeforeValidatorSlashed implements the staking hooks interface.
func (h Hooks) BeforeValidatorSlashed(ctx context.Context, valAddr sdk.ValAddress, fraction sdkmath.LegacyDec) error {

not sure if validator slashing is handled correctly because:

before slashing: 10 ucore
after slashing: 9 ucore

if we use 10ucore * period - it will be incorrect
if we use 9ucore * period - also incorrect

full formula should be 10ucore * period_1 + 9ucore * period_2.

However, if this is complicated to handle we can shortcut now, leave TODO and postpone

x/pse/keeper/hooks_test.go line 50 at r1 (raw file):

		sdk.NewInt64Coin(sdk.DefaultBondDenom, 11),
	))
	ctx, _, err = testApp.BeginNextBlockAtTime(ctx.BlockTime().Add(time.Second * 8))

nit: maybe more realistic would be to call BeginNextBlock(+time.Second) 8 times but this is really minor

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)

x/pse/keeper/hooks.go line 22 at r1 (raw file):

Done.

x/pse/keeper/hooks.go line 28 at r1 (raw file):

This hook is called at creation and modifican(partial redelegation and partial unbond).

x/pse/keeper/hooks.go line 50 at r1 (raw file):

Adopted AccountScoreSnapshot

x/pse/keeper/hooks.go line 50 at r1 (raw file):

Done.

x/pse/keeper/hooks.go line 59 at r1 (raw file):

Done.

x/pse/keeper/hooks.go line 64 at r1 (raw file):

that logic will not rise, since delegation can be zero only in case DelegationRemoval which is a different hook.

x/pse/keeper/hooks.go line 74 at r1 (raw file):

I am not fully understanding the scenario. we should discuss in call.

x/pse/keeper/hooks.go line 102 at r1 (raw file):

you are correct, we are using the second approach which is not super accurate, and yes, implementing the most accurate solution is hard. I will leave a TODO

x/pse/keeper/hooks.go line 103 at r1 (raw file):

take a look at the discussion from yaroslav, I answered our approach there. let's discuss it in the call if you still have questions/suggestions.

x/pse/keeper/hooks_test.go line 50 at r1 (raw file):

I think the current approach is good enough, but it should be easy to change it if you think it is needed.

Reviewable status: 3 of 8 files reviewed, 7 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @ysv)

x/pse/keeper/hooks.go line 74 at r1 (raw file):

I am asking about this special scenario, which user will unblond/undelegate the entire delegation, means the shares become zero. please look into the this line, the delegation will be remove from staking module state but AfterDelegationModified will not be fired according to the staking module's logic. instead the other hooks BeforeDelegationRemoved will be called here, so we need to handle that scenario and subtract the score accordingly.

Let's discuss in the call.

Reviewable status: 3 of 8 files reviewed, 7 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)

x/pse/keeper/hooks.go line 74 at r1 (raw file):

Sure, there is a separte task to handle it.

@TxCorpi0x reviewed 3 of 6 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @masihyeganeh and @ysv)

@ysv reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh and @TxCorpi0x)

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)