@ysv reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @metalarm10, @miladz68, and @TxCorpi0x).

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

		// Only remove snapshots for non-excluded addresses. Excluded addresses keep their snapshots
		// to maintain score history for potential future inclusion.
		if _, excluded := excludedAddrMap[delStr]; !excluded {

I tend to disagree with this fix.
In this case address for excluded address will keep increasing between multiple PSE distributions. And if such address is removed from exclude list it will have huge score which is sum of score for multiple PSE iterations.

I'm also not sure how this fixes bug which we had

@TxCorpi0x made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @metalarm10, @miladz68, and @ysv).

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

The fix is related to undelegation hook failure; there is a problem in the logic, it prevents undelagation command of staking module to fail because of missing item in snapshots.

But related to that problem you mentioned, Currently the scoreMap is being filtered and excluded addresses will be removed from score addition step. This behaviour is the existing behaviour in master branch.
Generally, i think we should prevent excluded addresses from receiving the score (current code) and we do not need to prevent already created score to be rewarded, in fact exclusion should happen only on score calculation, but while the distribution we should distribute already calculated score (even if it is in the list of excluded addresses now)

@ysv reviewed 1 file and all commit messages, made 4 comments, and resolved 1 discussion.
Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @masihyeganeh, @metalarm10, @miladz68, and @TxCorpi0x).

x/pse/keeper/hooks.go line 52 at r2 (raw file):

	}); err != nil {
		return err
	}

maybe we should move this to be executed after isExcludedAddress check ?

This way both DelegationTimeEntry and AccountScoreSnapshot will always be empty for excluded address.
Because I can see that you remove it in params.go

Code quote:

	// Update DelegationTimeEntry for all addresses (needed for delegation tracking)
	if err := h.k.SetDelegationTimeEntry(ctx, valAddr, delAddr, types.DelegationTimeEntry{
		LastChangedUnixSec: blockTimeUnixSeconds,
		Shares:             delegation.Shares,
	}); err != nil {
		return err
	}

x/pse/keeper/hooks.go line 63 at r2 (raw file):

	} else if isExcludedAddress(delAddr, params.ExcludedAddresses, h.k.addressCodec) {
		return nil
	}

maybe it makes sense to replace whole this block with single call to isExcludedAddress function ?
Like this:

if excluded, err := isExcludedAddress {
}

the main idea is to move GetParams & !errors.Is(err, collections.ErrNotFound) into isExcludedAddress also.

Code quote:

	params, err := h.k.GetParams(ctx)
	if err != nil {
		// During genesis, params might not be initialized yet - treat all as non-excluded
		if !errors.Is(err, collections.ErrNotFound) {
			return err
		}
	} else if isExcludedAddress(delAddr, params.ExcludedAddresses, h.k.addressCodec) {
		return nil
	}

x/pse/keeper/hooks.go line 93 at r2 (raw file):

	// Remove DelegationTimeEntry for all addresses
	if err := h.k.RemoveDelegationTimeEntry(ctx, valAddr, delAddr); err != nil {

should we make this removal optional ?
For cases when it is called for excluded address

integration-tests/modules/pse_test.go line 33 at r2 (raw file):

)

func TestPSEDistribution(t *testing.T) {

I need to review this test carefully one more time but also I suggest to test this differently.
Can we test this by querying user score ?
E.g

1. address has XX score
2. address added to excluded_list
3. query address => has 0 score
4. remove from exclude_list
5. query address (e.g next block after removed from exclude list) => score is credited again starting from 0

We can also query score for excluded address before/after distribution and it should always be 0

@ysv made 1 comment.
Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @masihyeganeh, @metalarm10, @miladz68, and @TxCorpi0x).

integration-tests/modules/pse_test.go line 33 at r2 (raw file):
maybe try to do this via unit-tests

1. address has XX score
2. address added to excluded_list
3. query address => has 0 score
4. remove from exclude_list
5. query address (e.g next block after removed from exclude list) => score is credited again starting from 0

@ysv made 1 comment.
Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @masihyeganeh, @metalarm10, @miladz68, and @TxCorpi0x).

integration-tests/modules/pse_test.go line 33 at r2 (raw file):

agreed to just add unit test for score queries for now

@TxCorpi0x made 4 comments.
Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @masihyeganeh, @metalarm10, @miladz68, and @ysv).

integration-tests/modules/pse_test.go line 33 at r2 (raw file):

Integration test updated for better readability,
Unit test Updated with the requested steps.

x/pse/keeper/hooks.go line 52 at r2 (raw file):

Done, Moved

x/pse/keeper/hooks.go line 63 at r2 (raw file):

Done
Defined method for keeper according to discussion.

x/pse/keeper/hooks.go line 93 at r2 (raw file):

It was unnecessary change, reverted

@ysv reviewed 3 files and all commit messages, made 3 comments, and resolved 4 discussions.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @masihyeganeh, @metalarm10, @miladz68, and @TxCorpi0x).

x/pse/keeper/distribute.go line 45 at r3 (raw file):

	// Clear all account score snapshots.
	// Excluded addresses should not have snapshots (cleared when added to exclusion list),
	// but we clear unconditionally for simplicity and safety.

nit: but we clear unconditionally for all addresses.

Code quote:

but we clear unconditionally for simplicity and safety

x/pse/keeper/hooks.go line 104 at r3 (raw file):

	newScore := lastScore.Add(addedScore)

	// Remove DelegationTimeEntry for all addresses, including excluded ones

comment is wrong including excluded ones

x/pse/keeper/params.go line 171 at r3 (raw file):

// IsExcludedAddress checks if the given address is in the excluded addresses list.
// Returns false if params are not initialized (e.g., during genesis).
func (k Keeper) IsExcludedAddress(ctx context.Context, addr sdk.AccAddress) bool {

In my opinion this method should return (bool, error)
Because there are a few errors which are outside of scope of this function and should be handled on higher level.

Usage I invision:

isExcluded, err := h.k.IsExcludedAddress(ctx, delAddr)
if err != nil {
return err 
}
if isExcluded {
  return nil 
}

@TxCorpi0x made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @masihyeganeh, @metalarm10, @miladz68, and @ysv).

x/pse/keeper/hooks.go line 104 at r3 (raw file):

Done.

x/pse/keeper/params.go line 171 at r3 (raw file):

Done.

@ysv reviewed 5 files and all commit messages, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh, @metalarm10, and @miladz68).