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.

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

[Feature]: add --wait flag for all tx subcommands #19821

Closed
Pitasi opened this issue Mar 21, 2024 · 7 comments · Fixed by #19870
Closed

[Feature]: add --wait flag for all tx subcommands #19821

Pitasi opened this issue Mar 21, 2024 · 7 comments · Fixed by #19870

Comments

@Pitasi
Copy link
Contributor

Pitasi commented Mar 21, 2024

Summary

Back when we had the BROADCAST_MODE_BLOCK, it was possible to send a tx from the cli and wait for it to be included in a block, showing its results.

Problem Definition

I find myself, especially during development, but not only, to send a tx like:

$ appd tx module something --yes
...
txhash: 12341243

Just to then run:

$ appd q tx 12341243

To check whether it went through correctly or not.

Proposed Feature

My proposal would be to introduce a --wait flag:

$ appd tx module something --yes --wait
code: 1212
events: ...
height: 9999
txhash: 12341243

That essentially behaves like BROADCAST_MODE_BLOCK did, but waits on the client side (e.g. by polling the node).

Maybe we should take it even further and also have the possibility of specifying a timeout:

$ appd tx module something --yes --wait --wait-timeout 10s
@alexanderbez
Copy link
Contributor

The problem is, CometBFT's RPC will most likely timeout and you need to handle that error (this is why we removed block to begin with).

However, a --wait flag could silently ignore that error and still try and query for you. WDYT @julienrbrt?

@Pitasi
Copy link
Contributor Author

Pitasi commented Mar 21, 2024

Exactly, that's what I meant with "polling the node". I also implemented similar behaviors in a couple of automated scripts that needed to send multiple transactions, but I needed them to wait for the previous transaction to complete.

@julienrbrt
Copy link
Member

julienrbrt commented Mar 22, 2024

This related to this: #18927 (comment)
I would just use the event-query-tx-for command. Otherwise we would need to add CometBFT related logic in AutoCLI and we don't want that.

@julienrbrt
Copy link
Member

Given that this question pops up often, maybe we should document this command😅

@Pitasi
Copy link
Contributor Author

Pitasi commented Mar 22, 2024

Hey Julien, thanks, I didn't know about that command!

IMHO the name and the description are not helping, the description should at least say: "Wait for a transaction to be included in a block" instead of "Query for a transaction by hash".

Additionally, I'd also like to be able to pipe two commands:

appd tx ... | appd q event-query-tx-for

Just to avoid having to copy-paste the hash.

If the transaction was already included, that command just hangs in there and finally exits with error:

timed out waiting for event, the transaction could have already been included or wasn't yet included: internal logic error

I think we can make it more resilient to not rely on websocket only, but also to normal query for the tx hash.

For now what I was able to do is this:

appd q event-query-tx-for "$(appd tx ... --yes | grep txhash | cut -d' ' -f2)"

@Pitasi
Copy link
Contributor Author

Pitasi commented Mar 22, 2024

I implemented my version of the command here: warden-protocol/wardenprotocol#115. If you would like this to be integrated in SDK directly, just let me know and I'll open a patch!

Thanks for pointing me in the right direction with event-query-tx-for 😁

@julienrbrt
Copy link
Member

Feel free to upstream improvements yes 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants