ethers.js icon indicating copy to clipboard operation
ethers.js copied to clipboard

TransactionResponse.wait timeout argument not exposed

Open jakekidd opened this issue 4 years ago • 2 comments

Describe the bug In the implementation of TransactionResponse.wait in the BaseProvider class (@ethersproject/providers/src.ts/base-provider.ts, line 1174), I can see an optional timeout argument being handled appropriately, but this argument is not exposed in the top level interface, as this wait method defined here is coerced into the TransactionResponse interface's wait method.

It would be useful to have this optional argument exposed, as tx.wait offers no possibility of having a timeout without it. It also wouldn't be a breaking change for TransactionResponse.wait to have an additional optional argument in the definition.

jakekidd avatar Jul 15 '21 16:07 jakekidd

Adding this to clarify the issue, and illustrate how I'm currently having to work around it.

In my confirmation method, I'm calling response.wait, but I'm using the "hidden" timeout parameter by first coercing response to any:

return (response as any).wait(confirmations, timeout);

(Where confirmations, timeout are numbers, and timeout is in ms.)

I can confirm this solution does indeed work for me (using ethers 5.4.2) - meaning the timeout is successful.

Especially with the recent update, I have found that ethers' TransactionResponse.wait is significantly more useful in handling waiting for confirmations than Provider.waitForTransaction. I am mostly referring to this property described in the docs:

If the transaction is replaced by another transaction, a TRANSACTION_REPLACED error will be rejected with the following properties: ...

In my case, if I have to send multiple replacement transactions that bump the gas price, it's really convenient to only have to call response.wait() on the first transaction I sent. Otherwise, I have to manage a messy Promise.race with Provider.waitForTransaction(..).

To answer the question of: why do we need a timeout anyway? - I need a timeout in place to ensure that response.wait() doesn't hang forever (which shouldn't technically happen, but I can't risk an operation, such as an RPC call, blocking it indefinitely). Basically, the justification for why we need a timeout parameter here is the same as the justification for why we need a timeout parameter in Provider.waitForTransaction.

If there's a good reason why this parameter is currently being obfuscated, however, please let me know! Otherwise, this issue may be worth addressing for everyone's convenience.

jakekidd avatar Aug 11 '21 23:08 jakekidd

@ricmoo - Would it make sense for me to open a PR exposing the timeout parameter in the TransactionResponse.wait type definition? or is that obfuscated for a reason?

image

pedroyan avatar Aug 08 '22 22:08 pedroyan

Both values are now exposed in v6. There won't be any significant API changes (like this) in v5 though.

Thanks! :)

ricmoo avatar Jan 31 '23 22:01 ricmoo