diff --git a/x/incentives/keeper/distribute.go b/x/incentives/keeper/distribute.go index a24d787ddd2..77de5b1bcb6 100644 --- a/x/incentives/keeper/distribute.go +++ b/x/incentives/keeper/distribute.go @@ -374,7 +374,7 @@ func (k Keeper) doDistributionSends(ctx sdk.Context, distrs *distributionInfo) e // the distrInfo struct. It also updates the gauge for the distribution. // locks is expected to be the correct set of lock recipients for this gauge. func (k Keeper) distributeSyntheticInternal( - ctx sdk.Context, gauge types.Gauge, locks []lockuptypes.PeriodLock, distrInfo *distributionInfo, + ctx sdk.Context, gauge types.Gauge, locks []*lockuptypes.PeriodLock, distrInfo *distributionInfo, ) (sdk.Coins, error) { qualifiedLocks := k.lk.GetLocksLongerThanDurationDenom(ctx, gauge.DistributeTo.Denom, gauge.DistributeTo.Duration) @@ -398,12 +398,17 @@ func (k Keeper) distributeSyntheticInternal( } } - sortedAndTrimmedQualifiedLocks := make([]lockuptypes.PeriodLock, curIndex) + sortedAndTrimmedQualifiedLocks := make([]*lockuptypes.PeriodLock, curIndex) + // This is not an issue because we directly + // use v.index and &v.locks. However, we must be careful not to + // take the address of &v. + // nolint: exportloopref for _, v := range qualifiedLocksMap { + v := v if v.index < 0 { continue } - sortedAndTrimmedQualifiedLocks[v.index] = v.lock + sortedAndTrimmedQualifiedLocks[v.index] = &v.lock } return k.distributeInternal(ctx, gauge, sortedAndTrimmedQualifiedLocks, distrInfo) @@ -556,7 +561,7 @@ func (k Keeper) syncVolumeSplitGroup(ctx sdk.Context, group types.Group) error { // // CONTRACT: gauge passed in as argument must be an active gauge. func (k Keeper) distributeInternal( - ctx sdk.Context, gauge types.Gauge, locks []lockuptypes.PeriodLock, distrInfo *distributionInfo, + ctx sdk.Context, gauge types.Gauge, locks []*lockuptypes.PeriodLock, distrInfo *distributionInfo, ) (sdk.Coins, error) { totalDistrCoins := sdk.NewCoins() @@ -757,10 +762,10 @@ func (k Keeper) handleGroupPostDistribute(ctx sdk.Context, groupGauge types.Gaug } // getDistributeToBaseLocks takes a gauge along with cached period locks by denom and returns locks that must be distributed to -func (k Keeper) getDistributeToBaseLocks(ctx sdk.Context, gauge types.Gauge, cache map[string][]lockuptypes.PeriodLock) []lockuptypes.PeriodLock { +func (k Keeper) getDistributeToBaseLocks(ctx sdk.Context, gauge types.Gauge, cache map[string][]lockuptypes.PeriodLock) []*lockuptypes.PeriodLock { // if gauge is empty, don't get the locks if gauge.Coins.Empty() { - return []lockuptypes.PeriodLock{} + return []*lockuptypes.PeriodLock{} } // Confusingly, there is no way to get all synthetic lockups. Thus we use a separate method `distributeSyntheticInternal` to separately get lockSum for synthetic lockups. // All gauges have a precondition of being ByDuration. diff --git a/x/incentives/keeper/iterator.go b/x/incentives/keeper/iterator.go index 4192b4f7749..649fca91c68 100644 --- a/x/incentives/keeper/iterator.go +++ b/x/incentives/keeper/iterator.go @@ -63,11 +63,11 @@ func (k Keeper) FinishedGaugesIterator(ctx sdk.Context) sdk.Iterator { } // FilterLocksByMinDuration returns locks whose lock duration is greater than the provided minimum duration. -func FilterLocksByMinDuration(locks []lockuptypes.PeriodLock, minDuration time.Duration) []lockuptypes.PeriodLock { - filteredLocks := make([]lockuptypes.PeriodLock, 0, len(locks)) - for _, lock := range locks { - if lock.Duration >= minDuration { - filteredLocks = append(filteredLocks, lock) +func FilterLocksByMinDuration(locks []lockuptypes.PeriodLock, minDuration time.Duration) []*lockuptypes.PeriodLock { + filteredLocks := make([]*lockuptypes.PeriodLock, 0, len(locks)) + for i := range locks { + if locks[i].Duration >= minDuration { + filteredLocks = append(filteredLocks, &locks[i]) } } return filteredLocks diff --git a/x/incentives/keeper/iterator_test.go b/x/incentives/keeper/iterator_test.go new file mode 100644 index 00000000000..fe008a80c13 --- /dev/null +++ b/x/incentives/keeper/iterator_test.go @@ -0,0 +1,37 @@ +package keeper_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/osmosis-labs/osmosis/v21/x/incentives/keeper" + lockuptypes "github.com/osmosis-labs/osmosis/v21/x/lockup/types" +) + +// This test validates that the FilterLocksByMinDuration function +// copies the correct locks when filtering. +// It helps to ensure that we do not use a wrong pointer by referencing +// a loop variable. +func TestFilterLocksByMinDuration(t *testing.T) { + const ( + numLocks = 3 + minDuration = 2 + ) + + locks := make([]lockuptypes.PeriodLock, numLocks) + for i := 0; i < numLocks; i++ { + locks[i] = lockuptypes.PeriodLock{ + ID: uint64(i + 1), + Duration: minDuration, + } + } + + filteredLocks := keeper.FilterLocksByMinDuration(locks, minDuration) + + require.Equal(t, len(locks), len(filteredLocks)) + + for i := 0; i < len(locks); i++ { + require.Equal(t, locks[i].ID, filteredLocks[i].ID) + } +} diff --git a/x/lockup/types/lock.go b/x/lockup/types/lock.go index ad1d5006297..b15d2e7f07d 100644 --- a/x/lockup/types/lock.go +++ b/x/lockup/types/lock.go @@ -65,7 +65,7 @@ func (p PeriodLock) SingleCoin() (sdk.Coin, error) { // TODO: Can we use sumtree instead here? // Assumes that caller is passing in locks that contain denom -func SumLocksByDenom(locks []PeriodLock, denom string) osmomath.Int { +func SumLocksByDenom(locks []*PeriodLock, denom string) osmomath.Int { sumBi := big.NewInt(0) // validate the denom once, so we can avoid the expensive validate check in the hot loop. err := sdk.ValidateDenom(denom)