Split MetaClient errors from standard RPC errors#343
Conversation
|
| return a.c.PendingNonceAt(ctx, address) | ||
| } | ||
|
|
||
| // SendTransactions handles three diffenent cases: |
There was a problem hiding this comment.
| // SendTransactions handles three diffenent cases: | |
| // SendTransactions handles three different cases: |
| if !tx.IsPurgeable { | ||
| if meta != nil && | ||
| meta.DualBroadcast != nil && *meta.DualBroadcast && meta.DualBroadcastParams != nil && meta.FwdrDestAddress != nil && | ||
| tx.AttemptCount == 1 { | ||
| meta, err := a.SendRequest(ctx, tx, attempt, *meta.DualBroadcastParams, tx.ToAddress) | ||
| if err != nil { | ||
| a.metrics.RecordSendRequestError(ctx) | ||
| return fmt.Errorf("error sending request for transactionID(%d): %w", tx.ID, ErrAuction) | ||
| } | ||
| return nil | ||
| if meta != nil { | ||
| if err := a.SendOperation(ctx, tx, attempt, *meta); err != nil { | ||
| a.metrics.RecordSendOperationError(ctx) | ||
| return fmt.Errorf("failed to send operation for transactionID(%d): %w", tx.ID, ErrAuction) | ||
| } | ||
| return nil | ||
| } | ||
| a.lggr.Infof("No bids for transactionID(%d): ", tx.ID) | ||
| return ErrNoBids | ||
| } | ||
| if len(tx.Attempts) > 1 { | ||
| a.lggr.Infow("Intercepted attempt for tx(rebroadcasting first attempt)", "txID", tx.ID, "attempt", tx.Attempts[0]) | ||
| return a.c.SendTransaction(ctx, tx.Attempts[0].SignedTransaction) |
There was a problem hiding this comment.
Maybe it's me, but I find it hard to reason about this many ifs. Is there any way to make this less indented, maybe through an early return?
There was a problem hiding this comment.
Wasn't sure about simplifying the is.IsPurgeable statement as well, so I can revert this back to three main ifs which are explained at the top of the method.
- First failed transmission
- Other attempts
- Empty transaction
Note: Part of the reason why this doesn't have much info, is because the Atlas integration was not publicly known before.
product-security-plaid-production
Bot
requested review from
agparadiso and
ilija42
There was a problem hiding this comment.
Pull request overview
This PR separates MetaClient-specific errors from standard RPC errors in the dual broadcast system. MetaClient errors are now treated as fatal on first attempt, while RPC errors trigger rebroadcasting of the first attempt instead of re-auctioning.
Changes:
- Introduced
ErrAuctionerror type to distinguish MetaClient auction/validation failures from other errors - Modified error handling to mark transactions as fatal when MetaClient errors occur on first attempt
- Updated
SendTransactionlogic to rebroadcast the first attempt for subsequent broadcast failures instead of re-auctioning - Added
UpdateSignedAttemptfunctionality to store and retrieve signed transactions for rebroadcasting
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/txmgr/builder.go | Passes inMemoryStoreManager to SelectClient for transaction store access |
| pkg/txm/storage/inmemory_store_test.go | Adds test coverage for UpdateSignedAttempt functionality |
| pkg/txm/storage/inmemory_store_manager.go | Implements UpdateSignedAttempt method in store manager |
| pkg/txm/storage/inmemory_store.go | Adds UpdateSignedAttempt method to update signed transactions on attempts |
| pkg/txm/clientwrappers/dualbroadcast/selector.go | Updates SelectClient signature to accept transaction store |
| pkg/txm/clientwrappers/dualbroadcast/meta_error_handler_test.go | Adds test for auction error handling on first attempt |
| pkg/txm/clientwrappers/dualbroadcast/meta_error_handler.go | Extends fatal error marking to include ErrAuction |
| pkg/txm/clientwrappers/dualbroadcast/meta_client.go | Implements three-case SendTransaction logic with auction error wrapping and first-attempt rebroadcasting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| a.metrics.RecordSendRequestError(ctx) | ||
| return fmt.Errorf("error sending request for transactionID(%d): %w", tx.ID, err) | ||
| return fmt.Errorf("error sending request for transactionID(%d): %w", tx.ID, ErrAuction) |
There was a problem hiding this comment.
The error wrapping is incorrect. When wrapping ErrAuction with fmt.Errorf using %w, the actual underlying error from SendRequest is being discarded. This loses important diagnostic information about why the auction failed. The line should wrap both the original error and ErrAuction, or include the original error message in the format string.
| if err := a.SendOperation(ctx, tx, attempt, *meta); err != nil { | ||
| a.metrics.RecordSendOperationError(ctx) | ||
| return fmt.Errorf("failed to send operation for transactionID(%d): %w", tx.ID, err) | ||
| return fmt.Errorf("failed to send operation for transactionID(%d): %w", tx.ID, ErrAuction) |
There was a problem hiding this comment.
The error wrapping is incorrect. When wrapping ErrAuction with fmt.Errorf using %w, the actual underlying error from SendOperation is being discarded. This loses important diagnostic information about why the operation failed. The line should wrap both the original error and ErrAuction, or include the original error message in the format string.
| return ErrNoBids | ||
| } | ||
| // #2 | ||
| if !tx.IsPurgeable && len(tx.Attempts) > 1 { |
There was a problem hiding this comment.
The condition checks len(tx.Attempts) > 1 but accesses tx.Attempts[0] on line 207. If the slice is empty or has only one element when this condition is false, accessing index 0 in case #3 (line 211) could cause a panic. The logic should ensure that tx.Attempts has at least one element before accessing index 0, or the conditions need to be restructured to guarantee safe access.
| if !tx.IsPurgeable && len(tx.Attempts) > 1 { | |
| if !tx.IsPurgeable && len(tx.Attempts) > 0 { |
dimriou
force-pushed
the
oev-795_split_meta_client_from_rpc_errors
branch
from
231b531 to
4a2d4cc
Compare
| var _ txm.Client = &MetaClient{} | ||
|
|
||
| type MetaClientTxStore interface { | ||
| UpdateSignedAttempt(_ context.Context, txID uint64, attemptID uint64, signedTransaction *evmtypes.Transaction, fromAddress common.Address) error |
There was a problem hiding this comment.
This could use a comment/bit of documentation
This PR distinguishes the errors returned by the MetaClient from the standard RPC errors and marks as fatal the former. Additionally, it for the RPC errors it now rebroadcasts a past attempt instead of reauctioning.