-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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: add event-query-tx-for cmd to subscribe and wait for transaction #17274
Conversation
e5c9776
to
e98849e
Compare
x/auth/client/cli/query.go
Outdated
if err != nil { | ||
return err | ||
} | ||
c, err := rpchttp.New(clientCtx.NodeURI, "/websocket") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tightly coupled to CometBFT, maybe we should say something about that in Use
or Short
. Ideally the client shouldn't make too many assumptions about the server implementation, but when it's a unavoidable let's at least make them clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, see if it's ok
x/auth/client/cli/query.go
Outdated
} | ||
|
||
// QueryEventForTxCmd returns a CLI command that subscribes to a WebSocket connection and waits for a transaction event with the given hash. | ||
func QueryEventForTxCmd() *cobra.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add this command in simapp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! However, imho this should not be in auth.
Can we move it somewhere else?
Move under client/rpc/tx, see if it's ok |
Makes indeed more sense, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK. One nit.
client/rpc/tx.go
Outdated
return clientCtx.PrintProto(newResponseFormatBroadcastTxCommit(res)) | ||
} | ||
case <-ctx.Done(): | ||
return errors.ErrLogic.Wrapf("timed out waiting for event") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be more clear here, the transaction could have already been included or wasn't yet included.
client/rpc/tx.go
Outdated
return clientCtx.PrintProto(newResponseFormatBroadcastTxCommit(res)) | ||
} | ||
case <-ctx.Done(): | ||
return errors.ErrLogic.Wrapf("the transaction could have already been included or wasn't yet included") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should still keep the timeout message you had, just be more precise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I copied from the comment shamelessly
Description
allow client wait for transaction by hash after block mode is removed in #12659
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
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...
!
in the type prefix if API or client breaking change