feat: add timeout when waiting for transaction to be minted #621
Conversation
WalkthroughThe codebase has been updated to include a timeout parameter for the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- benchmark/main.go (1 hunks)
- cmd/blobstream/deploy/cmd.go (2 hunks)
- evm/evm_client.go (2 hunks)
- relayer/relayer.go (1 hunks)
- relayer/relayer_test.go (2 hunks)
Files skipped from review due to trivial changes (2)
- relayer/relayer.go
- relayer/relayer_test.go
Additional comments: 5
cmd/blobstream/deploy/cmd.go (2)
3-9: The import of the "time" package is necessary for the timeout feature and is correctly placed in the import block.
142-145: The
WaitForTransactionfunction call correctly includes the new timeout parameter. Ensure that all other calls to this function in the codebase are updated to include the timeout parameter as well.evm/evm_client.go (3)
236-237: The context with timeout is correctly implemented, ensuring that the function will not wait indefinitely for a transaction to be mined.
234-241: The logging within the WaitForTransaction function is sufficient and provides useful information for debugging.
229-241: Ensure that all calls to the WaitForTransaction function throughout the codebase have been updated to include the new timeout parameter.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #621 +/- ##
==========================================
- Coverage 26.13% 26.10% -0.03%
==========================================
Files 29 29
Lines 3019 3022 +3
==========================================
Hits 789 789
- Misses 2135 2138 +3
Partials 95 95 ☔ View full report in Codecov by Sentry. |
relayer/relayer.go
Outdated
| _, err = r.EVMClient.WaitForTransaction(ctx, ethClient, tx) | ||
| _, err = r.EVMClient.WaitForTransaction(ctx, ethClient, tx, 15*time.Minute) |
There was a problem hiding this comment.
can we make this configurable so users can change this on the fly? perhaps for other timeouts as well, but this one feels more important
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- cmd/blobstream/base/config.go (2 hunks)
- cmd/blobstream/relayer/cmd.go (1 hunks)
- cmd/blobstream/relayer/config.go (3 hunks)
- relayer/relayer.go (4 hunks)
- testing/qgb.go (1 hunks)
Additional comments: 9
cmd/blobstream/base/config.go (1)
- 76-82: The addition of the
FlagEVMRetryTimeoutflag and its associated helper functionsGetEVMRetryTimeoutFlagandAddEVMRetryTimeoutFlagare correctly implemented and follow the existing code patterns.cmd/blobstream/relayer/cmd.go (1)
- 204-204: Verify the unit of
config.EVMRetryTimeoutand ensure it is clearly documented to avoid confusion, as the code multiplies it bytime.Minute, implying that the value is expected to be in minutes.cmd/blobstream/relayer/config.go (5)
63-70: The addition of the
retry-timeoutconfiguration parameter in theDefaultConfigTemplateis correctly implemented and documented.89-94: The
addRelayerStartFlagsfunction has been correctly updated to include the newbase.AddEVMRetryTimeoutFlagcall.112-112: The
EVMRetryTimeoutfield has been correctly added to theStartConfigstruct to store the retry timeout configuration.125-125: The default value for
EVMRetryTimeoutis set to 15, which is consistent with the summary and the expected behavior.251-257: The
parseRelayerStartFlagsfunction has been correctly updated to parse theretry-timeoutflag and set theEVMRetryTimeoutfield in theStartConfigstruct.relayer/relayer.go (1)
- 38-44: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [38-62]
The changes to the
Relayerstruct and theNewRelayerfunction to include and initialize theRetryTimeoutfield are correctly implemented. TheRetryTimeoutis also being used appropriately in theStartmethod when callingWaitForTransaction. Ensure that theRetryTimeoutis being set to a reasonable default value whereNewRelayeris called, and verify that all other occurrences ofWaitForTransactionin the codebase are updated to use theRetryTimeoutif necessary.testing/qgb.go (1)
- 49-53: The hardcoded timeout value of 30 seconds in the test setup is noted. Ensure that this value is appropriate for the intended test scenarios.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- benchmark/main.go (1 hunks)
- evm/evm_client.go (2 hunks)
- relayer/relayer_test.go (2 hunks)
- testing/blobstream.go (1 hunks)
Additional comments: 2
relayer/relayer_test.go (2)
41-45: The changes to include a timeout when waiting for a transaction in the
ProcessAttestationtest function are correctly implemented.116-118: The changes to include a timeout when waiting for a transaction in the
TestUseValsetFromP2Ptest function are correctly implemented.
|
dope, thanks for adding the timeout |
Overview
This will make the relayer wait for 15 minutes before retrying to send the transaction again. This can be helpful when the relayer gets a gas price, then there is a network congestion and the transaction isn't picked up.
Checklist
Summary by CodeRabbit
New Features
Improvements
Configuration
Testing