hashgraph / hedera-sdk-js

Hedera™ Hashgraph SDK for JavaScript/TypeScript
https://docs.hedera.com/guides/docs/sdks
Apache License 2.0
277 stars 146 forks source link

TransactionReceiptQuery does not resolve for old transactions #1063

Closed KilianB closed 2 years ago

KilianB commented 2 years ago

Description

Trying to retrieve a receipt for a transaction which does not reside on the chain anymore will never resolve or error out with default parameters.


Issue resides here:

No results will be returned with header code 18. Receipt not found.

https://github.com/hashgraph/hedera-sdk-js/blob/7a4925fe3c233b7280bc5d1f9b1409104537f019/src/Executable.js#L420-L421

image

Due to which ExecutionState.Retry; will be returned.

https://github.com/hashgraph/hedera-sdk-js/blob/7a4925fe3c233b7280bc5d1f9b1409104537f019/src/Executable.js#L445-L452

https://github.com/hashgraph/hedera-sdk-js/blob/7a4925fe3c233b7280bc5d1f9b1409104537f019/src/transaction/TransactionReceiptQuery.js#L162-L185

As the receipt will never be found and max attempts are not taken into account the request will never resolve.

Steps to reproduce

Attempt to query a receipt for a transaction which happened more than 3 minutes ago.

try {
    const res = new TransactionReceiptQuery({
      transactionId: transactionId,
      includeDuplicates: includeDuplicates,
      includeChildren: includeChildren,
    })
    //Set max attempts is not taken into account in this case
    .setMaxAttempts(2);

    const receipt = await res.execute(client);

    console.log("I will never execute");
  } catch (err) {
    logger.error(err);
}

Additional context

This can be worked around by explicitly setting the request timeout to error out.

const receipt = await res.execute(client,timeout);

While the function might work as intended it certainly was not my expectation to have a function which never returns and it took me some investigation to see what happened. I would expect an error to be thrown instead.

Hedera network

testnet

Version

v2.12.1

Operating system

No response

janaakhterov commented 2 years ago

Hmm, a default timeout might be a good idea, but the bigger issue is the fact maxAttempts isn't being used.

janaakhterov commented 2 years ago

Fixed: https://github.com/hashgraph/hedera-sdk-js/commit/d6abece2efeea9d25600fd832b6b8c7ab1ef0dc6 Closing as I think this issue is mostly about the fact that maxAttempts is not being used. Since maxAttempts to a default value of 10 technically execute() should no longer run forever.

KilianB commented 2 years ago

@danielakhterov There is a small typo peristent => persistent
Without testing, if we always run into the retry case, the persistent error property will never be set. The thrown error, in case of max attempts is reached, won't contain any useful information. Wouldn't it make sense to include the retry status code in this error message? In case of the TransactionReceiptQuery it would expose the error that the Receipt was not found to the caller.

janaakhterov commented 2 years ago

That's a good point.

janaakhterov commented 2 years ago

Fixed: https://github.com/hashgraph/hedera-sdk-js/commit/8fa46337bcd1596964b426b041dc5aa3ea423efa