PR feedback for #270"

proto/cosmos/bank/v1beta1/tx.proto

@@ -50,6 +50,7 @@ message MsgMultiSend {
 // MsgMultiSendResponse defines the Msg/MultiSend response type.
 message MsgMultiSendResponse {}
 
+// MsgUpdateDenomMetadata represents a message to update denom metadata
 message MsgUpdateDenomMetadata {
   option (cosmos.msg.v1.signer) = "from_address";
 

simapp/app.go

@@ -260,7 +260,6 @@ func NewSimApp(
 		appCodec, keys[authtypes.StoreKey], app.GetSubspace(authtypes.ModuleName), authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix,
 	)
 
-	// set the governance module account as the authority
 	app.BankKeeper = bankkeeper.NewBaseKeeper(
 		appCodec, keys[banktypes.StoreKey], app.AccountKeeper, app.GetSubspace(banktypes.ModuleName), app.ModuleAccountAddrs(),
 	)

x/bank/keeper/keeper.go

@@ -3,7 +3,6 @@ package keeper
 import (
 	"fmt"
 
-	govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
 	"cosmossdk.io/math"
 	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/internal/conv"
@@ -19,6 +18,8 @@ import (
 
 var _ Keeper = (*BaseKeeper)(nil)
 
+const govModuleName = "gov"
+
 // Keeper defines a module interface that facilitates the transfer of coins
 // between accounts.
 type Keeper interface {
@@ -62,7 +63,7 @@ type BaseKeeper struct {
 	storeKey               storetypes.StoreKey
 	paramSpace             paramtypes.Subspace
 	mintCoinsRestrictionFn MintingRestrictionFn
-	authority  			   string
+	authority              string
 }
 
 type MintingRestrictionFn func(ctx sdk.Context, coins sdk.Coins) error
@@ -117,7 +118,7 @@ func NewBaseKeeper(
 		storeKey:               storeKey,
 		paramSpace:             paramSpace,
 		mintCoinsRestrictionFn: func(ctx sdk.Context, coins sdk.Coins) error { return nil },
-		authority: 				authtypes.NewModuleAddress(govtypes.ModuleName).String(), // hardcoded till all transitive dependencies can be updated to pass in the constructor instead
+		authority:              authtypes.NewModuleAddress(govModuleName).String(), // hardcoded till all transitive dependencies can be updated to pass in the constructor instead
 	}
 }
 
@@ -558,4 +559,4 @@ func (k BaseViewKeeper) IterateTotalSupply(ctx sdk.Context, cb func(sdk.Coin) bo
 
 func (k BaseKeeper) GetAuthority() string {
 	return k.authority
-}
+}

x/bank/keeper/msg_server.go

@@ -2,7 +2,9 @@ package keeper
 
 import (
 	"context"
+
 	"github.com/armon/go-metrics"
+
 	"cosmossdk.io/errors"
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	sdk "github.com/cosmos/cosmos-sdk/types"

x/bank/keeper/msg_server_test.go

@@ -0,0 +1,74 @@
+package keeper_test
+
+import (
+	"github.com/cosmos/cosmos-sdk/simapp"
+	"github.com/cosmos/cosmos-sdk/x/bank/keeper"
+	"github.com/cosmos/cosmos-sdk/x/bank/types"
+	"github.com/stretchr/testify/require"
+	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
+	"testing"
+)
+
+func TestUpdateDenomMetadata(t *testing.T) {
+	app := simapp.Setup(t, false)
+	ctx := app.BaseApp.NewContext(false, tmproto.Header{})
+	msgServer := keeper.NewMsgServerImpl(app.BankKeeper)
+	testCases := []struct {
+		Name        string
+		ExpectErr   bool
+		ExpectedErr string
+		req         types.MsgUpdateDenomMetadata
+	}{
+		{
+			Name:        "fail - wrong authority",
+			ExpectErr:   true,
+			ExpectedErr: "expected cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn got nongovaccount: expected gov account as only signer for proposal message",
+			req: types.MsgUpdateDenomMetadata{
+				FromAddress: "nongovaccount",
+				Title:       "title",
+				Description: "description",
+				Metadata: &types.Metadata{
+					Name:        "diamondback",
+					Symbol:      "DB",
+					Description: "The native staking token",
+					DenomUnits: []*types.DenomUnit{
+						{"udiamondback", uint32(0), []string{"microdiamondback"}},
+					},
+				},
+			},
+		},
+		{
+			Name:        "success - correct authority",
+			ExpectErr:   false,
+			ExpectedErr: "",
+			req: types.MsgUpdateDenomMetadata{
+				FromAddress: "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn",
+				Title:       "title",
+				Description: "description",
+				Metadata: &types.Metadata{
+					Base:        "diamondback",
+					Name:        "diamondback",
+					Symbol:      "DB",
+					Description: "The native staking token",
+					DenomUnits: []*types.DenomUnit{
+						{"udiamondback", uint32(0), []string{"microdiamondback"}},
+					},
+				},
+			},
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.Name, func(t *testing.T) {
+			_, err := msgServer.UpdateDenomMetadata(ctx, &testCase.req)
+			if testCase.ExpectErr {
+				require.Error(t, err)
+				require.Equal(t, testCase.ExpectedErr, err.Error())
+			} else {
+				require.NoError(t, err)
+				metadata, _ := app.BankKeeper.GetDenomMetaData(ctx, "diamondback")
+				require.Equal(t, testCase.req.Metadata, metadata)
+			}
+		})
+	}
+}

x/bank/keeper/send.go

@@ -200,7 +200,7 @@ func (k BaseSendKeeper) subUnlockedCoins(ctx sdk.Context, addr sdk.AccAddress, a
 
 		_, hasNeg := sdk.Coins{spendable}.SafeSub(coin)
 		if hasNeg {
-			return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, "%s is smaller than %s", spendable, balance)
+			return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, "%s is smaller than %s", spendable, coin)
 		}
 
 		newBalance := balance.Sub(coin)
@@ -425,11 +425,12 @@ func (k BaseSendKeeper) GetAllSendEnabledEntries(ctx sdk.Context) []types.SendEn
 // getSendEnabled returns whether send is enabled and whether that flag was set for a denom.
 //
 // Example usage:
-//    store := ctx.KVStore(k.storeKey)
-//    sendEnabled, found := getSendEnabled(store, "atom")
-//    if !found {
-//        sendEnabled = DefaultSendEnabled
-//    }
+//
+//	store := ctx.KVStore(k.storeKey)
+//	sendEnabled, found := getSendEnabled(store, "atom")
+//	if !found {
+//	    sendEnabled = DefaultSendEnabled
+//	}
 func (k BaseSendKeeper) getSendEnabled(store sdk.KVStore, denom string) (bool, bool) {
 	key := types.CreateSendEnabledKey(denom)
 	if !store.Has(key) {

x/bank/types/msgs.go

@@ -210,11 +210,7 @@ func (msg MsgUpdateDenomMetadata) Type() string {
 
 // ValidateBasic Implements Msg.
 func (msg MsgUpdateDenomMetadata) ValidateBasic() error {
-	err := msg.Metadata.Validate()
-	if err != nil {
-		return err
-	}
-	return nil
+	return msg.Metadata.Validate()
 }
 
 // GetSignBytes Implements Msg.

x/bank/types/msgs_test.go

@@ -283,22 +283,22 @@ func TestUpdateDenomMetadataGetSignBytes(t *testing.T) {
 		Title: "title",
 		Description: "description",
 		Metadata: &Metadata {
-			Name:        "Cosmos Hub Atom",
-			Symbol:      "ATOM",
-			Description: "The native staking token of the Cosmos Hub.",
+			Name:        "diamondback",
+			Symbol:      "DB",
+			Description: "The native staking token",
 			DenomUnits: []*DenomUnit{
-				{"uatom", uint32(0), []string{"microatom"}},
+				{"udiamondback", uint32(0), []string{"microdiamondback"}},
 			},
 		},
 	}
 	res := msg.GetSignBytes()
 
-	expected := `{"type":"cosmos-sdk/MsgUpdateDenomMetadata","value":{"description":"description","metadata":{"denom_units":[{"aliases":["microatom"],"denom":"uatom"}],"description":"The native staking token of the Cosmos Hub.","name":"Cosmos Hub Atom","symbol":"ATOM"},"title":"title"}}`
+	expected := `{"type":"cosmos-sdk/MsgUpdateDenomMetadata","value":{"description":"description","metadata":{"denom_units":[{"aliases":["microdiamondback"],"denom":"udiamondback"}],"description":"The native staking token.","name":"diamondback","symbol":"DB"},"title":"title"}}`
 	require.Equal(t, expected, string(res))
 }
 
 func TestUpdateDenomMetadataGetSigners(t *testing.T) {
-	from := sdk.AccAddress("input111111111111111")
+	from := sdk.AccAddress("cosmos1d9h8qat57ljhcm")
 	title := "Proposal Title"
 	description := "Proposal description"
 	metadata := Metadata{}

x/gov/keeper/keeper_test.go

@@ -41,9 +41,9 @@ func (suite *KeeperTestSuite) SetupTest() {
 	err = app.BankKeeper.SendCoinsFromModuleToModule(ctx, minttypes.ModuleName, types.ModuleName, coins)
 	suite.NoError(err)
 
-	// Create a new denom 'atom' so the proposal to update fee denom can be tested
-	atomCoins := sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(100000)))
-	err = app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, atomCoins)
+	// Create a new denom 'diamondback' so the proposal to update fee denom can be tested
+	diamondbackCoins := sdk.NewCoins(sdk.NewCoin("diamondback", sdk.NewInt(100000)))
+	err = app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, diamondbackCoins)
 	suite.NoError(err)
 
 	queryHelper := baseapp.NewQueryServerTestHelper(ctx, app.InterfaceRegistry())

x/gov/keeper/msg_server_test.go

@@ -29,13 +29,13 @@ func (suite *KeeperTestSuite) TestSubmitProposalReq() {
 		Title:       "title",
 		Description: "description",
 		Metadata: &banktypes.Metadata{
-			Base:        "atom",
-			Name:        "Cosmos Hub Atom",
-			Symbol:      "ATOM",
-			Display:     "atom",
-			Description: "The native staking token of the Cosmos Hub.",
+			Base:        "diamondback",
+			Name:        "diamondback",
+			Symbol:      "DB",
+			Display:     "diamondback",
+			Description: "The native token.",
 			DenomUnits: []*banktypes.DenomUnit{
-				{"atom", uint32(0), []string{"atom"}},
+				{"diamondback", uint32(0), []string{"diamondback"}},
 			},
 		},
 	}
@@ -186,13 +186,13 @@ func (suite *KeeperTestSuite) TestVoteReq() {
 		Title:       "title",
 		Description: "description",
 		Metadata: &banktypes.Metadata{
-			Base:        "atom",
-			Name:        "Cosmos Hub Atom",
-			Symbol:      "ATOM",
-			Display:     "atom",
-			Description: "The native staking token of the Cosmos Hub.",
+			Base:        "diamondback",
+			Name:        "diamondback",
+			Symbol:      "DB",
+			Display:     "diamondback",
+			Description: "Native token",
 			DenomUnits: []*banktypes.DenomUnit{
-				{"atom", uint32(0), []string{"atom"}},
+				{"diamondback", uint32(0), []string{"diamondback"}},
 			},
 		},
 	}
@@ -335,13 +335,13 @@ func (suite *KeeperTestSuite) TestVoteWeightedReq() {
 		Title:       "title",
 		Description: "description",
 		Metadata: &banktypes.Metadata{
-			Base:        "atom",
-			Name:        "Cosmos Hub Atom",
-			Symbol:      "ATOM",
-			Display:     "atom",
-			Description: "The native staking token of the Cosmos Hub.",
+			Base:        "diamondback",
+			Name:        "diamondback",
+			Symbol:      "DB",
+			Display:     "diamondback",
+			Description: "The native token",
 			DenomUnits: []*banktypes.DenomUnit{
-				{"atom", uint32(0), []string{"atom"}},
+				{"diamondback", uint32(0), []string{"diamondback"}},
 			},
 		},
 	}