Refactor to use a single transaction builder (#175)

* add gas limit before signing commitments * initial integration test * debug, fix flags, and cleanup * change func name from DecodeChildTx -> UnwrapChildTx * update go mod to use event tracking and cleanup test * update to the latest commit from celestia-core * get rid of commented out code Co-authored-by: John Adler <[email protected]> * use a singular transaction builder and add options * fix bug and cleanup * Bump goreleaser/goreleaser-action from 2.7.0 to 2.8.0 (#137) Bumps [goreleaser/goreleaser-action](https://github.com/goreleaser/goreleaser-action) from 2.7.0 to 2.8.0. - [Release notes](https://github.com/goreleaser/goreleaser-action/releases) - [Commits](goreleaser/goreleaser-action@v2.7.0...v2.8.0) --- updated-dependencies: - dependency-name: goreleaser/goreleaser-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump actions/cache from 2.1.6 to 2.1.7 (#151) Bumps [actions/cache](https://github.com/actions/cache) from 2.1.6 to 2.1.7. - [Release notes](https://github.com/actions/cache/releases) - [Commits](actions/cache@v2.1.6...v2.1.7) --- updated-dependencies: - dependency-name: actions/cache dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump github.com/celestiaorg/nmt from 0.7.0 to 0.8.0 (#146) Bumps [github.com/celestiaorg/nmt](https://github.com/celestiaorg/nmt) from 0.7.0 to 0.8.0. - [Release notes](https://github.com/celestiaorg/nmt/releases) - [Commits](celestiaorg/nmt@v0.7.0...v0.8.0) --- updated-dependencies: - dependency-name: github.com/celestiaorg/nmt dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * use a singular transaction builder and add options * fix bug and cleanup Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
celestiaorg · Jan 14, 2022 · b4c8ebd · b4c8ebd
1 parent 3ce0f1e
commit b4c8ebd
Show file tree

Hide file tree

Showing 6 changed files with 105 additions and 48 deletions.
diff --git a/app/abci_test.go b/app/abci_test.go
@@ -241,7 +241,7 @@ func generateSignedWirePayForMessage(t *testing.T, k uint64, ns, message []byte,
 		t.Error(err)
 	}
 
-	err = msg.SignShareCommitments(signer, signer.NewTxBuilder())
+	err = msg.SignShareCommitments(signer)
 	if err != nil {
 		t.Error(err)
 	}

diff --git a/x/payment/client/cli/wirepayformessage.go b/x/payment/client/cli/wirepayformessage.go
@@ -57,6 +57,7 @@ func CmdWirePayForMessage() *cobra.Command {
 				return err
 			}
 
+			// use the keyring to programmatically sign multiple PayForMessage txs
 			signer := types.NewKeyringSigner(clientCtx.Keyring, accName, clientCtx.ChainID)
 
 			signer.SetAccountNumber(account.GetAccountNumber())
@@ -82,13 +83,12 @@ func CmdWirePayForMessage() *cobra.Command {
 				return err
 			}
 
-			// get the gas price and such and add it to the tx builder that is used to create the signed share commitments
-			builder := signer.NewTxBuilder()
-			builder.SetGasLimit(gasSetting.Gas)
-			builder.SetFeeAmount(parsedFees)
-
 			// sign the  MsgPayForMessage's ShareCommitments
-			err = pfmMsg.SignShareCommitments(signer, builder)
+			err = pfmMsg.SignShareCommitments(
+				signer,
+				types.SetGasLimit(gasSetting.Gas),
+				types.SetFeeAmount(parsedFees),
+			)
 			if err != nil {
 				return err
 			}

diff --git a/x/payment/types/builder_options.go b/x/payment/types/builder_options.go
@@ -0,0 +1,22 @@
+package types
+
+import (
+	sdkclient "github.com/cosmos/cosmos-sdk/client"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+type TxBuilderOption func(builder sdkclient.TxBuilder) sdkclient.TxBuilder
+
+func SetGasLimit(limit uint64) TxBuilderOption {
+	return func(builder sdkclient.TxBuilder) sdkclient.TxBuilder {
+		builder.SetGasLimit(limit)
+		return builder
+	}
+}
+
+func SetFeeAmount(fees sdk.Coins) TxBuilderOption {
+	return func(builder sdkclient.TxBuilder) sdkclient.TxBuilder {
+		builder.SetFeeAmount(fees)
+		return builder
+	}
+}
diff --git a/x/payment/types/builder_test.go b/x/payment/types/builder_test.go
@@ -15,7 +15,7 @@ import (
 	"google.golang.org/grpc"
 )
 
-func TestBuildPayForMessage(t *testing.T) {
+func TestBuildWirePayForMessage(t *testing.T) {
 	testRing := generateKeyring(t)
 
 	info, err := testRing.Key(testAccName)
@@ -36,8 +36,8 @@ func TestBuildPayForMessage(t *testing.T) {
 	rawTx, err := makeEncodingConfig().TxConfig.TxEncoder()(signedTx)
 	require.NoError(t, err)
 
-	_, _, isChild := coretypes.UnwrapMalleatedTx(rawTx)
-	require.False(t, isChild)
+	_, _, isMalleated := coretypes.UnwrapMalleatedTx(rawTx)
+	require.False(t, isMalleated)
 
 	sigs, err := signedTx.GetSignaturesV2()
 	require.NoError(t, err)

diff --git a/x/payment/types/payformessage_test.go b/x/payment/types/payformessage_test.go
@@ -2,8 +2,11 @@ package types
 
 import (
 	"bytes"
+	"fmt"
 	"testing"
 
+	sdkclient "github.com/cosmos/cosmos-sdk/client"
+	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 	authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
 	"github.com/stretchr/testify/assert"
@@ -136,11 +139,15 @@ func TestPadMessage(t *testing.T) {
 	}
 }
 
-func TestSignShareCommitments(t *testing.T) {
+// TestSignMalleatedTxs checks to see that the signatures that are generated for
+// the PayForMessages malleated from the original WirePayForMessage are actually
+// valid.
+func TestSignMalleatedTxs(t *testing.T) {
 	type test struct {
 		name    string
 		ns, msg []byte
-		ss      uint64
+		ss      []uint64
+		options []TxBuilderOption
 	}
 
 	kb := generateKeyring(t, "test")
@@ -149,52 +156,67 @@ func TestSignShareCommitments(t *testing.T) {
 
 	tests := []test{
 		{
-			name: "single share square size 2",
-			ns:   []byte{1, 1, 1, 1, 1, 1, 1, 1},
-			msg:  bytes.Repeat([]byte{1}, ShareSize-8),
-			ss:   2,
+			name:    "single share",
+			ns:      []byte{1, 1, 1, 1, 1, 1, 1, 1},
+			msg:     bytes.Repeat([]byte{1}, ShareSize-8),
+			ss:      []uint64{2, 4, 8, 16},
+			options: []TxBuilderOption{SetGasLimit(2000000)},
 		},
 		{
-			name: "15 shares square size 4",
+			name: "15 shares",
 			ns:   []byte{1, 1, 1, 1, 1, 1, 1, 2},
-			msg:  bytes.Repeat([]byte{2}, ShareSize*15),
-			ss:   4,
+			msg:  bytes.Repeat([]byte{2}, ShareSize*12),
+			ss:   []uint64{4, 8, 16, 64},
+			options: []TxBuilderOption{
+				SetGasLimit(123456789),
+				SetFeeAmount(sdk.NewCoins(sdk.NewCoin("tio", sdk.NewInt(987654321))))},
 		},
 	}
 
 	for _, tt := range tests {
-		wpfm, err := NewWirePayForMessage(tt.ns, tt.msg, tt.ss)
+		wpfm, err := NewWirePayForMessage(tt.ns, tt.msg, tt.ss...)
 		require.NoError(t, err, tt.name)
-		err = wpfm.SignShareCommitments(signer, signer.NewTxBuilder())
+		err = wpfm.SignShareCommitments(signer, tt.options...)
 		// there should be no error
 		assert.NoError(t, err)
 		// the signature should exist
 		assert.Equal(t, len(wpfm.MessageShareCommitment[0].Signature), 64)
 
-		// verify the signature
-		unsignedPFM, err := wpfm.unsignedPayForMessage(tt.ss)
-		require.NoError(t, err)
-		tx, err := signer.BuildSignedTx(signer.NewTxBuilder(), unsignedPFM)
-		require.NoError(t, err)
-
-		// Generate the bytes to be signed.
-		bytesToSign, err := signer.encCfg.TxConfig.SignModeHandler().GetSignBytes(
-			signing.SignMode_SIGN_MODE_DIRECT,
-			authsigning.SignerData{
-				ChainID:       signer.chainID,
-				AccountNumber: signer.accountNumber,
-				Sequence:      signer.sequence,
-			},
-			tx,
-		)
-		require.NoError(t, err)
-
-		// verify the signature using the public key
-		assert.True(t, signer.GetSignerInfo().GetPubKey().VerifySignature(
-			bytesToSign,
-			wpfm.MessageShareCommitment[0].Signature,
-		))
-
+		// verify the signature for the PayForMessages derived from the
+		// WirePayForMessage
+		for i, size := range tt.ss {
+			unsignedPFM, err := wpfm.unsignedPayForMessage(size)
+			require.NoError(t, err)
+
+			// create a new tx builder to create an unsigned PayForMessage
+			builder := applyOptions(signer.NewTxBuilder(), tt.options...)
+			tx, err := signer.BuildSignedTx(builder, unsignedPFM)
+			require.NoError(t, err)
+
+			// Generate the bytes to be signed.
+			bytesToSign, err := signer.encCfg.TxConfig.SignModeHandler().GetSignBytes(
+				signing.SignMode_SIGN_MODE_DIRECT,
+				authsigning.SignerData{
+					ChainID:       signer.chainID,
+					AccountNumber: signer.accountNumber,
+					Sequence:      signer.sequence,
+				},
+				tx,
+			)
+			require.NoError(t, err)
+
+			// compare the commitments generated by the WirePayForMessage with
+			// that of independently generated PayForMessage
+			assert.Equal(t, unsignedPFM.MessageShareCommitment, wpfm.MessageShareCommitment[i].ShareCommitment)
+
+			// verify the signature
+			assert.True(t, signer.GetSignerInfo().GetPubKey().VerifySignature(
+				bytesToSign,
+				wpfm.MessageShareCommitment[i].Signature,
+			),
+				fmt.Sprintf("test: %s size: %d", tt.name, size),
+			)
+		}
 	}
 }
 
@@ -325,7 +347,7 @@ func TestProcessMessage(t *testing.T) {
 	for _, tt := range tests {
 		wpfm, err := NewWirePayForMessage(tt.ns, tt.msg, tt.ss)
 		require.NoError(t, err, tt.name)
-		err = wpfm.SignShareCommitments(signer, signer.NewTxBuilder())
+		err = wpfm.SignShareCommitments(signer)
 		assert.NoError(t, err)
 
 		wpfm = tt.modify(wpfm)
@@ -358,9 +380,16 @@ func validWirePayForMessage(t *testing.T) *MsgWirePayForMessage {
 
 	signer := generateKeyringSigner(t)
 
-	err = msg.SignShareCommitments(signer, signer.NewTxBuilder())
+	err = msg.SignShareCommitments(signer)
 	if err != nil {
 		panic(err)
 	}
 	return msg
 }
+
+func applyOptions(builder sdkclient.TxBuilder, options ...TxBuilderOption) sdkclient.TxBuilder {
+	for _, option := range options {
+		builder = option(builder)
+	}
+	return builder
+}
diff --git a/x/payment/types/wirepayformessage.go b/x/payment/types/wirepayformessage.go
@@ -41,10 +41,16 @@ func NewWirePayForMessage(namespace, message []byte, sizes ...uint64) (*MsgWireP
 
 // SignShareCommitments creates and signs MsgPayForMessages for each square size configured in the MsgWirePayForMessage
 // to complete each shares commitment.
-func (msg *MsgWirePayForMessage) SignShareCommitments(signer *KeyringSigner, builder sdkclient.TxBuilder) error {
+func (msg *MsgWirePayForMessage) SignShareCommitments(signer *KeyringSigner, options ...TxBuilderOption) error {
 	msg.Signer = signer.GetSignerInfo().GetAddress().String()
 	// create an entire MsgPayForMessage and signing over it, including the signature in each commitment
 	for i, commit := range msg.MessageShareCommitment {
+		builder := signer.NewTxBuilder()
+
+		for _, option := range options {
+			builder = option(builder)
+		}
+
 		sig, err := msg.createPayForMessageSignature(signer, builder, commit.K)
 		if err != nil {
 			return err