Modify all message constructors to take in a string for address (#108)

* Modify all message constructors to take in a string for address All NewMsg() functions take in the signer as a string. This prevents bugs from happening at the level of the caller since String relies on external context * add changelog
cosmos · Apr 12, 2021 · d5cc991 · d5cc991
1 parent 58df13c
commit d5cc991
Show file tree

Hide file tree

Showing 20 changed files with 140 additions and 138 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -36,6 +36,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+## API Breaking
+
+* (modules) [\#108](https://github.com/cosmos/ibc-go/pull/108) All message constructors take the signer as a string to prevent upstream bugs. The `String()` function for an SDK Acc Address relies on external context.
+
 ### State Machine Breaking
 
 * (modules/light-clients/07-tendermint) [\#99](https://github.com/cosmos/ibc-go/pull/99) Enforce maximum chain-id length for tendermint client. 

diff --git a/modules/apps/transfer/client/cli/tx.go b/modules/apps/transfer/client/cli/tx.go
@@ -40,7 +40,7 @@ to the counterparty channel. Any timeout set to 0 is disabled.`),
 			if err != nil {
 				return err
 			}
-			sender := clientCtx.GetFromAddress()
+			sender := clientCtx.GetFromAddress().String()
 			srcPort := args[0]
 			srcChannel := args[1]
 			receiver := args[2]

diff --git a/modules/apps/transfer/handler_test.go b/modules/apps/transfer/handler_test.go
@@ -43,7 +43,7 @@ func (suite *TransferTestSuite) TestHandleMsgTransfer() {
 	coinToSendToB := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
 
 	// send from chainA to chainB
-	msg := types.NewMsgTransfer(channelA.PortID, channelA.ID, coinToSendToB, suite.chainA.SenderAccount.GetAddress(), suite.chainB.SenderAccount.GetAddress().String(), timeoutHeight, 0)
+	msg := types.NewMsgTransfer(channelA.PortID, channelA.ID, coinToSendToB, suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), timeoutHeight, 0)
 
 	err := suite.coordinator.SendMsg(suite.chainA, suite.chainB, clientB, msg)
 	suite.Require().NoError(err) // message committed
@@ -67,7 +67,7 @@ func (suite *TransferTestSuite) TestHandleMsgTransfer() {
 	channelOnBForC, channelOnCForB := suite.coordinator.CreateTransferChannels(suite.chainB, suite.chainC, connOnBForC, connOnCForB, channeltypes.UNORDERED)
 
 	// send from chainB to chainC
-	msg = types.NewMsgTransfer(channelOnBForC.PortID, channelOnBForC.ID, coinSentFromAToB, suite.chainB.SenderAccount.GetAddress(), suite.chainC.SenderAccount.GetAddress().String(), timeoutHeight, 0)
+	msg = types.NewMsgTransfer(channelOnBForC.PortID, channelOnBForC.ID, coinSentFromAToB, suite.chainB.SenderAccount.GetAddress().String(), suite.chainC.SenderAccount.GetAddress().String(), timeoutHeight, 0)
 
 	err = suite.coordinator.SendMsg(suite.chainB, suite.chainC, clientOnCForB, msg)
 	suite.Require().NoError(err) // message committed
@@ -91,7 +91,7 @@ func (suite *TransferTestSuite) TestHandleMsgTransfer() {
 	suite.Require().Zero(balance.Amount.Int64())
 
 	// send from chainC back to chainB
-	msg = types.NewMsgTransfer(channelOnCForB.PortID, channelOnCForB.ID, coinSentFromBToC, suite.chainC.SenderAccount.GetAddress(), suite.chainB.SenderAccount.GetAddress().String(), timeoutHeight, 0)
+	msg = types.NewMsgTransfer(channelOnCForB.PortID, channelOnCForB.ID, coinSentFromBToC, suite.chainC.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), timeoutHeight, 0)
 
 	err = suite.coordinator.SendMsg(suite.chainC, suite.chainB, clientOnBForC, msg)
 	suite.Require().NoError(err) // message committed

diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go
@@ -103,7 +103,7 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
 			if !tc.sendFromSource {
 				// send coin from chainB to chainA
 				coinFromBToA := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
-				transferMsg := types.NewMsgTransfer(channelB.PortID, channelB.ID, coinFromBToA, suite.chainB.SenderAccount.GetAddress(), suite.chainA.SenderAccount.GetAddress().String(), clienttypes.NewHeight(0, 110), 0)
+				transferMsg := types.NewMsgTransfer(channelB.PortID, channelB.ID, coinFromBToA, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String(), clienttypes.NewHeight(0, 110), 0)
 				err = suite.coordinator.SendMsg(suite.chainB, suite.chainA, channelA.ClientID, transferMsg)
 				suite.Require().NoError(err) // message committed
 
@@ -115,7 +115,7 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
 				packetKey := host.PacketCommitmentKey(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
 				proof, proofHeight := suite.chainB.QueryProof(packetKey)
 
-				recvMsg := channeltypes.NewMsgRecvPacket(packet, proof, proofHeight, suite.chainA.SenderAccount.GetAddress())
+				recvMsg := channeltypes.NewMsgRecvPacket(packet, proof, proofHeight, suite.chainA.SenderAccount.GetAddress().String())
 				err = suite.coordinator.SendMsg(suite.chainA, suite.chainB, channelB.ClientID, recvMsg)
 				suite.Require().NoError(err) // message committed
 			}
@@ -190,7 +190,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 			if tc.recvIsSource {
 				// send coin from chainB to chainA, receive them, acknowledge them, and send back to chainB
 				coinFromBToA := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
-				transferMsg := types.NewMsgTransfer(channelB.PortID, channelB.ID, coinFromBToA, suite.chainB.SenderAccount.GetAddress(), suite.chainA.SenderAccount.GetAddress().String(), clienttypes.NewHeight(0, 110), 0)
+				transferMsg := types.NewMsgTransfer(channelB.PortID, channelB.ID, coinFromBToA, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String(), clienttypes.NewHeight(0, 110), 0)
 				err := suite.coordinator.SendMsg(suite.chainB, suite.chainA, channelA.ClientID, transferMsg)
 				suite.Require().NoError(err) // message committed
 
@@ -210,7 +210,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 			}
 
 			// send coin from chainA to chainB
-			transferMsg := types.NewMsgTransfer(channelA.PortID, channelA.ID, sdk.NewCoin(trace.IBCDenom(), amount), suite.chainA.SenderAccount.GetAddress(), receiver, clienttypes.NewHeight(0, 110), 0)
+			transferMsg := types.NewMsgTransfer(channelA.PortID, channelA.ID, sdk.NewCoin(trace.IBCDenom(), amount), suite.chainA.SenderAccount.GetAddress().String(), receiver, clienttypes.NewHeight(0, 110), 0)
 			err := suite.coordinator.SendMsg(suite.chainA, suite.chainB, channelB.ClientID, transferMsg)
 			suite.Require().NoError(err) // message committed
 

diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go
@@ -18,14 +18,14 @@ const (
 //nolint:interfacer
 func NewMsgTransfer(
 	sourcePort, sourceChannel string,
-	token sdk.Coin, sender sdk.AccAddress, receiver string,
+	token sdk.Coin, sender, receiver string,
 	timeoutHeight clienttypes.Height, timeoutTimestamp uint64,
 ) *MsgTransfer {
 	return &MsgTransfer{
 		SourcePort:       sourcePort,
 		SourceChannel:    sourceChannel,
 		Token:            token,
-		Sender:           sender.String(),
+		Sender:           sender,
 		Receiver:         receiver,
 		TimeoutHeight:    timeoutHeight,
 		TimeoutTimestamp: timeoutTimestamp,
@@ -77,9 +77,9 @@ func (msg MsgTransfer) GetSignBytes() []byte {
 
 // GetSigners implements sdk.Msg
 func (msg MsgTransfer) GetSigners() []sdk.AccAddress {
-	valAddr, err := sdk.AccAddressFromBech32(msg.Sender)
+	signer, err := sdk.AccAddressFromBech32(msg.Sender)
 	if err != nil {
 		panic(err)
 	}
-	return []sdk.AccAddress{valAddr}
+	return []sdk.AccAddress{signer}
 }
diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go
@@ -25,9 +25,9 @@ const (
 )
 
 var (
-	addr1     = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
+	addr1     = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String()
 	addr2     = sdk.AccAddress("testaddr2").String()
-	emptyAddr sdk.AccAddress
+	emptyAddr string
 
 	coin             = sdk.NewCoin("atom", sdk.NewInt(100))
 	ibcCoin          = sdk.NewCoin("ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2", sdk.NewInt(100))
@@ -96,8 +96,10 @@ func TestMsgTransferValidation(t *testing.T) {
 
 // TestMsgTransferGetSigners tests GetSigners for MsgTransfer
 func TestMsgTransferGetSigners(t *testing.T) {
-	msg := NewMsgTransfer(validPort, validChannel, coin, addr1, addr2, timeoutHeight, 0)
+	addr := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
+
+	msg := NewMsgTransfer(validPort, validChannel, coin, addr.String(), addr2, timeoutHeight, 0)
 	res := msg.GetSigners()
 
-	require.Equal(t, []sdk.AccAddress{addr1}, res)
+	require.Equal(t, []sdk.AccAddress{addr}, res)
 }
diff --git a/modules/apps/transfer/types/packet_test.go b/modules/apps/transfer/types/packet_test.go
@@ -18,11 +18,11 @@ func TestFungibleTokenPacketDataValidateBasic(t *testing.T) {
 		packetData FungibleTokenPacketData
 		expPass    bool
 	}{
-		{"valid packet", NewFungibleTokenPacketData(denom, amount, addr1.String(), addr2), true},
-		{"invalid denom", NewFungibleTokenPacketData("", amount, addr1.String(), addr2), false},
-		{"invalid amount", NewFungibleTokenPacketData(denom, 0, addr1.String(), addr2), false},
-		{"missing sender address", NewFungibleTokenPacketData(denom, amount, emptyAddr.String(), addr2), false},
-		{"missing recipient address", NewFungibleTokenPacketData(denom, amount, addr1.String(), emptyAddr.String()), false},
+		{"valid packet", NewFungibleTokenPacketData(denom, amount, addr1, addr2), true},
+		{"invalid denom", NewFungibleTokenPacketData("", amount, addr1, addr2), false},
+		{"invalid amount", NewFungibleTokenPacketData(denom, 0, addr1, addr2), false},
+		{"missing sender address", NewFungibleTokenPacketData(denom, amount, emptyAddr, addr2), false},
+		{"missing recipient address", NewFungibleTokenPacketData(denom, amount, addr1, emptyAddr), false},
 	}
 
 	for i, tc := range testCases {

diff --git a/modules/core/02-client/client/cli/tx.go b/modules/core/02-client/client/cli/tx.go
@@ -71,7 +71,7 @@ func NewCreateClientCmd() *cobra.Command {
 				}
 			}
 
-			msg, err := types.NewMsgCreateClient(clientState, consensusState, clientCtx.GetFromAddress())
+			msg, err := types.NewMsgCreateClient(clientState, consensusState, clientCtx.GetFromAddress().String())
 			if err != nil {
 				return err
 			}
@@ -124,7 +124,7 @@ func NewUpdateClientCmd() *cobra.Command {
 				}
 			}
 
-			msg, err := types.NewMsgUpdateClient(clientID, header, clientCtx.GetFromAddress())
+			msg, err := types.NewMsgUpdateClient(clientID, header, clientCtx.GetFromAddress().String())
 			if err != nil {
 				return err
 			}
@@ -172,7 +172,7 @@ func NewSubmitMisbehaviourCmd() *cobra.Command {
 				}
 			}
 
-			msg, err := types.NewMsgSubmitMisbehaviour(misbehaviour.GetClientID(), misbehaviour, clientCtx.GetFromAddress())
+			msg, err := types.NewMsgSubmitMisbehaviour(misbehaviour.GetClientID(), misbehaviour, clientCtx.GetFromAddress().String())
 			if err != nil {
 				return err
 			}
@@ -242,7 +242,7 @@ func NewUpgradeClientCmd() *cobra.Command {
 			proofUpgradeClient := []byte(args[3])
 			proofUpgradeConsensus := []byte(args[4])
 
-			msg, err := types.NewMsgUpgradeClient(clientID, clientState, consensusState, proofUpgradeClient, proofUpgradeConsensus, clientCtx.GetFromAddress())
+			msg, err := types.NewMsgUpgradeClient(clientID, clientState, consensusState, proofUpgradeClient, proofUpgradeConsensus, clientCtx.GetFromAddress().String())
 			if err != nil {
 				return err
 			}

diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
@@ -7,6 +7,7 @@ import (
 
 	tmtypes "github.com/tendermint/tendermint/types"
 
+	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 	"github.com/cosmos/ibc-go/modules/core/02-client/types"
 	clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
 	commitmenttypes "github.com/cosmos/ibc-go/modules/core/23-commitment/types"
@@ -15,7 +16,6 @@ import (
 	localhosttypes "github.com/cosmos/ibc-go/modules/light-clients/09-localhost/types"
 	ibctesting "github.com/cosmos/ibc-go/testing"
 	ibctestingmock "github.com/cosmos/ibc-go/testing/mock"
-	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 )
 
 func (suite *KeeperTestSuite) TestCreateClient() {
@@ -598,7 +598,7 @@ func (suite *KeeperTestSuite) TestUpdateClientEventEmission() {
 
 	msg, err := clienttypes.NewMsgUpdateClient(
 		clientID, header,
-		suite.chainA.SenderAccount.GetAddress(),
+		suite.chainA.SenderAccount.GetAddress().String(),
 	)
 
 	result, err := suite.chainA.SendMsgs(msg)

diff --git a/modules/core/02-client/types/msgs.go b/modules/core/02-client/types/msgs.go
@@ -31,7 +31,7 @@ var (
 // NewMsgCreateClient creates a new MsgCreateClient instance
 //nolint:interfacer
 func NewMsgCreateClient(
-	clientState exported.ClientState, consensusState exported.ConsensusState, signer sdk.AccAddress,
+	clientState exported.ClientState, consensusState exported.ConsensusState, signer string,
 ) (*MsgCreateClient, error) {
 
 	anyClientState, err := PackClientState(clientState)
@@ -47,7 +47,7 @@ func NewMsgCreateClient(
 	return &MsgCreateClient{
 		ClientState:    anyClientState,
 		ConsensusState: anyConsensusState,
-		Signer:         signer.String(),
+		Signer:         signer,
 	}, nil
 }
 
@@ -119,7 +119,7 @@ func (msg MsgCreateClient) UnpackInterfaces(unpacker codectypes.AnyUnpacker) err
 
 // NewMsgUpdateClient creates a new MsgUpdateClient instance
 //nolint:interfacer
-func NewMsgUpdateClient(id string, header exported.Header, signer sdk.AccAddress) (*MsgUpdateClient, error) {
+func NewMsgUpdateClient(id string, header exported.Header, signer string) (*MsgUpdateClient, error) {
 	anyHeader, err := PackHeader(header)
 	if err != nil {
 		return nil, err
@@ -128,7 +128,7 @@ func NewMsgUpdateClient(id string, header exported.Header, signer sdk.AccAddress
 	return &MsgUpdateClient{
 		ClientId: id,
 		Header:   anyHeader,
-		Signer:   signer.String(),
+		Signer:   signer,
 	}, nil
 }
 
@@ -185,7 +185,7 @@ func (msg MsgUpdateClient) UnpackInterfaces(unpacker codectypes.AnyUnpacker) err
 // NewMsgUpgradeClient creates a new MsgUpgradeClient instance
 // nolint: interfacer
 func NewMsgUpgradeClient(clientID string, clientState exported.ClientState, consState exported.ConsensusState,
-	proofUpgradeClient, proofUpgradeConsState []byte, signer sdk.AccAddress) (*MsgUpgradeClient, error) {
+	proofUpgradeClient, proofUpgradeConsState []byte, signer string) (*MsgUpgradeClient, error) {
 	anyClient, err := PackClientState(clientState)
 	if err != nil {
 		return nil, err
@@ -201,7 +201,7 @@ func NewMsgUpgradeClient(clientID string, clientState exported.ClientState, cons
 		ConsensusState:             anyConsState,
 		ProofUpgradeClient:         proofUpgradeClient,
 		ProofUpgradeConsensusState: proofUpgradeConsState,
-		Signer:                     signer.String(),
+		Signer:                     signer,
 	}, nil
 }
 
@@ -276,7 +276,7 @@ func (msg MsgUpgradeClient) UnpackInterfaces(unpacker codectypes.AnyUnpacker) er
 
 // NewMsgSubmitMisbehaviour creates a new MsgSubmitMisbehaviour instance.
 //nolint:interfacer
-func NewMsgSubmitMisbehaviour(clientID string, misbehaviour exported.Misbehaviour, signer sdk.AccAddress) (*MsgSubmitMisbehaviour, error) {
+func NewMsgSubmitMisbehaviour(clientID string, misbehaviour exported.Misbehaviour, signer string) (*MsgSubmitMisbehaviour, error) {
 	anyMisbehaviour, err := PackMisbehaviour(misbehaviour)
 	if err != nil {
 		return nil, err
@@ -285,7 +285,7 @@ func NewMsgSubmitMisbehaviour(clientID string, misbehaviour exported.Misbehaviou
 	return &MsgSubmitMisbehaviour{
 		ClientId:     clientID,
 		Misbehaviour: anyMisbehaviour,
-		Signer:       signer.String(),
+		Signer:       signer,
 	}, nil
 }