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

[Pitasi]

Description

Closes: #19821

After removing BROADCAST_MODE_BLOCK that allowed the client to wait for the transaction to be included in the block, checking if your transaction was successful or not involves a bit more work.

After discussing in #19821, adding a flag such as --wait would require mixing CometBFT RPC and SDK commands which are agnostic. So, the current solution is a separate command.

The new wait-tx commands features the following improvements over the removed event-query-tx-for:

• timeout is configurable using --timeout, instead of hardcoded and fixed 15 seconds
• if the tx was already present in an older block, return it without errors (the websocket event won't be fired again, so it's implemented by querying the Tx RPC after subscribing)
• add ability to pipe wait-tx to the normal tx subcommands, the hash will be parsed from stdin and the tx will be waited. Making wait-tx more convenient to use.

Note on the last point: the code tries to parse the stdin as json (e.g. works with appd tx some foo -o json | appd q wait-tx), if not, it tried to parse it as key: value pairs and looks for txhash. Maybe it's a bit brittle but I think it's good enough, comments?

Example usage

By providing the transaction hash:

$ appd q wait-tx [hash]

Or, by piping a "tx" command:

$ appd tx foo bar | appd q wait-tx
# works with -o json too
$ appd tx foo bar -o json | appd q wait-tx

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• reviewed "Files changed" and left comments if necessary
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• updated the relevant documentation or specification, including comments for documenting Go code
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed all author checklist items have been addressed
• reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

• New Features
  • Enhanced WaitTxCmd function to include features like handling timeouts, reading transaction hashes, subscribing to websocket events, and checking inclusion status.
  • Added parseHashFromInput function to extract transaction hashes.
  • Updated queryCommand to utilize rpc.WaitTxCmd() instead of rpc.QueryEventForTxCmd().
  • Introduced wait-tx command in the client module, replacing event-query-tx-for command in the CHANGELOG.md.

[coderabbitai]

Walkthrough

Walkthrough

This update introduces a new --wait flag for transaction subcommands, enabling users to wait for transactions to be included in a block directly from the command line interface. This feature includes handling timeouts, reading transaction hashes, and subscribing to websocket events for real-time updates. The changes aim to enhance the developer experience by streamlining the process of sending transactions and immediately checking their inclusion status without the need for separate commands.

Changes

File(s)	Change Summary
client/rpc/tx.go	Updated WaitTxCmd function to include enhancements like handling timeouts, reading transaction hashes, subscribing to websocket events, and checking inclusion status. Added parseHashFromInput function to extract transaction hashes from input data.
simapp/simd/cmd/commands.go	Updated queryCommand to use rpc.WaitTxCmd() instead of rpc.QueryEventForTxCmd().

Assessment against linked issues

Objective	Addressed	Explanation
Add --wait flag for all tx subcommands to wait for inclusion in a block (#19821)	✅
Implement client-side waiting for transaction inclusion, similar to BROADCAST_MODE_BLOCK (#19821)	✅
Allow specifying a timeout for how long to wait for a transaction to be included (#19821)	✅

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between 6e9528a and 1b833481220766aa11dcf3bd8cd7bc3ed45a73cb.

Files selected for processing (2)

• client/rpc/tx.go (5 hunks)
• simapp/simd/cmd/commands.go (1 hunks)

Additional comments: 3

simapp/simd/cmd/commands.go (1)

• 88-88: The addition of rpc.WaitTxCmd() aligns with the PR's objectives to replace the event-query-tx-for command. Ensure that rpc.WaitTxCmd() is thoroughly implemented and tested to guarantee its functionality and error handling.

client/rpc/tx.go (2)

• 122-175: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [92-191]

The implementation of WaitTxCmd introduces a robust mechanism for waiting on transaction inclusion with a configurable timeout. Consider the following improvements:

• Enhance error messages for better clarity and troubleshooting.
• Ensure that resources, such as websocket connections, are properly cleaned up in all error paths to prevent resource leaks.

Additionally, verify the comprehensive testing of this function, especially for edge cases like invalid transaction hashes and timeout scenarios.

• 196-212: The parseHashFromInput function effectively handles different input formats to extract transaction hashes. Consider the following improvements:

• Provide more explicit error messages to aid in troubleshooting, especially when the input format is unrecognized or invalid.
• Refactor the function for improved readability, potentially by separating the JSON and plain text parsing into distinct helper functions.

Ensure thorough testing of this function with various input formats to validate its robustness.

[coderabbitai]

Review Status

Actionable comments generated: 3

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between 6e9528a and 4d3108b.

Files selected for processing (3)

• CHANGELOG.md (1 hunks)
• client/rpc/tx.go (5 hunks)
• simapp/simd/cmd/commands.go (1 hunks)

Files skipped from review as they are similar to previous changes (1)

• simapp/simd/cmd/commands.go

Additional comments: 1

client/rpc/tx.go (1)

• 122-175: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [92-191]

The WaitTxCmd function is well-structured and follows the Uber Golang style guide closely. However, there are a few areas that could be improved for better readability and maintainability:

1. Error handling: When stopping the WebSocket client (c.Stop()) and unsubscribing (c.UnsubscribeAll), errors are explicitly ignored with //nolint:errcheck. While this might be intentional, consider logging these errors for debugging purposes, especially in production environments where diagnosing issues could become challenging.

2. Magic strings: The use of strings like "subscriber" directly in the code can make it harder to maintain and understand. Consider defining them as constants at the beginning of the file or function.

3. Error wrapping: When returning errors, especially in a context where the function interacts with external systems (like a WebSocket connection), providing more context in the error message can be very helpful for debugging. For instance, when failing to subscribe to tx events, the error message includes "failed to subscribe to tx", which is good. However, for other errors, consider adding similar context.

4. Documentation: The function could benefit from more detailed comments, especially explaining the logic behind choosing to read from stdin vs. command arguments and the process of subscribing to WebSocket events and waiting for the transaction.

+ // log errors for debugging purposes
- defer c.Stop() //nolint:errcheck // ignore stop error
+ defer func() {
+     if err := c.Stop(); err != nil {
+         fmt.Fprintf(os.Stderr, "error stopping WebSocket client: %v\n", err)
+     }
+ }()

+ // define magic strings as constants
+ const subscriberName = "subscriber"

- const subscriber = "subscriber"
+ const subscriber = subscriberName

+ // add more context to errors and document complex logic

[coderabbitai]

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.

[tac0turtle]

hey lets create a new command, replacing a command that is orthogonal to the issue isnt needed.

[julienrbrt]

I don't think we need a totally new command @tac0turtle. This is quite valid improvement to the original command and can be made not breaking.
However, imho we should definitely add an alias of the previous command in cobra, and as well of the function. This way we can backport this improvement as well.

aliassing solves the need to avoid another commant

[Pitasi]

@julienrbrt I added the aliases you mentioned. The behaviour is slightly different, the previous command returned an error even if the tx was already present, the new command doesn't return the error.

I assume it's not a deal-breaker for backwards compatibility.

[coderabbitai]

Review Status

Actionable comments generated: 2

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between 4d3108b and 5f30196.

Files selected for processing (1)

• client/rpc/tx.go (5 hunks)

Additional comments: 2

client/rpc/tx.go (2)

• 92-95: Maintaining the QueryEventForTxCmd function for backward compatibility is a thoughtful approach. It ensures that existing scripts and commands that users might have will continue to work without modification.
• 128-181: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [97-197]

The implementation of WaitTxCmd is comprehensive, covering various scenarios such as reading the transaction hash from stdin or arguments, subscribing to websocket events, and handling transactions already included in a block. A few suggestions for improvement:

1. Error Handling and Logging: When the websocket connection fails to start or when unsubscribing, consider logging these errors for better debugging and user feedback.
2. Magic Strings: Use constants for strings like event types and subscriber names to improve maintainability.
3. Timeout Handling: The error message for a timeout is clear, but consider suggesting next steps or alternatives for the user.

+ const eventTypeKey = tmtypes.EventTypeKey
+ const eventTxHashKey = tmtypes.TxHashKey
+ const subscriberName = "subscriber"

- query := fmt.Sprintf("%s='%s' AND %s='%X'", tmtypes.EventTypeKey, tmtypes.EventTx, tmtypes.TxHashKey, hash)
+ query := fmt.Sprintf("%s='%s' AND %s='%X'", eventTypeKey, tmtypes.EventTx, eventTxHashKey, hash)
- const subscriber = "subscriber"
+ const subscriber = subscriberName

- defer c.UnsubscribeAll(context.Background(), subscriber) //nolint:errcheck // ignore unsubscribe error
+ defer func() {
+     if err := c.UnsubscribeAll(context.Background(), subscriber); err != nil {
+         fmt.Fprintf(os.Stderr, "error unsubscribing: %v\n", err)
+     }
+ }()

[coderabbitai]

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"

[coderabbitai]

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")
	}

[julienrbrt]

lgtm! thank you!!

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between 5f30196 and 6095170.

Files selected for processing (1)

• CHANGELOG.md (1 hunks)

Files skipped from review as they are similar to previous changes (1)

• CHANGELOG.md