Check receipt of sent transaction #323
Conversation
cam-schultz
requested review from
michaelkaplan13,
minghinmatthewlam,
geoff-vball and
bernard-avalabs
as code owners
| @@ -15,10 +15,8 @@ import ( | |||
|
|
|||
| var ErrInvalidEndpoint = errors.New("invalid rpc endpoint") | |||
|
|
|||
| type Client ethclient.Client | |||
There was a problem hiding this comment.
If we're removing the Client type here, should we rename the package/file?
I think would better belong under utils now, since it is just a utility for creating a subnet-evm/ethclient instance with headers/query params.
| // Wait for the message to be included in a block before returning | ||
| callCtx, callCtxCancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer callCtxCancel() | ||
| receipt, err := utils.CallWithRetry[*types.Receipt]( | ||
| callCtx, | ||
| func() (*types.Receipt, error) { | ||
| return destinationClient.Client().(ethclient.Client).TransactionReceipt(callCtx, txHash) | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| m.logger.Error( | ||
| "Failed to get transaction receipt", | ||
| zap.String("destinationBlockchainID", destinationBlockchainID.String()), | ||
| zap.String("warpMessageID", signedMessage.ID().String()), | ||
| zap.String("teleporterMessageID", teleporterMessageID.String()), | ||
| zap.Error(err), | ||
| ) | ||
| return err | ||
| } | ||
| if receipt.Status != types.ReceiptStatusSuccessful { | ||
| m.logger.Error( | ||
| "Transaction failed", | ||
| zap.String("destinationBlockchainID", destinationBlockchainID.String()), | ||
| zap.String("warpMessageID", signedMessage.ID().String()), | ||
| zap.String("teleporterMessageID", teleporterMessageID.String()), | ||
| zap.String("txHash", txHash.String()), | ||
| ) | ||
| return fmt.Errorf("transaction failed with status: %d", receipt.Status) | ||
| } |
There was a problem hiding this comment.
Can we move this section out to a helper? It also is close to duplicating the utility functions we have in Teleporter for use in the E2E such as WaitMined. Would it make sense to export those from a non-test package to use here?
There was a problem hiding this comment.
I think this logic should continue to live here for a couple reasons:
- Moving
WaitMinedto a non-test utility inteleporterdoesn't make a ton of sense, since there's nothing specific to Teleporter in that function. It's really a utility to support our current test framework awm-relayerhas slightly different semantics. InWaitMined, we also wait for receipt's block height to account for differences in network views among nodes behind the public API. This is necessary to avoid races in the tests when run on Fuji. In the relayer, we instead assume that a valid receipt indicates that the tx will eventually be seen by all nodes.
| ) | ||
| return fmt.Errorf("transaction failed with status: %d", receipt.Status) | ||
| } | ||
|
|
||
| m.logger.Info( |
There was a problem hiding this comment.
Should this message be kept directly after the SendTx call? Alternatively, we could re-word it to say "delivered" rather than "sent". Would be great to add the txHash to it now that it's available also.
There was a problem hiding this comment.
Reworded to "delivered". This log message is intended to signal successful delivery.
| err error | ||
| ) | ||
| for { | ||
| t, err = f() |
There was a problem hiding this comment.
Should we state in the comments that we assume the time to call f() is significant less than the ticker delay? This should generally be true unless f() itself blocks or has some sleep logic.
Otherwise the ticker will not have the intended effect, and we should only start the ticker after f() completes in each loop iteration.
There was a problem hiding this comment.
Go's time.Ticker type "will adjust the time interval or drop ticks to make up for slow receivers" (docs). This is implemented using a buffered channel with a single element; if the client falls behind, new ticks are dropped.
I don't think we should make any assumptions on f, especially since f is usually a blocking network request. time.Ticker should account for arbitrarily expensive fs though.
cam-schultz
dismissed stale reviews from bernard-avalabs and geoff-vball
via
7f73574
cam-schultz
requested review from
michaelkaplan13,
geoff-vball and
bernard-avalabs
Why this should be merged
Fixes #321
Fixes #236
Modifies the retry mechanism introduced in #317 to retry a generic function in a loop. It runs until the context provided by the caller is cancelled, or the call is successful. The utility uses the same retry pattern implemented here: https://github.com/ava-labs/teleporter/blob/main/tests/utils/utils.go#L519
How this works
The
MessageHandler.SendMessagemethod waits until the API node returns the transaction receipt. It returns an error if either the receipt is not found within a time bound, or the receipt status is not successful. That error will be propagated toApplicationRelayer.ProcessHeight, causing the height to not be committed to the database. The error will also be propagated to theListener, which will mark the relayer as unhealthy.Also cleans up some of the
awm-relayer/ethclientusage. That package really only contains a customsubnet-evm/ethclientconstructor that enables query parameters and http headers. Wrapping thesubnet-evmtype was overkill.How this was tested
CI. In the
AllowedAddressesE2E test, the wait is increased before killing the relayer application to allow time for receipt fetching + the database write interval.How is this documented
N/A