Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(bank): remove .String() calls #18175

Merged
merged 13 commits into from
Dec 11, 2023
Merged

refactor(bank): remove .String() calls #18175

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8c020d9
remove bech32 decoding from bank module
tac0turtle Oct 19, 2023
cfe76bd
more rremovals
tac0turtle Oct 19, 2023
967f3c8
Merge branch 'main' into marko/bank_bech322
tac0turtle Oct 26, 2023
391ce06
Merge branch 'main' into marko/bank_bech322
tac0turtle Nov 14, 2023
33ae04e
Merge branch 'main' into marko/bank_bech322
tac0turtle Nov 27, 2023
6d2955f
fix some tests
tac0turtle Nov 27, 2023
83e0806
fix lint
tac0turtle Nov 27, 2023
e4d2424
fix some tests
tac0turtle Nov 28, 2023
d6e6641
clean up code
tac0turtle Nov 29, 2023
b1e5d10
return error
tac0turtle Nov 29, 2023
e6afb16
Merge branch 'main' into marko/bank_bech322
tac0turtle Dec 4, 2023
4cf6a13
minor fixes
tac0turtle Dec 11, 2023
2eba77f
fix msg send sim
facundomedica Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions tests/e2e/auth/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"

"cosmossdk.io/core/address"
"cosmossdk.io/depinject"
"cosmossdk.io/math"
authcli "cosmossdk.io/x/auth/client/cli"
Expand All @@ -22,6 +23,7 @@ import (

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
addresscodec "github.com/cosmos/cosmos-sdk/codec/address"
"github.com/cosmos/cosmos-sdk/crypto/hd"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
kmultisig "github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
Expand All @@ -40,6 +42,7 @@ type E2ETestSuite struct {
suite.Suite

cfg network.Config
ac address.Codec
network network.NetworkI
}

Expand Down Expand Up @@ -75,6 +78,8 @@ func (s *E2ETestSuite) SetupSuite() {
_, err = kb.SaveMultisig("multi", multi)
s.Require().NoError(err)
s.Require().NoError(s.network.WaitForNextBlock())

s.ac = addresscodec.NewBech32Codec("cosmos")
}

func (s *E2ETestSuite) TearDownSuite() {
Expand Down Expand Up @@ -1302,11 +1307,7 @@ func TestGetBroadcastCommandWithoutOfflineFlag(t *testing.T) {
// Create new file with tx
builder := txCfg.NewTxBuilder()
builder.SetGasLimit(200000)
from, err := sdk.AccAddressFromBech32("cosmos1cxlt8kznps92fwu3j6npahx4mjfutydyene2qw")
require.NoError(t, err)
to, err := sdk.AccAddressFromBech32("cosmos1cxlt8kznps92fwu3j6npahx4mjfutydyene2qw")
require.NoError(t, err)
err = builder.SetMsgs(banktypes.NewMsgSend(from, to, sdk.Coins{sdk.NewInt64Coin("stake", 10000)}))
err = builder.SetMsgs(banktypes.NewMsgSend("cosmos1cxlt8kznps92fwu3j6npahx4mjfutydyene2qw", "cosmos1cxlt8kznps92fwu3j6npahx4mjfutydyene2qw", sdk.Coins{sdk.NewInt64Coin("stake", 10000)}))
require.NoError(t, err)
txContents, err := txCfg.TxJSONEncoder()(builder.GetTx())
require.NoError(t, err)
Expand All @@ -1330,7 +1331,7 @@ func (s *E2ETestSuite) TestTxWithoutPublicKey() {

// Create a txBuilder with an unsigned tx.
txBuilder := txCfg.NewTxBuilder()
msg := banktypes.NewMsgSend(val1.GetAddress(), val1.GetAddress(), sdk.NewCoins(
msg := banktypes.NewMsgSend(val1.GetAddress().String(), val1.GetAddress().String(), sdk.NewCoins(
sdk.NewCoin(s.cfg.BondDenom, math.NewInt(10)),
))
err := txBuilder.SetMsgs(msg)
Expand Down Expand Up @@ -1397,9 +1398,15 @@ func (s *E2ETestSuite) TestSignWithMultiSignersAminoJSON() {
// because DIRECT doesn't support multi signers via the CLI.
// Since we use amino, we don't need to pre-populate signer_infos.
txBuilder := val0.GetClientCtx().TxConfig.NewTxBuilder()
err := txBuilder.SetMsgs(
banktypes.NewMsgSend(val0.GetAddress(), addr1, sdk.NewCoins(val0Coin)),
banktypes.NewMsgSend(val1.GetAddress(), addr1, sdk.NewCoins(val1Coin)),
val0Str, err := s.ac.BytesToString(val0.GetAddress())
s.Require().NoError(err)
val1Str, err := s.ac.BytesToString(val1.GetAddress())
s.Require().NoError(err)
addrStr, err := s.ac.BytesToString(addr1)
s.Require().NoError(err)
err = txBuilder.SetMsgs(
banktypes.NewMsgSend(val0Str, addrStr, sdk.NewCoins(val0Coin)),
banktypes.NewMsgSend(val1Str, addrStr, sdk.NewCoins(val1Coin)),
)
require.NoError(err)
txBuilder.SetFeeAmount(sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, math.NewInt(10))))
Expand Down
15 changes: 11 additions & 4 deletions tests/e2e/bank/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ import (
"github.com/cosmos/gogoproto/proto"
"github.com/stretchr/testify/suite"

"cosmossdk.io/core/address"
"cosmossdk.io/math"
"cosmossdk.io/x/bank/client/cli"
"cosmossdk.io/x/bank/types"

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
addresscodec "github.com/cosmos/cosmos-sdk/codec/address"
"github.com/cosmos/cosmos-sdk/testutil"
clitestutil "github.com/cosmos/cosmos-sdk/testutil/cli"
"github.com/cosmos/cosmos-sdk/testutil/network"
Expand All @@ -23,6 +25,7 @@ type E2ETestSuite struct {
suite.Suite

cfg network.Config
ac address.Codec
network network.NetworkI
}

Expand Down Expand Up @@ -85,6 +88,7 @@ func (s *E2ETestSuite) SetupSuite() {
s.network, err = network.New(s.T(), s.T().TempDir(), s.cfg)
s.Require().NoError(err)
s.Require().NoError(s.network.WaitForNextBlock())
s.ac = addresscodec.NewBech32Codec("cosmos")
}

func (s *E2ETestSuite) TearDownSuite() {
Expand All @@ -101,10 +105,13 @@ func (s *E2ETestSuite) TestNewSendTxCmdGenOnly() {
sdk.NewCoin(fmt.Sprintf("%stoken", val.GetMoniker()), math.NewInt(10)),
sdk.NewCoin(s.cfg.BondDenom, math.NewInt(10)),
)

fromStr, err := s.ac.BytesToString(from)
s.Require().NoError(err)
toStr, err := s.ac.BytesToString(to)
s.Require().NoError(err)
msgSend := &types.MsgSend{
FromAddress: from.String(),
ToAddress: to.String(),
FromAddress: fromStr,
ToAddress: toStr,
Amount: amount,
}

Expand All @@ -120,7 +127,7 @@ func (s *E2ETestSuite) TestNewSendTxCmdGenOnly() {

tx, err := s.cfg.TxConfig.TxJSONDecoder()(bz.Bytes())
s.Require().NoError(err)
s.Require().Equal([]sdk.Msg{types.NewMsgSend(from, to, amount)}, tx.GetMsgs())
s.Require().Equal([]sdk.Msg{types.NewMsgSend(fromStr, toStr, amount)}, tx.GetMsgs())
}

func (s *E2ETestSuite) TestNewSendTxCmdDryRun() {
Expand Down
27 changes: 17 additions & 10 deletions tests/integration/auth/client/cli/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,11 +840,8 @@ func (s *CLITestSuite) TestGetBroadcastCommandWithoutOfflineFlag() {
// Create new file with tx
builder := txCfg.NewTxBuilder()
builder.SetGasLimit(200000)
from, err := s.ac.StringToBytes("cosmos1cxlt8kznps92fwu3j6npahx4mjfutydyene2qw")
s.Require().NoError(err)
to, err := s.ac.StringToBytes("cosmos1cxlt8kznps92fwu3j6npahx4mjfutydyene2qw")
s.Require().NoError(err)
err = builder.SetMsgs(banktypes.NewMsgSend(from, to, sdk.Coins{sdk.NewInt64Coin("stake", 10000)}))

err := builder.SetMsgs(banktypes.NewMsgSend("cosmos1cxlt8kznps92fwu3j6npahx4mjfutydyene2qw", "cosmos1cxlt8kznps92fwu3j6npahx4mjfutydyene2qw", sdk.Coins{sdk.NewInt64Coin("stake", 10000)}))
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
s.Require().NoError(err)
txContents, err := txCfg.TxJSONEncoder()(builder.GetTx())
s.Require().NoError(err)
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -864,12 +861,15 @@ func (s *CLITestSuite) TestGetBroadcastCommandWithoutOfflineFlag() {
func (s *CLITestSuite) TestTxWithoutPublicKey() {
txCfg := s.clientCtx.TxConfig

valStr, err := s.ac.BytesToString(s.val)
s.Require().NoError(err)

// Create a txBuilder with an unsigned tx.
txBuilder := txCfg.NewTxBuilder()
msg := banktypes.NewMsgSend(s.val, s.val, sdk.NewCoins(
msg := banktypes.NewMsgSend(valStr, valStr, sdk.NewCoins(
sdk.NewCoin("Stake", math.NewInt(10)),
))
err := txBuilder.SetMsgs(msg)
err = txBuilder.SetMsgs(msg)
s.Require().NoError(err)
txBuilder.SetFeeAmount(sdk.NewCoins(sdk.NewCoin("Stake", math.NewInt(150))))
txBuilder.SetGasLimit(testdata.NewTestGasLimit())
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -917,14 +917,21 @@ func (s *CLITestSuite) TestSignWithMultiSignersAminoJSON() {
val1Coin := sdk.NewCoin("test2token", math.NewInt(10))
_, _, addr1 := testdata.KeyTestPubAddr()

valStr, err := s.ac.BytesToString(val0)
s.Require().NoError(err)
val1Str, err := s.ac.BytesToString(val1)
s.Require().NoError(err)

addrStr, err := s.ac.BytesToString(addr1)
s.Require().NoError(err)
// Creating a tx with 2 msgs from 2 signers: val0 and val1.
// The validators need to sign with SIGN_MODE_LEGACY_AMINO_JSON,
// because DIRECT doesn't support multi signers via the CLI.
// Since we use amino, we don't need to pre-populate signer_infos.
txBuilder := s.clientCtx.TxConfig.NewTxBuilder()
err := txBuilder.SetMsgs(
banktypes.NewMsgSend(val0, addr1, sdk.NewCoins(val0Coin)),
banktypes.NewMsgSend(val1, addr1, sdk.NewCoins(val1Coin)),
err = txBuilder.SetMsgs(
banktypes.NewMsgSend(valStr, addrStr, sdk.NewCoins(val0Coin)),
banktypes.NewMsgSend(val1Str, addrStr, sdk.NewCoins(val1Coin)),
)
s.Require().NoError(err)
txBuilder.SetFeeAmount(sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(10))))
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
8 changes: 6 additions & 2 deletions tests/integration/bank/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ var (
coins = sdk.Coins{sdk.NewInt64Coin("foocoin", 10)}
halfCoins = sdk.Coins{sdk.NewInt64Coin("foocoin", 5)}

sendMsg1 = types.NewMsgSend(addr1, addr2, coins)
sendMsg1 = types.NewMsgSend(addr1.String(), addr2.String(), coins)

multiSendMsg1 = &types.MsgMultiSend{
Inputs: []types.Input{types.NewInput(addr1, coins)},
Expand Down Expand Up @@ -167,7 +167,11 @@ func TestSendNotEnoughBalance(t *testing.T) {
origAccNum := res1.GetAccountNumber()
origSeq := res1.GetSequence()

sendMsg := types.NewMsgSend(addr1, addr2, sdk.Coins{sdk.NewInt64Coin("foocoin", 100)})
addr1Str, err := s.AccountKeeper.AddressCodec().BytesToString(addr1)
require.NoError(t, err)
addr2Str, err := s.AccountKeeper.AddressCodec().BytesToString(addr2)
require.NoError(t, err)
sendMsg := types.NewMsgSend(addr1Str, addr2Str, sdk.Coins{sdk.NewInt64Coin("foocoin", 100)})
header := header.Info{Height: baseApp.LastBlockHeight() + 1}
txConfig := moduletestutil.MakeTestTxConfig()
_, _, err = simtestutil.SignCheckDeliver(t, txConfig, baseApp, header, []sdk.Msg{sendMsg}, "", []uint64{origAccNum}, []uint64{origSeq}, false, false, priv1)
Expand Down
2 changes: 1 addition & 1 deletion tests/starship/tests/transfer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func (s *TestSuite) TestChainTokenTransfer() {
s.Require().NoError(err)

// build tx into the txBuilder
msg := banktypes.NewMsgSend(addr1, addr2, sdk.NewCoins(sdk.NewCoin(denom, math.NewInt(1230000))))
msg := banktypes.NewMsgSend(addr1.String(), addr2.String(), sdk.NewCoins(sdk.NewCoin(denom, math.NewInt(1230000))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR objective mentions the removal of .String() method calls on addresses, but the hunk still uses addr1.String() and addr2.String(). Please verify if this aligns with the intended refactoring and if an address.Codec should be used instead.

s.Require().NoError(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a redundant error check after the msg construction. The err variable is not reassigned after line 47, so this check is unnecessary.

- s.Require().NoError(err)
Committable suggestion

❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
s.Require().NoError(err)

err = txBuilder.SetMsgs(msg)
s.Require().NoError(err)
Expand Down
11 changes: 10 additions & 1 deletion x/authz/simulation/operations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,16 @@ func SimulateMsgExec(
return simtypes.NoOpMsg(authz.ModuleName, TypeMsgExec, err.Error()), nil, nil
}

msg := []sdk.Msg{banktype.NewMsgSend(granterAddr, granteeAddr, coins)}
graStr, err := ak.AddressCodec().BytesToString(granteeAddr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
graStr, err := ak.AddressCodec().BytesToString(granteeAddr)
graStr, err := ak.AddressCodec().BytesToString(granterAddr)

if err != nil {
return simtypes.NoOpMsg(authz.ModuleName, TypeMsgExec, err.Error()), nil, err
}
greStr, err := ak.AddressCodec().BytesToString(granteeAddr)
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return simtypes.NoOpMsg(authz.ModuleName, TypeMsgExec, err.Error()), nil, err
}

msg := []sdk.Msg{banktype.NewMsgSend(graStr, greStr, coins)}

_, err = sendAuth.Accept(ctx, msg[0])
if err != nil {
Expand Down
26 changes: 16 additions & 10 deletions x/bank/client/cli/tx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ func (s *CLITestSuite) SetupSuite() {

func (s *CLITestSuite) TestMultiSendTxCmd() {
accounts := testutil.CreateKeyringAccounts(s.T(), s.kr, 3)
accountStr := make([]string, len(accounts))
for i, acc := range accounts {
addrStr, err := s.baseCtx.AddressCodec.BytesToString(acc.Address)
s.Require().NoError(err)
accountStr[i] = addrStr
}

cmd := cli.NewMultiSendTxCmd()
cmd.SetOutput(io.Discard)
Expand All @@ -79,10 +85,10 @@ func (s *CLITestSuite) TestMultiSendTxCmd() {
func() client.Context {
return s.baseCtx
},
accounts[0].Address.String(),
accountStr[0],
[]string{
accounts[1].Address.String(),
accounts[2].Address.String(),
accountStr[1],
accountStr[2],
},
sdk.NewCoins(
sdk.NewCoin("stake", sdkmath.NewInt(10)),
Expand All @@ -98,8 +104,8 @@ func (s *CLITestSuite) TestMultiSendTxCmd() {
},
"foo",
[]string{
accounts[1].Address.String(),
accounts[2].Address.String(),
accountStr[1],
accountStr[2],
},
sdk.NewCoins(
sdk.NewCoin("stake", sdkmath.NewInt(10)),
Expand All @@ -113,9 +119,9 @@ func (s *CLITestSuite) TestMultiSendTxCmd() {
func() client.Context {
return s.baseCtx
},
accounts[0].Address.String(),
accountStr[0],
[]string{
accounts[1].Address.String(),
accountStr[1],
"bar",
},
sdk.NewCoins(
Expand All @@ -130,10 +136,10 @@ func (s *CLITestSuite) TestMultiSendTxCmd() {
func() client.Context {
return s.baseCtx
},
accounts[0].Address.String(),
accountStr[0],
[]string{
accounts[1].Address.String(),
accounts[2].Address.String(),
accountStr[1],
accountStr[2],
},
nil,
extraArgs,
Expand Down
5 changes: 4 additions & 1 deletion x/bank/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,10 @@ func (suite *KeeperTestSuite) TestSpendableBalanceByDenom() {
_, err := queryClient.SpendableBalanceByDenom(ctx, &types.QuerySpendableBalanceByDenomRequest{})
suite.Require().Error(err)

req := types.NewQuerySpendableBalanceByDenomRequest(addr, fooDenom)
addrStr, err := suite.authKeeper.AddressCodec().BytesToString(addr)
suite.Require().NoError(err)

req := types.NewQuerySpendableBalanceByDenomRequest(addrStr, fooDenom)
acc := authtypes.NewBaseAccountWithAddress(addr)

suite.mockSpendableCoins(ctx, acc)
Expand Down
20 changes: 16 additions & 4 deletions x/bank/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,12 +152,16 @@ func (k BaseKeeper) DelegateCoins(ctx context.Context, delegatorAddr, moduleAccA
return errorsmod.Wrap(err, "failed to track delegation")
}
// emit coin spent event
delAddrStr, err := k.ak.AddressCodec().BytesToString(delegatorAddr)
if err != nil {
return err
}
sdkCtx := sdk.UnwrapSDKContext(ctx)
sdkCtx.EventManager().EmitEvent(
types.NewCoinSpentEvent(delegatorAddr, amt),
types.NewCoinSpentEvent(delAddrStr, amt),
)

err := k.addCoins(ctx, moduleAccAddr, amt)
err = k.addCoins(ctx, moduleAccAddr, amt)
if err != nil {
return err
}
Expand Down Expand Up @@ -370,9 +374,13 @@ func (k BaseKeeper) MintCoins(ctx context.Context, moduleName string, amounts sd

k.logger.Debug("minted coins from module account", "amount", amounts.String(), "from", moduleName)

addrStr, err := k.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
return err
}
// emit mint event
sdkCtx.EventManager().EmitEvent(
types.NewCoinMintEvent(acc.GetAddress(), amounts),
types.NewCoinMintEvent(addrStr, amounts),
)

return nil
Expand Down Expand Up @@ -405,10 +413,14 @@ func (k BaseKeeper) BurnCoins(ctx context.Context, address []byte, amounts sdk.C

k.logger.Debug("burned tokens from account", "amount", amounts.String(), "from", address)

addrStr, err := k.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
return err
}
// emit burn event
sdkCtx := sdk.UnwrapSDKContext(ctx)
sdkCtx.EventManager().EmitEvent(
types.NewCoinBurnEvent(acc.GetAddress(), amounts),
types.NewCoinBurnEvent(addrStr, amounts),
)

return nil
Expand Down
Loading
Loading