feat(cmd): adding timeout flag

[sebasti810]

fixes #3575

[jcstein]

does this time out the transaction in the mempool, or just in the client?

[sebasti810]

just in the client. is it supposed to also set it for the mempool?

[jcstein]

I'm thinking of a case where I send transaction with nonce x, it times out, and then when I go to send another transaction, also with nonce x, it errors because it has the same nonce as the tx that is already timed out on client, but in the mempool

[vgonkivs]

But node does not support nonce so far. We agreed that's it is not necessary.

[distractedm1nd]

I'm thinking of a case where I send transaction with nonce x, it times out, and then when I go to send another transaction, also with nonce x, it errors because it has the same nonce as the tx that is already timed out on client, but in the mempool

This does not happen anymore - the nonce error was fixed by upgrading app :)

But node does not support nonce so far. We agreed that's it is not necessary.

I don't think he's saying we need to support nonce, I think @jcstein was just describing that what happens with a nonce in the background was throwing errors to the user, and that a timeout for txs in the mempool could fix this.

What if we add a timeout to TxConfig in a separate issue?

[vgonkivs]

I'd rather pass it through the context.

[distractedm1nd]

Fair, I guess we can't have a timeout in the transaction itself that gets submitted, right?

[vgonkivs]

I don't want to grow the TxConfig. Timeout is smth we can configure through the context.
wdyt @distractedm1nd ?

[jcstein]

This does not happen anymore - the nonce error was fixed by upgrading app :)

😄 🚢

I think @jcstein was just describing that what happens with a nonce in the background was throwing errors to the user, and that a timeout for txs in the mempool could fix this.

yeah, i was thinking of a case where the tx is timed out on client, but when user makes new tx, there is still error due to mempool. it sounds like the nonce upgrades fixed this though

[renaynay]

@jcstein Did you test this PR already?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 51.61290% with 15 lines in your changes missing coverage. Please review.

Project coverage is 45.51%. Comparing base (2469e7a) to head (007d9ea).
Report is 162 commits behind head on main.

Files	Patch %	Lines
cmd/rpc.go	0.00%	10 Missing ⚠️
api/rpc/client/client.go	76.19%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3580      +/-   ##
==========================================
+ Coverage   44.83%   45.51%   +0.68%     
==========================================
  Files         265      274       +9     
  Lines       14620    15422     +802     
==========================================
+ Hits         6555     7020     +465     
- Misses       7313     7598     +285     
- Partials      752      804      +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[vgonkivs]

Could you describe how the user should interact with the flag? In what units the timeout should be passed, bc duration represents time in nanoseconds.?

[sebasti810]

@vgonkivs The flag docstring prints the usage, e.g. 30s, 1m. This is because pflag has a DurationVar option, which handles this parsing underneath.

And it's tested, this is the output you get when the timeline is exceeded

[jcstein]

@jcstein Did you test this PR already?

@renaynay just saw this, no I haven't tested it yet

[Wondertan]

Hey @sebasti810!

Thank you for the contribution