feat: improve node tx cmd (ignite#2813)

* feat(cosmosclient): more contextualized errors

This mitigates the problem 1) listed in ignite#2788. If the tendermint RPC
client returns a JSON error, now it's prefixed properly with a label
mentionning the node address.

* fix: node q tx because of empty keyring backend

* add test removed with TestNew

* remove unecessary comments

* add TODO about wrapper removal condition

* Update ignite/cmd/node.go

Co-authored-by: Alex Johnson <alex@shmeeload.xyz>

* style: use pkg/errors

Co-authored-by: Alex Johnson <alex@shmeeload.xyz>

Author: tbruyelle

ignite/cmd/account.go

@@ -70,7 +70,7 @@ func printAccounts(cmd *cobra.Command, accounts ...cosmosaccount.Account) error
 
 func flagSetKeyringBackend() *flag.FlagSet {
 	fs := flag.NewFlagSet("", flag.ContinueOnError)
-	fs.String(flagKeyringBackend, "test", "Keyring backend to store your account keys")
+	fs.String(flagKeyringBackend, string(cosmosaccount.KeyringTest), "Keyring backend to store your account keys")
 	return fs
 }
 

ignite/cmd/node.go

@@ -3,6 +3,7 @@ package ignitecmd
 import (
 	"github.com/spf13/cobra"
 
+	"github.com/ignite/cli/ignite/pkg/cosmosaccount"
 	"github.com/ignite/cli/ignite/pkg/cosmosclient"
 	"github.com/ignite/cli/ignite/pkg/xurl"
 )
@@ -31,7 +32,7 @@ func newNodeCosmosClient(cmd *cobra.Command) (cosmosclient.Client, error) {
 	var (
 		home           = getHome(cmd)
 		prefix         = getAddressPrefix(cmd)
-		node           = getRPC(cmd)
+		node           = getNode(cmd)
 		keyringBackend = getKeyringBackend(cmd)
 		keyringDir     = getKeyringDir(cmd)
 		gas            = getGas(cmd)
@@ -40,6 +41,11 @@ func newNodeCosmosClient(cmd *cobra.Command) (cosmosclient.Client, error) {
 		broadcastMode  = getBroadcastMode(cmd)
 		generateOnly   = getGenerateOnly(cmd)
 	)
+	if keyringBackend == "" {
+		// Makes cosmosclient usable for commands that doesn't expose the keyring
+		// backend flag (cosmosclient.New returns an error if it's empty).
+		keyringBackend = cosmosaccount.KeyringTest
+	}
 
 	options := []cosmosclient.Option{
 		cosmosclient.WithAddressPrefix(prefix),
@@ -64,7 +70,7 @@ func newNodeCosmosClient(cmd *cobra.Command) (cosmosclient.Client, error) {
 	return cosmosclient.New(cmd.Context(), options...)
 }
 
-func getRPC(cmd *cobra.Command) (rpc string) {
-	rpc, _ = cmd.Flags().GetString(flagNode)
+func getNode(cmd *cobra.Command) (node string) {
+	node, _ = cmd.Flags().GetString(flagNode)
 	return
 }

ignite/cmd/node_query_tx.go

@@ -5,7 +5,6 @@ import (
 	"encoding/json"
 	"fmt"
 
-	sdkclient "github.com/cosmos/cosmos-sdk/client"
 	"github.com/spf13/cobra"
 )
 
@@ -24,11 +23,12 @@ func nodeQueryTxHandler(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	rpc, err := sdkclient.NewClientFromNode(getRPC(cmd))
+	client, err := newNodeCosmosClient(cmd)
 	if err != nil {
 		return err
 	}
-	resp, err := rpc.Tx(cmd.Context(), bz, false)
+
+	resp, err := client.RPC.Tx(cmd.Context(), bz, false)
 	if err != nil {
 		return err
 	}

ignite/cmd/node_tx_bank_send.go

@@ -78,7 +78,7 @@ func nodeTxBankSendHandler(cmd *cobra.Command, args []string) error {
 	if getBroadcastMode(cmd) != flags.BroadcastBlock {
 		session.Println("Transaction waiting to be included in a block.")
 		session.Println("Run the following command to follow the transaction status:")
-		session.Printf("  ignite node --node %s q tx %s\n", getRPC(cmd), resp.TxHash)
+		session.Printf("  ignite node --node %s q tx %s\n", getNode(cmd), resp.TxHash)
 	}
 	return nil
 }

ignite/pkg/cosmosclient/bank.go

@@ -20,7 +20,7 @@ func (c Client) BankBalances(ctx context.Context, address string, pagination *qu
 
 	resp, err := c.bankQueryClient.AllBalances(ctx, req)
 	if err != nil {
-		return nil, err
+		return nil, rpcError(c.nodeAddress, err)
 	}
 	return resp.Balances, nil
 }

ignite/pkg/cosmosclient/cosmosclient.go

@@ -273,6 +273,11 @@ func New(ctx context.Context, options ...Option) (Client, error) {
 			return Client{}, err
 		}
 	}
+	// Wrap RPC client to have more contextualized errors
+	c.RPC = rpcWrapper{
+		Client:      c.RPC,
+		nodeAddress: c.nodeAddress,
+	}
 
 	statusResp, err := c.RPC.Status(ctx)
 	if err != nil {

ignite/pkg/cosmosclient/cosmosclient_test.go

@@ -273,14 +273,39 @@ func TestClientStatus(t *testing.T) {
 			NodeInfo: p2p.DefaultNodeInfo{Network: "mychain"},
 		}
 	)
-	c := newClient(t, func(s suite) {
-		s.rpcClient.EXPECT().Status(ctx).Return(expectedStatus, nil).Once()
-	})
+	tests := []struct {
+		name          string
+		expectedError string
+		setup         func(suite)
+	}{
+		{
+			name: "ok",
+			setup: func(s suite) {
+				s.rpcClient.EXPECT().Status(ctx).Return(expectedStatus, nil).Once()
+			},
+		},
+		{
+			name:          "fail",
+			expectedError: "error while requesting node 'http://localhost:26657': oups",
+			setup: func(s suite) {
+				s.rpcClient.EXPECT().Status(ctx).Return(expectedStatus, errors.New("oups")).Once()
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := newClient(t, tt.setup)
 
-	status, err := c.Status(ctx)
+			status, err := c.Status(ctx)
 
-	require.NoError(t, err)
-	assert.Equal(t, expectedStatus, status)
+			if tt.expectedError != "" {
+				require.EqualError(t, err, tt.expectedError)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, expectedStatus, status)
+		})
+	}
 }
 
 func TestClientCreateTx(t *testing.T) {

ignite/pkg/cosmosclient/rpc.go

@@ -0,0 +1,156 @@
+package cosmosclient
+
+import (
+	"context"
+
+	"github.com/pkg/errors"
+	"github.com/tendermint/tendermint/libs/bytes"
+	"github.com/tendermint/tendermint/rpc/client"
+	rpcclient "github.com/tendermint/tendermint/rpc/client"
+	ctypes "github.com/tendermint/tendermint/rpc/core/types"
+	"github.com/tendermint/tendermint/types"
+)
+
+// rpcWrapper is a rpclient.Client but with more contextualized errors.
+// Useful because the original implementation may return JSON errors when the
+// requested node is busy, which is confusing for the user. With rpcWrapper,
+// the error is prefixed with 'error while requesting node xxx: JSON error'.
+// TODO(tb): we may remove this wrapper once https://github.com/tendermint/tendermint/issues/9312 is fixed.
+type rpcWrapper struct {
+	rpcclient.Client
+	nodeAddress string
+}
+
+func rpcError(node string, err error) error {
+	return errors.Wrapf(err, "error while requesting node '%s'", node)
+}
+
+func (rpc rpcWrapper) ABCIInfo(ctx context.Context) (*ctypes.ResultABCIInfo, error) {
+	res, err := rpc.Client.ABCIInfo(ctx)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) ABCIQuery(ctx context.Context, path string, data bytes.HexBytes) (*ctypes.ResultABCIQuery, error) {
+	res, err := rpc.Client.ABCIQuery(ctx, path, data)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) ABCIQueryWithOptions(ctx context.Context, path string, data bytes.HexBytes, opts client.ABCIQueryOptions) (*ctypes.ResultABCIQuery, error) {
+	res, err := rpc.Client.ABCIQueryWithOptions(ctx, path, data, opts)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) BroadcastTxCommit(ctx context.Context, tx types.Tx) (*ctypes.ResultBroadcastTxCommit, error) {
+	res, err := rpc.Client.BroadcastTxCommit(ctx, tx)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) BroadcastTxAsync(ctx context.Context, tx types.Tx) (*ctypes.ResultBroadcastTx, error) {
+	res, err := rpc.Client.BroadcastTxAsync(ctx, tx)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) BroadcastTxSync(ctx context.Context, tx types.Tx) (*ctypes.ResultBroadcastTx, error) {
+	res, err := rpc.Client.BroadcastTxSync(ctx, tx)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) GenesisChunked(ctx context.Context, n uint) (*ctypes.ResultGenesisChunk, error) {
+	res, err := rpc.Client.GenesisChunked(ctx, n)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) BlockchainInfo(ctx context.Context, minHeight int64, maxHeight int64) (*ctypes.ResultBlockchainInfo, error) {
+	res, err := rpc.Client.BlockchainInfo(ctx, minHeight, maxHeight)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) NetInfo(ctx context.Context) (*ctypes.ResultNetInfo, error) {
+	res, err := rpc.Client.NetInfo(ctx)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) DumpConsensusState(ctx context.Context) (*ctypes.ResultDumpConsensusState, error) {
+	res, err := rpc.Client.DumpConsensusState(ctx)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) ConsensusState(ctx context.Context) (*ctypes.ResultConsensusState, error) {
+	res, err := rpc.Client.ConsensusState(ctx)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) ConsensusParams(ctx context.Context, height *int64) (*ctypes.ResultConsensusParams, error) {
+	res, err := rpc.Client.ConsensusParams(ctx, height)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) Health(ctx context.Context) (*ctypes.ResultHealth, error) {
+	res, err := rpc.Client.Health(ctx)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) Block(ctx context.Context, height *int64) (*ctypes.ResultBlock, error) {
+	res, err := rpc.Client.Block(ctx, height)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) BlockByHash(ctx context.Context, hash []byte) (*ctypes.ResultBlock, error) {
+	res, err := rpc.Client.BlockByHash(ctx, hash)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) BlockResults(ctx context.Context, height *int64) (*ctypes.ResultBlockResults, error) {
+	res, err := rpc.Client.BlockResults(ctx, height)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) Commit(ctx context.Context, height *int64) (*ctypes.ResultCommit, error) {
+	res, err := rpc.Client.Commit(ctx, height)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) Validators(ctx context.Context, height *int64, page *int, perPage *int) (*ctypes.ResultValidators, error) {
+	res, err := rpc.Client.Validators(ctx, height, page, perPage)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) Tx(ctx context.Context, hash []byte, prove bool) (*ctypes.ResultTx, error) {
+	res, err := rpc.Client.Tx(ctx, hash, prove)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) TxSearch(ctx context.Context, query string, prove bool, page *int, perPage *int, orderBy string) (*ctypes.ResultTxSearch, error) {
+	res, err := rpc.Client.TxSearch(ctx, query, prove, page, perPage, orderBy)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) BlockSearch(ctx context.Context, query string, page *int, perPage *int, orderBy string) (*ctypes.ResultBlockSearch, error) {
+	res, err := rpc.Client.BlockSearch(ctx, query, page, perPage, orderBy)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) Status(ctx context.Context) (*ctypes.ResultStatus, error) {
+	res, err := rpc.Client.Status(ctx)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) BroadcastEvidence(ctx context.Context, e types.Evidence) (*ctypes.ResultBroadcastEvidence, error) {
+	res, err := rpc.Client.BroadcastEvidence(ctx, e)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) UnconfirmedTxs(ctx context.Context, limit *int) (*ctypes.ResultUnconfirmedTxs, error) {
+	res, err := rpc.Client.UnconfirmedTxs(ctx, limit)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) NumUnconfirmedTxs(ctx context.Context) (*ctypes.ResultUnconfirmedTxs, error) {
+	res, err := rpc.Client.NumUnconfirmedTxs(ctx)
+	return res, rpcError(rpc.nodeAddress, err)
+}
+
+func (rpc rpcWrapper) CheckTx(ctx context.Context, tx types.Tx) (*ctypes.ResultCheckTx, error) {
+	res, err := rpc.Client.CheckTx(ctx, tx)
+	return res, rpcError(rpc.nodeAddress, err)
+}