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

x/pse/types/expected_keepers.go line 13 at r1 (raw file):

// StakingQuerier interface.
type StakingQuerier interface {

nit: is it still querier, now it has write action Delegate

Code quote:

StakingQuerier

x/pse/keeper/distribute.go line 41 at r1 (raw file):

	}

	// add uncalculated score to account score snapshot and total score per delegator.

nit: I'm not sure if this comment clearly describes what is done in this loop.

if I understood correctly it: "adds last account score snapshot to corresponding delegator total score"

x/pse/keeper/distribute.go line 53 at r1 (raw file):

		}
		score := kv.Value
		dellAddr := kv.Key

nit: delAddr (single l) - delegator

Code quote:

dellAddr := kv.Key

x/pse/keeper/distribute.go line 108 at r1 (raw file):

		indexMap:     make(map[string]int),
		addressCodec: addressCodec,
		totalScore:   sdkmath.NewInt(0),

nit: another option is to have func TotalScore() sdkmath.Int as method of scoreMap. We can iterate through all numbers once and just sum them

It is matter of taste but I usually prefer to store 'source of truth data' only and calculate other on fly unless operation is too heavy/repeated.

But I'm Also ok with current version.

Code quote:

totalScore:   sdkmath.NewInt(0),

x/pse/keeper/distribute.go line 131 at r1 (raw file):

		m.items[idx].score = m.items[idx].score.Add(value)
	}
}

question:
I guess we store it as slice of struct instead of map because of map non-determinism which we faced before, right ?

Code quote:

	if !found {
		m.items = append(m.items, struct {
			addr  sdk.AccAddress
			score sdkmath.Int
		}{
			addr:  addr,
			score: value,
		})
		m.indexMap[key] = len(m.items) - 1
	} else {
		m.items[idx].score = m.items[idx].score.Add(value)
	}
}

x/pse/keeper/distribute.go line 182 at r1 (raw file):

	}
	for _, delegation := range delegations {
		delegationAmount := delegation.Balance.Amount.Mul(amount).Quo(totalDelegationAmount)

so we will have another rounding error here.
But I guess we can do nothing with it.

Also I've just realized that we can probably estimated rounding error worst case:

number_of_delegations * 1ucore (actually it is 0.99999...9ucore in the worst case)

lets say 50k addresses with 10 delegations each exist on-chain then rounding error is:
50_000*10 = 500_000 ucore = 0.5 CORE, which is acceptable IMO

x/pse/keeper/distribute.go line 182 at r1 (raw file):

	}
	for _, delegation := range delegations {
		delegationAmount := delegation.Balance.Amount.Mul(amount).Quo(totalDelegationAmount)

maybe worth adding comments about rounding error here and in another place

x/pse/keeper/distribute_test.go line 36 at r1 (raw file):

				func(r *runEnv) {
					assertDistributionAction(r, map[*sdk.AccAddress]sdkmath.Int{
						&r.delegators[0]: sdkmath.NewInt(1_100_366), // + 1000 * 1.1 m / 3 m

why do we divide by 3M here if we have 1.1m + 0.9m = 2m ?

Code quote:

/ 3 m

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

x/pse/keeper/distribute.go line 182 at r1 (raw file):

We can store the difference between actual Decimal value and the Truncated Int, and append it to the calculations in the next round of distribution, so we don't loose the precision incrementally (Imaging 100 distribution each with 0.5 precision error would become 50 Tokens )

x/pse/keeper/distribute.go line 60 at r1 (raw file):

	totalPSEScore := finalScoreMap.totalScore
	err = finalScoreMap.Walk(func(addr sdk.AccAddress, score sdkmath.Int) error {
		userAmount := totalPSEAmount.Mul(score).Quo(totalPSEScore)

Is it possible to have totalPSEScore as zero? we should take care of that division then (division by zero).

x/pse/keeper/distribute.go line 83 at r1 (raw file):

		err = k.DelegationTimeEntries.Set(ctx, key, types.DelegationTimeEntry{
			LastChangedUnixSec: blockTimeUnixSeconds,
			Shares:             sdkmath.LegacyNewDec(0),

Do we need to store the distribution history? if yes, we should move it to history and try to create a new record. so we can have the distribution history query endpoint instead of refreshing the `Shares

x/pse/keeper/distribute.go line 112 at r1 (raw file):

}

func (m *scoreMap) AddScore(addr sdk.AccAddress, value sdkmath.Int) {

We can check for zero value and return in the beginning of the method (prevent extra process?)

x/pse/keeper/distribute.go line 113 at r1 (raw file):

func (m *scoreMap) AddScore(addr sdk.AccAddress, value sdkmath.Int) {
	m.totalScore = m.totalScore.Add(value)

It is better to move this to the end of the function m.totalScore = m.totalScore.Add(value)

x/pse/keeper/distribute.go line 114 at r1 (raw file):

func (m *scoreMap) AddScore(addr sdk.AccAddress, value sdkmath.Int) {
	m.totalScore = m.totalScore.Add(value)
	key, err := m.addressCodec.BytesToString(addr)

can we use .String() of sdk.AccAddress and remove the empty return?

x/pse/keeper/distribute.go line 147 at r1 (raw file):

}

func (k Keeper) distributeToDelegator(ctx context.Context, delAddr sdk.AccAddress, amount sdkmath.Int) error {

We can also check the zero for the amount at the beginning

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

x/pse/keeper/distribute.go line 41 at r1 (raw file):

that is correct, I will add more context to the comment.

x/pse/keeper/distribute.go line 53 at r1 (raw file):

Done.

x/pse/keeper/distribute.go line 60 at r1 (raw file):

it is not practically possible. but I will add an in statement.

x/pse/keeper/distribute.go line 83 at r1 (raw file):

we don't need to store the history.

x/pse/keeper/distribute.go line 112 at r1 (raw file):

zero score does not see

x/pse/keeper/distribute.go line 113 at r1 (raw file):

what is the difference ?

x/pse/keeper/distribute.go line 114 at r1 (raw file):

.String is the old way, we should use the codec.

x/pse/keeper/distribute.go line 131 at r1 (raw file):

that is correct.

x/pse/keeper/distribute.go line 147 at r1 (raw file):

Done.

x/pse/keeper/distribute.go line 182 at r1 (raw file):

that is correct, the rounding errors are insignificant and can be ignored.

x/pse/keeper/distribute.go line 182 at r1 (raw file):

Done.

x/pse/keeper/distribute_test.go line 36 at r1 (raw file):

there is an existing validator from test app initiation with 1 m stake.

x/pse/types/expected_keepers.go line 13 at r1 (raw file):

well the delegate is a keeper action which exists on querier. I am not sure what to call it TBH.

Reviewable status: 2 of 6 files reviewed, 9 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @ysv)

x/pse/keeper/distribute.go line 112 at r1 (raw file):

If the value be zero, is it possible?

x/pse/keeper/distribute.go line 113 at r1 (raw file):

Just better readability, doing total at the end

x/pse/keeper/distribute.go line 114 at r1 (raw file):

Nice, So do we have any plan for changing in the rest of the modules?

Reviewable status: 2 of 6 files reviewed, 9 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)

x/pse/keeper/distribute.go line 112 at r1 (raw file):

I see my answer was incomplete, but I mean that zero score is not possible, but it does not hurt to add a check. Added.

x/pse/keeper/distribute.go line 113 at r1 (raw file):

Moved to the end.

x/pse/keeper/distribute.go line 114 at r1 (raw file):

I don't think that we do at the moment.

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

x/pse/keeper/distribute_test.go line 36 at r1 (raw file):

I see

@TxCorpi0x reviewed 2 of 6 files at r1, 3 of 4 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh)

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