From ffbb851e5b48faf05bc79413e3eae1c54bf2f375 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Mon, 13 May 2024 13:13:39 -0600 Subject: [PATCH] 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 | 3 ++- x/marker/keeper/keeper_test.go | 32 ++++++++++++++++++++++++------ x/marker/keeper/marker.go | 18 ----------------- x/marker/types/expected_keepers.go | 8 -------- x/marker/types/marker_test.go | 7 ------- 5 files changed, 28 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f66a7e58ee..cb6b862988 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/x/marker/keeper/keeper_test.go b/x/marker/keeper/keeper_test.go index eeaffa3cd2..afcb876d03 100644 --- a/x/marker/keeper/keeper_test.go +++ b/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) diff --git a/x/marker/keeper/marker.go b/x/marker/keeper/marker.go index 55e8b32388..bb7ebb56d6 100644 --- a/x/marker/keeper/marker.go +++ b/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") diff --git a/x/marker/types/expected_keepers.go b/x/marker/types/expected_keepers.go index 4f591b3029..828219d973 100644 --- a/x/marker/types/expected_keepers.go +++ b/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. diff --git a/x/marker/types/marker_test.go b/x/marker/types/marker_test.go index b47ba7654f..643a3f8826 100644 --- a/x/marker/types/marker_test.go +++ b/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) }