fix: general oracle improvements and ABCI determinism (#1391)

* oracle: audit updates

* use GetBondedValidatorsByPower

* rename oracle test_utils.go to utils_test.go

* fix tests

Co-authored-by: Adam Wozniak <29418299+adamewozniak@users.noreply.github.com>

x/oracle/abci.go

@@ -3,15 +3,15 @@ package oracle
 import (
 	"time"
 
-	"github.com/umee-network/umee/v3/x/oracle/keeper"
-	"github.com/umee-network/umee/v3/x/oracle/types"
-
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	"github.com/umee-network/umee/v3/x/oracle/keeper"
+	"github.com/umee-network/umee/v3/x/oracle/types"
 )
 
-// IsPeriodLastBlock returns true if we are at the last block of the period
-func IsPeriodLastBlock(ctx sdk.Context, blocksPerPeriod uint64) bool {
+// isPeriodLastBlock returns true if we are at the last block of the period
+func isPeriodLastBlock(ctx sdk.Context, blocksPerPeriod uint64) bool {
 	return (uint64(ctx.BlockHeight())+1)%blocksPerPeriod == 0
 }
 
@@ -20,26 +20,13 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) error {
 	defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker)
 
 	params := k.GetParams(ctx)
-	if IsPeriodLastBlock(ctx, params.VotePeriod) {
+	if isPeriodLastBlock(ctx, params.VotePeriod) {
 		// Build claim map over all validators in active set
 		validatorClaimMap := make(map[string]types.Claim)
-
-		maxValidators := k.StakingKeeper.MaxValidators(ctx)
-		iterator := k.StakingKeeper.ValidatorsPowerStoreIterator(ctx)
-		defer iterator.Close()
-
 		powerReduction := k.StakingKeeper.PowerReduction(ctx)
-
-		i := 0
-		for ; iterator.Valid() && i < int(maxValidators); iterator.Next() {
-			validator := k.StakingKeeper.Validator(ctx, iterator.Value())
-
-			// Exclude not bonded validator
-			if validator.IsBonded() {
-				valAddr := validator.GetOperator()
-				validatorClaimMap[valAddr.String()] = types.NewClaim(validator.GetConsensusPower(powerReduction), 0, 0, valAddr)
-				i++
-			}
+		for _, v := range k.StakingKeeper.GetBondedValidatorsByPower(ctx) {
+			addr := v.GetOperator()
+			validatorClaimMap[addr.String()] = types.NewClaim(v.GetConsensusPower(powerReduction), 0, 0, addr)
 		}
 
 		var (
@@ -52,17 +39,10 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) error {
 			voteTargetDenoms = append(voteTargetDenoms, v.BaseDenom)
 		}
 
-		// Clear all exchange rates
-		k.IterateExchangeRates(ctx, func(denom string, _ sdk.Dec) (stop bool) {
-			k.DeleteExchangeRate(ctx, denom)
-			return false
-		})
-
-		// Organize votes to ballot by denom
-		// NOTE: **Filter out inactive or jailed validators**
-		voteMap := k.OrganizeBallotByDenom(ctx, validatorClaimMap)
+		k.ClearExchangeRates(ctx)
 
-		ballotDenomSlice := types.BallotMapToSlice(voteMap)
+		// NOTE: it filters out inactive or jailed validators
+		ballotDenomSlice := k.OrganizeBallotByDenom(ctx, validatorClaimMap)
 
 		// Iterate through ballots and update exchange rates; drop if not enough votes have been achieved.
 		for _, ballotDenom := range ballotDenomSlice {
@@ -106,14 +86,14 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) error {
 
 	// Slash oracle providers who missed voting over the threshold and
 	// reset miss counters of all validators at the last block of slash window
-	if IsPeriodLastBlock(ctx, params.SlashWindow) {
+	if isPeriodLastBlock(ctx, params.SlashWindow) {
 		k.SlashAndResetMissCounters(ctx)
 	}
 
 	return nil
 }
 
-// Tally calculates the median and returns it. It sets the set of voters to be
+// Tally calculates and returns the median. It sets the set of voters to be
 // rewarded, i.e. voted within a reasonable spread from the weighted median to
 // the store. Note, the ballot is sorted by ExchangeRate.
 func Tally(

x/oracle/keeper/ballot.go

@@ -13,8 +13,8 @@ import (
 func (k Keeper) OrganizeBallotByDenom(
 	ctx sdk.Context,
 	validatorClaimMap map[string]types.Claim,
-) (votes map[string]types.ExchangeRateBallot) {
-	votes = map[string]types.ExchangeRateBallot{}
+) []types.BallotDenom {
+	votes := map[string]types.ExchangeRateBallot{}
 
 	// collect aggregate votes
 	aggregateHandler := func(voterAddr sdk.ValAddress, vote types.AggregateExchangeRateVote) bool {
@@ -43,8 +43,7 @@ func (k Keeper) OrganizeBallotByDenom(
 		sort.Sort(ballot)
 		votes[denom] = ballot
 	}
-
-	return votes
+	return types.BallotMapToSlice(votes)
 }
 
 // ClearBallots clears all tallied prevotes and votes from the store.

x/oracle/keeper/ballot_test.go

@@ -7,12 +7,13 @@ import (
 )
 
 func (s *IntegrationTestSuite) TestBallot_OrganizeBallotByDenom() {
+	require := s.Require()
 	s.app.OracleKeeper.SetExchangeRate(s.ctx, displayDenom, sdk.OneDec())
 	claimMap := make(map[string]types.Claim)
 
 	// Empty Map
 	res := s.app.OracleKeeper.OrganizeBallotByDenom(s.ctx, claimMap)
-	s.Require().Equal(make(map[string]types.ExchangeRateBallot), res)
+	require.Empty(res)
 
 	s.app.OracleKeeper.SetAggregateExchangeRateVote(
 		s.ctx, valAddr, types.AggregateExchangeRateVote{
@@ -33,10 +34,10 @@ func (s *IntegrationTestSuite) TestBallot_OrganizeBallotByDenom() {
 		Recipient: valAddr,
 	}
 	res = s.app.OracleKeeper.OrganizeBallotByDenom(s.ctx, claimMap)
-	s.Require().Equal(map[string]types.ExchangeRateBallot{
-		"UMEE": {
-			types.NewVoteForTally(sdk.OneDec(), "UMEE", valAddr, 1),
-		},
+	require.Equal([]types.BallotDenom{
+		{
+			Ballot: types.ExchangeRateBallot{types.NewVoteForTally(sdk.OneDec(), "UMEE", valAddr, 1)},
+			Denom:  "UMEE"},
 	}, res)
 }
 

x/oracle/keeper/keeper.go

@@ -130,13 +130,6 @@ func (k Keeper) SetExchangeRateWithEvent(ctx sdk.Context, denom string, exchange
 		Denom: denom, Rate: exchangeRate})
 }
 
-// DeleteExchangeRate deletes the consensus exchange rate of USD denominated in
-// the denom asset from the store.
-func (k Keeper) DeleteExchangeRate(ctx sdk.Context, denom string) {
-	store := ctx.KVStore(k.storeKey)
-	store.Delete(types.GetExchangeRateKey(denom))
-}
-
 // IterateExchangeRates iterates over USD rates in the store.
 func (k Keeper) IterateExchangeRates(ctx sdk.Context, handler func(string, sdk.Dec) bool) {
 	store := ctx.KVStore(k.storeKey)
@@ -156,6 +149,15 @@ func (k Keeper) IterateExchangeRates(ctx sdk.Context, handler func(string, sdk.D
 	}
 }
 
+func (k Keeper) ClearExchangeRates(ctx sdk.Context) {
+	store := ctx.KVStore(k.storeKey)
+	iter := sdk.KVStorePrefixIterator(store, types.KeyPrefixExchangeRate)
+	defer iter.Close()
+	for ; iter.Valid(); iter.Next() {
+		store.Delete(iter.Key())
+	}
+}
+
 // GetFeederDelegation gets the account address to which the validator operator
 // delegated oracle vote rights.
 func (k Keeper) GetFeederDelegation(ctx sdk.Context, vAddr sdk.ValAddress) (sdk.AccAddress, error) {

x/oracle/keeper/keeper_test.go

@@ -267,11 +267,11 @@ func (s *IntegrationTestSuite) TestGetExchangeRateBase() {
 	s.Require().Equal(rate.Mul(power), sdk.OneDec())
 }
 
-func (s *IntegrationTestSuite) TestDeleteExchangeRate() {
+func (s *IntegrationTestSuite) TestClearExchangeRate() {
 	app, ctx := s.app, s.ctx
 
 	app.OracleKeeper.SetExchangeRate(ctx, displayDenom, sdk.OneDec())
-	app.OracleKeeper.DeleteExchangeRate(ctx, displayDenom)
+	app.OracleKeeper.ClearExchangeRates(ctx)
 	_, err := app.OracleKeeper.GetExchangeRate(ctx, displayDenom)
 	s.Require().Error(err)
 }

x/oracle/types/expected_keeper.go

@@ -12,6 +12,7 @@ import (
 // module.
 type StakingKeeper interface {
 	Validator(ctx sdk.Context, address sdk.ValAddress) stakingtypes.ValidatorI
+	GetBondedValidatorsByPower(ctx sdk.Context) []stakingtypes.Validator
 	TotalBondedTokens(sdk.Context) sdkmath.Int
 	Slash(sdk.Context, sdk.ConsAddress, int64, int64, sdk.Dec) sdkmath.Int
 	Jail(sdk.Context, sdk.ConsAddress)

x/oracle/types/test_utils.go → x/oracle/types/utils_test.go

@@ -123,6 +123,10 @@ func (MockStakingKeeper) TotalBondedTokens(_ sdk.Context) sdkmath.Int {
 	return sdk.ZeroInt()
 }
 
+func (k MockStakingKeeper) GetBondedValidatorsByPower(ctx sdk.Context) []stakingtypes.Validator {
+	return nil
+}
+
 func (MockStakingKeeper) ValidatorsPowerStoreIterator(ctx sdk.Context) sdk.Iterator {
 	return sdk.KVStoreReversePrefixIterator(nil, nil)
 }