Conversation
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
product-security-plaid-production
bot
requested review from
patrickhuie19,
pavel-raykov and
silaslenihan
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM
Improves observability for EVM SubmitTransaction failures (especially around receipt retrieval) and fixes a formatting issue in remote capability request logging to make troubleshooting “write report” paths easier.
Changes:
- Capture and log the last receipt-retrieval error from
retry.DoinevmService.SubmitTransaction. - Return a more detailed error on receipt-retrieval failure (including both “last” and “retry” errors).
- Fix a log call that previously used
Warnwith printf-style formatting by switching toWarnf.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/services/relay/evm/evm_service.go | Adds logging and richer error propagation for failed receipt retrieval during SubmitTransaction. |
| core/capabilities/remote/executable/request/client_request.go | Fixes incorrect logger method usage so formatted warning output is correct. |
Areas needing scrupulous human review:
core/services/relay/evm/evm_service.go:288-305— retry/error composition and whether returned error strings are relied upon by callers/tests; confirm behavior under context timeout/cancel.core/services/relay/evm/evm_service.go:289-300— confirmretry.Dosemantics (e.g., whether the closure can be skipped on early context cancellation) match the newlastReceiptErrapproach.
Reviewer recommendations (from CODEOWNERS):
- For
core/capabilities/...: @smartcontractkit/keystone,@smartcontractkit/capabilities-team - For overall/core ownership: @smartcontractkit/foundations,
@smartcontractkit/core
| if err != nil { | ||
| return nil, fmt.Errorf("failed getting transaction receipt. %w", err) | ||
| e.logger.Warnw("Failed getting transaction receipt", "txID", txID, "lastErr", lastReceiptErr, "retryErr", err) | ||
| return &evm.TransactionResult{TxStatus: evm.TxFatal, TxIdempotencyKey: txID}, fmt.Errorf("last err: %w retry err: %w", lastReceiptErr, err) |
|
|
|
| return &evm.TransactionResult{TxStatus: txStatus, TxIdempotencyKey: txID}, nil | ||
| } | ||
|
|
||
| var lastReceiptErr error |
There was a problem hiding this comment.
this seems like an ad-hoc solution. Could you please update https://github.com/smartcontractkit/chainlink-common/blob/main/pkg/utils/retry/retry.go with error wrapping so that the latest error is also returned as the retry.Do resut?
There was a problem hiding this comment.
nice catch, will do.
Improve troubleshooting WriteReport errors. The retry.Do is swalloging internal error, making it hard to troubleshoot
Requires
Supports