refactor: Imporve signer confirm tx (#2970)

This PR using a `Ticker` for periodically call `GetTx` by given hash
instead of a `Timer`. This makes the code clean and avoids calling reset
`Timer` multiple times.

It's similar to `WaitMined` in
[geth](https://github.com/ethereum/go-ethereum/blob/09e0208029ff96a9cda0c69dbaebfd3f31a39771/accounts/abi/bind/util.go#L32).

Other changes:
* Add lock when get `pollTime`

---------

Co-authored-by: Rootul Patel <[email protected]>

pkg/user/signer.go

@@ -211,31 +211,31 @@ func (s *Signer) BroadcastTx(ctx context.Context, txBytes []byte) (*sdktypes.TxR
 // is encountered.
 func (s *Signer) ConfirmTx(ctx context.Context, txHash string) (*sdktypes.TxResponse, error) {
 	txClient := tx.NewServiceClient(s.grpc)
-	timer := time.NewTimer(0)
-	defer timer.Stop()
+
+	s.mtx.RLock()
+	pollTime := s.pollTime
+	s.mtx.RUnlock()
+
+	pollTicker := time.NewTicker(pollTime)
+	defer pollTicker.Stop()
+
 	for {
+		resp, err := txClient.GetTx(ctx, &tx.GetTxRequest{Hash: txHash})
+		if err == nil {
+			if resp.TxResponse.Code != 0 {
+				return resp.TxResponse, fmt.Errorf("tx failed with code %d: %s", resp.TxResponse.Code, resp.TxResponse.RawLog)
+			}
+			return resp.TxResponse, nil
+		}
+		if !strings.Contains(err.Error(), "not found") {
+			return &sdktypes.TxResponse{}, err
+		}
+
+		// Wait for the next round.
 		select {
 		case <-ctx.Done():
 			return &sdktypes.TxResponse{}, ctx.Err()
-		case <-timer.C:
-			resp, err := txClient.GetTx(
-				ctx,
-				&tx.GetTxRequest{
-					Hash: txHash,
-				},
-			)
-			if err == nil {
-				if resp.TxResponse.Code != 0 {
-					return resp.TxResponse, fmt.Errorf("tx failed with code %d: %s", resp.TxResponse.Code, resp.TxResponse.RawLog)
-				}
-				return resp.TxResponse, nil
-			}
-
-			if !strings.Contains(err.Error(), "not found") {
-				return &sdktypes.TxResponse{}, err
-			}
-
-			timer.Reset(s.pollTime)
+		case <-pollTicker.C:
 		}
 	}
 }

pkg/user/signer_test.go

@@ -2,6 +2,7 @@ package user_test
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -69,12 +70,48 @@ func (s *SignerTestSuite) TestSubmitTx() {
 	require.EqualValues(t, 0, resp.Code)
 }
 
-func (s *SignerTestSuite) ConfirmTxTimeout() {
-	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
-	defer cancel()
-	_, err := s.signer.ConfirmTx(ctx, string("E32BD15CAF57AF15D17B0D63CF4E63A9835DD1CEBB059C335C79586BC3013728"))
-	require.Error(s.T(), err)
-	require.Equal(s.T(), err, context.DeadlineExceeded)
+func (s *SignerTestSuite) TestConfirmTx() {
+	t := s.T()
+
+	fee := user.SetFee(1e6)
+	gas := user.SetGasLimit(1e6)
+
+	t.Run("deadline exceeded when the context times out", func(t *testing.T) {
+		ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+		defer cancel()
+		_, err := s.signer.ConfirmTx(ctx, "E32BD15CAF57AF15D17B0D63CF4E63A9835DD1CEBB059C335C79586BC3013728")
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), context.DeadlineExceeded.Error())
+	})
+
+	t.Run("should error when tx is not found", func(t *testing.T) {
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+		_, err := s.signer.ConfirmTx(ctx, "not found tx")
+		assert.Error(t, err)
+	})
+
+	t.Run("should success when tx is found immediately", func(t *testing.T) {
+		msg := bank.NewMsgSend(s.signer.Address(), testnode.RandomAddress().(sdk.AccAddress), sdk.NewCoins(sdk.NewInt64Coin(app.BondDenom, 10)))
+		resp, err := s.submitTxWithoutConfirm([]sdk.Msg{msg}, fee, gas)
+		assert.NoError(t, err)
+		assert.NotNil(t, resp)
+		resp, err = s.signer.ConfirmTx(s.ctx.GoContext(), resp.TxHash)
+		assert.NoError(t, err)
+		assert.Equal(t, abci.CodeTypeOK, resp.Code)
+	})
+
+	t.Run("should error when tx is found with a non-zero error code", func(t *testing.T) {
+		balance := s.queryCurrentBalance(t)
+		// Create a msg send with out of balance, ensure this tx fails
+		msg := bank.NewMsgSend(s.signer.Address(), testnode.RandomAddress().(sdk.AccAddress), sdk.NewCoins(sdk.NewInt64Coin(app.BondDenom, 1+balance)))
+		resp, err := s.submitTxWithoutConfirm([]sdk.Msg{msg}, fee, gas)
+		assert.NoError(t, err)
+		assert.NotNil(t, resp)
+		resp, err = s.signer.ConfirmTx(s.ctx.GoContext(), resp.TxHash)
+		assert.Error(t, err)
+		assert.NotEqual(t, abci.CodeTypeOK, resp.Code)
+	})
 }
 
 // TestGasConsumption verifies that the amount deducted from a user's balance is
@@ -118,3 +155,19 @@ func (s *SignerTestSuite) queryCurrentBalance(t *testing.T) int64 {
 	require.NoError(t, err)
 	return balanceResp.Balances.AmountOf(app.BondDenom).Int64()
 }
+
+func (s *SignerTestSuite) submitTxWithoutConfirm(msgs []sdk.Msg, opts ...user.TxOption) (*sdk.TxResponse, error) {
+	txBytes, err := s.signer.CreateTx(msgs, opts...)
+	if err != nil {
+		return nil, err
+	}
+
+	resp, err := s.signer.BroadcastTx(s.ctx.GoContext(), txBytes)
+	if err != nil {
+		return nil, err
+	}
+	if resp.Code != 0 {
+		return resp, fmt.Errorf("tx failed with code %d: %s", resp.Code, resp.RawLog)
+	}
+	return resp, nil
+}