Clean up the Marker's expected BankKeeper interface. (#1954)

* [1760]: Clean up the marker module's BankKeeper expected keeper.

* [1760]: For getAllMarkerHolders, use the bank keeper's DenomOwners query directly (instead of marker's Holding query) because the marker's Holding query fails if the marker doesn't exist, which we don't care about here.

* [1760]: Add changelog entry.

CHANGELOG.md

@@ -72,12 +72,13 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * Switch to InputOutputCoinsProv for exchange transfers [#1930](https://github.com/provenance-io/provenance/pull/1930).
 * Use fields of the SimulationState for the encoders needed for simulations [#1931](https://github.com/provenance-io/provenance/pull/1931).
 * Removes sync-info code for sdk v0.50 [#1760](https://github.com/provenance-io/provenance/issues/1760).
-* Fix most of the failing unit tests [#1943](https://github.com/provenance-io/provenance/pull/1943)
+* Fix most of the failing unit tests [#1943](https://github.com/provenance-io/provenance/pull/1943).
 * Clean up ReadFromClient [#1760](https://github.com/provenance-io/provenance/issues/1760).
 * Remove all `GetSigners()` methods [#1957](https://github.com/provenance-io/provenance/pull/1957).
 * Ensure all `Msg`s have correctly identified `signer` fields [#1957](https://github.com/provenance-io/provenance/pull/1957).
 * Clean up all the module codecs [#1957](https://github.com/provenance-io/provenance/pull/1957).
 * Switch to auto-generated `String` and `Equal` methods for most proto messages [#1957](https://github.com/provenance-io/provenance/pull/1957).
+* Clean up the marker module's expected BankKeeper interface [#1954](https://github.com/provenance-io/provenance/pull/1954).
 
 ### Dependencies
 

x/marker/keeper/keeper_test.go

@@ -11,13 +11,14 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	abci "github.com/cometbft/cometbft/abci/types"
+
 	sdkmath "cosmossdk.io/math"
 	"cosmossdk.io/x/feegrant"
 
-	abci "github.com/cometbft/cometbft/abci/types"
-
 	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/types/query"
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 	"github.com/cosmos/cosmos-sdk/x/bank/testutil"
 	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
@@ -41,6 +42,25 @@ func setNewAccount(app *simapp.App, ctx sdk.Context, acc sdk.AccountI) sdk.Accou
 	return newAcc
 }
 
+// getAllMarkerHolders gets all the accounts holding a given denom, and the amount they each hold.
+func getAllMarkerHolders(t *testing.T, ctx context.Context, app *simapp.App, denom string) []types.Balance {
+	req := &banktypes.QueryDenomOwnersRequest{
+		Denom:      denom,
+		Pagination: &query.PageRequest{Limit: 10000},
+	}
+	resp, err := app.BankKeeper.DenomOwners(ctx, req)
+	require.NoError(t, err, "BankKeeper.DenomOwners(%q)", denom)
+	if len(resp.DenomOwners) == 0 {
+		return nil
+	}
+	rv := make([]types.Balance, len(resp.DenomOwners))
+	for i, owner := range resp.DenomOwners {
+		rv[i].Address = owner.Address
+		rv[i].Coins = sdk.Coins{owner.Balance}
+	}
+	return rv
+}
+
 func TestAccountMapperGetSet(t *testing.T) {
 	app := simapp.Setup(t)
 	ctx := app.BaseApp.NewContext(false)
@@ -87,7 +107,7 @@ func TestAccountMapperGetSet(t *testing.T) {
 	acc = app.AccountKeeper.GetAccount(ctx, addr)
 	require.Nil(t, acc)
 
-	require.Empty(t, app.MarkerKeeper.GetAllMarkerHolders(ctx, "testcoin"))
+	require.Empty(t, getAllMarkerHolders(t, ctx, app, "testcoin"))
 
 	// check for error on invaid marker denom
 	_, err := app.MarkerKeeper.GetMarkerByDenom(ctx, "doesntexist")
@@ -415,13 +435,13 @@ func TestMintBurnCoins(t *testing.T) {
 	require.Error(t, app.MarkerKeeper.CancelMarker(ctx, user, "testcoin"))
 
 	// two a user and the marker
-	require.Equal(t, 2, len(app.MarkerKeeper.GetAllMarkerHolders(ctx, "testcoin")))
+	require.Equal(t, 2, len(getAllMarkerHolders(t, ctx, app, "testcoin")))
 
 	// put the coins back in the types.
 	require.NoError(t, app.BankKeeper.SendCoins(ctx, user, addr, sdk.NewCoins(sdk.NewInt64Coin("testcoin", 50))))
 
 	// one, only the marker
-	require.Equal(t, 1, len(app.MarkerKeeper.GetAllMarkerHolders(ctx, "testcoin")))
+	require.Equal(t, 1, len(getAllMarkerHolders(t, ctx, app, "testcoin")))
 
 	// succeeds because marker has all its supply
 	require.NoError(t, app.MarkerKeeper.CancelMarker(ctx, user, "testcoin"))
@@ -446,7 +466,7 @@ func TestMintBurnCoins(t *testing.T) {
 	require.NoError(t, app.MarkerKeeper.DeleteMarker(ctx, user, "testcoin"))
 
 	// none, marker has been deleted
-	require.Equal(t, 0, len(app.MarkerKeeper.GetAllMarkerHolders(ctx, "testcoin")))
+	require.Equal(t, 0, len(getAllMarkerHolders(t, ctx, app, "testcoin")))
 
 	// verify status is destroyed and supply is zero.
 	m, err = app.MarkerKeeper.GetMarker(ctx, addr)

x/marker/keeper/marker.go

@@ -16,24 +16,6 @@ import (
 	"github.com/provenance-io/provenance/x/marker/types"
 )
 
-// GetAllMarkerHolders returns an array of all account addresses holding the given denom (and the amount)
-func (k Keeper) GetAllMarkerHolders(ctx sdk.Context, denom string) []types.Balance {
-	defer telemetry.MeasureSince(time.Now(), types.ModuleName, "get_all_marker_holders")
-
-	var results []types.Balance
-	k.bankKeeper.IterateAllBalances(ctx, func(addr sdk.AccAddress, coin sdk.Coin) (stop bool) {
-		if coin.Denom == denom && !coin.Amount.IsZero() {
-			results = append(results,
-				types.Balance{
-					Address: addr.String(),
-					Coins:   sdk.NewCoins(coin),
-				})
-		}
-		return false // do not stop iterating
-	})
-	return results
-}
-
 // GetMarkerByDenom looks up marker with the given denom
 func (k Keeper) GetMarkerByDenom(ctx sdk.Context, denom string) (types.MarkerAccountI, error) {
 	defer telemetry.MeasureSince(time.Now(), types.ModuleName, "get_marker_by_denom")

x/marker/types/expected_keepers.go

@@ -49,14 +49,6 @@ type BankKeeper interface {
 
 	GetDenomMetaData(context context.Context, denom string) (banktypes.Metadata, bool)
 	SetDenomMetaData(context context.Context, denomMetaData banktypes.Metadata)
-
-	// TODO[1760]: bank: Delete the below entries when no longer needed (or change this back to a regular TODO).
-
-	// IterateAllBalances only used in GetAllMarkerHolders used by the unneeded querier.
-	// The Holding query just uses the DenomOwners query endpoint.
-	IterateAllBalances(context context.Context, cb func(address sdk.AccAddress, coin sdk.Coin) (stop bool))
-	// GetAllSendEnabledEntries only needed by RemoveIsSendEnabledEntries in the quicksilver upgrade.
-	GetAllSendEnabledEntries(context context.Context) []banktypes.SendEnabled
 }
 
 // FeeGrantKeeper defines the fee-grant functionality needed by the marker module.

x/marker/types/marker_test.go

@@ -81,13 +81,6 @@ func TestNewEmptyMarkerValidate(t *testing.T) {
 		m.SetSupply(sdk.NewInt64Coin("other", 1000)), "expected failure setting supply to invalid denom of coin")
 	require.EqualValues(t, m.GetSupply(), sdk.NewInt64Coin("test", 1), "supply should be still be one")
 
-	// TODO[1760]: yaml: Decide if we still want this yaml convertion tested.
-	/*
-		yaml, merr := m.MarshalYAML()
-		require.NoError(t, merr, "marshall of yaml should succeed without error")
-		require.Equal(t, yaml, m.String(), "should use yaml for string() view")
-	*/
-
 	if err != nil {
 		t.Fatalf("expect no errors from marker validation: %s", err)
 	}