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

feat(client): replace event-query-tx-for with wait-tx #19870

Merged
merged 3 commits into from
Mar 27, 2024
Merged

feat(client): replace event-query-tx-for with wait-tx #19870

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d3108b
feat(client): replace `event-query-tx-for` with `wait-tx`
Pitasi Mar 26, 2024
5f30196
feat(client): add alias `event-query-tx-for` to `wait-tx` for backwar…
Pitasi Mar 27, 2024
6095170
docs: update changelog to add alias
Pitasi Mar 27, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
* (gRPC) [#19049](https://github.com/cosmos/cosmos-sdk/pull/19049) Add debug log prints for each gRPC request.
* (x/consensus) [#19483](https://github.com/cosmos/cosmos-sdk/pull/19483) Add consensus messages registration to consensus module.
* (types) [#19759](https://github.com/cosmos/cosmos-sdk/pull/19759) Align SignerExtractionAdapter in PriorityNonceMempool Remove.
* (client) [#19870](https://github.com/cosmos/cosmos-sdk/pull/19870) Replace `event-query-tx-for` command with a new one called `wait-tx`
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry for PR #19870 correctly notes the replacement of the event-query-tx-for command with wait-tx. However, it could be enhanced by including details about the key improvements introduced by the wait-tx command, such as the configurable timeout feature, the ability to handle transactions from older blocks, and improved usability through stdin parsing. This would provide a more comprehensive overview of the changes and their benefits.

* (client) [#19870](https://github.com/cosmos/cosmos-sdk/pull/19870) Replace `event-query-tx-for` command with a new one called `wait-tx`
+ * (client) [#19870](https://github.com/cosmos/cosmos-sdk/pull/19870) Replace `event-query-tx-for` command with a new one called `wait-tx`, introducing a configurable timeout feature, the ability to return transactions from older blocks without errors, and improved usability through stdin parsing for transaction hashes.
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
* (client) [#19870](https://github.com/cosmos/cosmos-sdk/pull/19870) Replace `event-query-tx-for` command with a new one called `wait-tx`
* (client) [#19870](https://github.com/cosmos/cosmos-sdk/pull/19870) Replace `event-query-tx-for` command with a new one called `wait-tx`, introducing a configurable timeout feature, the ability to return transactions from older blocks without errors, and improved usability through stdin parsing for transaction hashes.

julienrbrt marked this conversation as resolved.
Show resolved Hide resolved

### Improvements

Expand Down
98 changes: 88 additions & 10 deletions client/rpc/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ package rpc
import (
"context"
"encoding/hex"
"encoding/json"
"fmt"
"io"
"strings"
"time"

Expand All @@ -16,8 +18,11 @@ import (
"github.com/cosmos/cosmos-sdk/client/flags"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/version"
)

const TimeoutFlag = "timeout"
Copy link
Contributor

Choose a reason for hiding this comment

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

The constant TimeoutFlag is well-named and clear, enhancing readability and maintainability. However, adding a comment describing its purpose and usage would further improve code documentation.

+ // TimeoutFlag specifies the flag name for setting the wait-tx command timeout duration.
  const TimeoutFlag = "timeout"
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
const TimeoutFlag = "timeout"
// TimeoutFlag specifies the flag name for setting the wait-tx command timeout duration.
const TimeoutFlag = "timeout"


func newTxResponseCheckTx(res *coretypes.ResultBroadcastTxCommit) *sdk.TxResponse {
if res == nil {
return nil
Expand Down Expand Up @@ -84,18 +89,36 @@ func newResponseFormatBroadcastTxCommit(res *coretypes.ResultBroadcastTxCommit)
return newTxResponseDeliverTx(res)
}

// QueryEventForTxCmd returns a CLI command that subscribes to a WebSocket connection and waits for a transaction event with the given hash.
// QueryEventForTxCmd is an alias for WaitTxCmd, kept for backwards compatibility.
func QueryEventForTxCmd() *cobra.Command {
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
return WaitTxCmd()
}

// WaitTx returns a CLI command that waits for a transaction with the given hash to be included in a block.
func WaitTxCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "event-query-tx-for [hash]",
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
Short: "Query for a transaction by hash",
Long: `Subscribes to a CometBFT WebSocket connection and waits for a transaction event with the given hash.`,
Args: cobra.ExactArgs(1),
Use: "wait-tx [hash]",
Aliases: []string{"event-query-tx-for"},
Short: "Wait for a transaction to be included in a block",
Long: `Subscribes to a CometBFT WebSocket connection and waits for a transaction event with the given hash.`,
Example: fmt.Sprintf(`By providing the transaction hash:
$ %[1]sd q wait-tx [hash]

Or, by piping a "tx" command:
$ %[1]sd tx [flags] | %[1]sd q wait-tx
`, version.AppName),
Args: cobra.MaximumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientTxContext(cmd)
if err != nil {
return err
}

timeout, err := cmd.Flags().GetDuration(TimeoutFlag)
if err != nil {
return err
}

c, err := rpchttp.New(clientCtx.NodeURI, "/websocket")
if err != nil {
return err
Expand All @@ -105,18 +128,54 @@ func QueryEventForTxCmd() *cobra.Command {
}
defer c.Stop() //nolint:errcheck // ignore stop error

ctx, cancel := context.WithTimeout(context.Background(), time.Second*15)
ctx, cancel := context.WithTimeout(context.Background(), timeout)
defer cancel()

hash := args[0]
query := fmt.Sprintf("%s='%s' AND %s='%s'", tmtypes.EventTypeKey, tmtypes.EventTx, tmtypes.TxHashKey, hash)
var hash []byte
if len(args) == 0 {
// read hash from stdin
in, err := io.ReadAll(cmd.InOrStdin())
if err != nil {
return err
}
hashByt, err := parseHashFromInput(in)
if err != nil {
return err
}

hash = hashByt
} else {
// read hash from args
hashByt, err := hex.DecodeString(args[0])
if err != nil {
return err
}

hash = hashByt
}

// subscribe to websocket events
query := fmt.Sprintf("%s='%s' AND %s='%X'", tmtypes.EventTypeKey, tmtypes.EventTx, tmtypes.TxHashKey, hash)
const subscriber = "subscriber"
eventCh, err := c.Subscribe(ctx, subscriber, query)
if err != nil {
return fmt.Errorf("failed to subscribe to tx: %w", err)
}
defer c.UnsubscribeAll(context.Background(), subscriber) //nolint:errcheck // ignore unsubscribe error

// return immediately if tx is already included in a block
res, err := c.Tx(ctx, hash, false)
if err == nil {
// tx already included in a block
res := &coretypes.ResultBroadcastTxCommit{
TxResult: res.TxResult,
Hash: res.Hash,
Height: res.Height,
}
return clientCtx.PrintProto(newResponseFormatBroadcastTxCommit(res))
}

// tx not yet included in a block, wait for event on websocket
select {
case evt := <-eventCh:
if txe, ok := evt.Data.(tmtypes.EventDataTx); ok {
Expand All @@ -128,13 +187,32 @@ func QueryEventForTxCmd() *cobra.Command {
return clientCtx.PrintProto(newResponseFormatBroadcastTxCommit(res))
}
case <-ctx.Done():
return errors.ErrLogic.Wrapf("timed out waiting for event, the transaction could have already been included or wasn't yet included")
return errors.ErrLogic.Wrapf("timed out waiting for transaction %X to be included in a block", hash)
}
return nil
},
}

flags.AddTxFlagsToCmd(cmd)
cmd.Flags().Duration(TimeoutFlag, 15*time.Second, "The maximum time to wait for the transaction to be included in a block")
flags.AddQueryFlagsToCmd(cmd)

return cmd
}

func parseHashFromInput(in []byte) ([]byte, error) {
var resultTx coretypes.ResultTx
if err := json.Unmarshal(in, &resultTx); err == nil {
// input was JSON, return the hash
return resultTx.Hash, nil
}

// try to parse the hash from the output of a tx command
lines := strings.Split(string(in), "\n")
for _, line := range lines {
if strings.HasPrefix(line, "txhash:") {
hash := strings.TrimSpace(line[len("txhash:"):])
return hex.DecodeString(hash)
}
}
return nil, fmt.Errorf("txhash not found")
}
Comment on lines +202 to +218
Copy link
Contributor

Choose a reason for hiding this comment

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

The parseHashFromInput function is crucial for the flexibility of the wait-tx command. The previous comments about error handling and magic strings are still valid. Additionally, consider handling the case where the input is neither valid JSON nor contains a recognizable txhash: prefix more gracefully, perhaps by providing more detailed error messages or suggestions.

+ const txHashPrefix = "txhash:"

- if strings.HasPrefix(line, "txhash:") {
+ if strings.HasPrefix(line, txHashPrefix) {
- return nil, fmt.Errorf("txhash not found")
+ return nil, fmt.Errorf("unable to find or parse txhash from input")
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
func parseHashFromInput(in []byte) ([]byte, error) {
var resultTx coretypes.ResultTx
if err := json.Unmarshal(in, &resultTx); err == nil {
// input was JSON, return the hash
return resultTx.Hash, nil
}
// try to parse the hash from the output of a tx command
lines := strings.Split(string(in), "\n")
for _, line := range lines {
if strings.HasPrefix(line, "txhash:") {
hash := strings.TrimSpace(line[len("txhash:"):])
return hex.DecodeString(hash)
}
}
return nil, fmt.Errorf("txhash not found")
}
func parseHashFromInput(in []byte) ([]byte, error) {
var resultTx coretypes.ResultTx
if err := json.Unmarshal(in, &resultTx); err == nil {
// input was JSON, return the hash
return resultTx.Hash, nil
}
// try to parse the hash from the output of a tx command
lines := strings.Split(string(in), "\n")
const txHashPrefix = "txhash:"
for _, line := range lines {
if strings.HasPrefix(line, txHashPrefix) {
hash := strings.TrimSpace(line[len(txHashPrefix):])
return hex.DecodeString(hash)
}
}
return nil, fmt.Errorf("unable to find or parse txhash from input")
}

2 changes: 1 addition & 1 deletion simapp/simd/cmd/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func queryCommand() *cobra.Command {
}

cmd.AddCommand(
rpc.QueryEventForTxCmd(),
rpc.WaitTxCmd(),
server.QueryBlockCmd(),
authcmd.QueryTxsByEventsCmd(),
server.QueryBlocksCmd(),
Expand Down
Loading