Report Cadence transaction status #270
Conversation
WalkthroughThe recent changes to Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
services/requester/requester.go
Outdated
| @@ -232,6 +238,41 @@ func (e *EVM) signAndSend(ctx context.Context, script []byte, args ...cadence.Va | |||
| return flow.EmptyID, fmt.Errorf("failed to send transaction: %w", err) | |||
| } | |||
|
|
|||
| // get transaction status after it is submitted | |||
| go func(id flow.Identifier) { | |||
There was a problem hiding this comment.
this allows an unbounded number of gorountines to be started, that are then held open for 10+ seconds. In situations where the network pauses sealing (rare but does happen), the number could grow very large.
I think a better approach is to add new tx into a list, and create a long running worker that periodically cycles through the list checking the tx.
There was a problem hiding this comment.
yeah I do plan to do that with transaction pool, already started some work on that, but this is just something I want to get out asap because it helps with monitoring for issues, the issue for it is here #264
services/requester/requester.go
Outdated
| case <-fetchTicker.C: | ||
| res, err := e.client.GetTransactionResult(context.Background(), id) | ||
| if err != nil { | ||
| e.logger.Error().Err(err).Msg("failed to get transaction result") |
There was a problem hiding this comment.
you should include the id in the message so its easier to debug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
services/requester/requester.go (1)
Line range hint
191-275: The restructuring of thesignAndSendfunction improves readability and maintainability. However, the introduction of the goroutine to fetch transaction status asynchronously raises concerns about potential unbounded growth of goroutines, especially under network issues as noted in previous comments.Consider implementing a more robust solution such as a worker pool or a managed queue to handle the asynchronous tasks more safely and efficiently. This would prevent potential resource exhaustion and improve system stability.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- services/requester/requester.go (3 hunks)
Additional comments not posted (1)
services/requester/requester.go (1)
11-11: The import of thetimepackage is necessary for the new asynchronous functionality introduced in this PR.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- services/requester/requester.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/requester/requester.go
Description
Report transaction status after transactions are submitted, this is starting point for transaction pool.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit