Conversation
|
dimriou
force-pushed
the
oev-1176_upgrade_handler
branch
from
cab58c2 to
d34d05d
Compare
| txErr error, | ||
| txStore txm.TxStore, | ||
| setNonce func(common.Address, uint64), | ||
| isFromBroadcastMethod bool, |
There was a problem hiding this comment.
Let's remove the unused isFromBroadcastMethod while we're touching this func anyway
| txStore txm.TxStore, | ||
| setNonce func(common.Address, uint64), | ||
| isFromBroadcastMethod bool, | ||
| ) (noTransmission bool, err error) { |
There was a problem hiding this comment.
Nitpick: I somewhat dislike negations in var names, since it will lead to double negations, which can lead to confusion.
Maybe flip it around and use something like transmissionHappened?
There was a problem hiding this comment.
Seems reasonable to me, but the issue with this particular case is that there is guarantee a transmission happened and we can only be sure about the noTransmission. If we flip the name to transmissionHappened or transmissionMayHappened and we check for the inversed result, the reader might assume that the error handler can deterministically returned if a transaction was broadcasted. It feels like noTransmission provides better conviction as to what happened.
There was a problem hiding this comment.
Ah I see, so otherwise it would be transmissionMightHaveHappened, which is not great 😅
Let's leave it like this then. Might be worthwhile to add a comment to the code about that we can only be 100% sure a transmission has not happened
| } | ||
| setNonce(tx.FromAddress, *tx.Nonce) | ||
| return fmt.Errorf("transaction with txID: %d marked as fatal", tx.ID) | ||
| lggr.Infof("transaction with txID: %d marked as fatal", tx.ID) |
There was a problem hiding this comment.
Can we log the transactionLifecycleID here?
| if hErr != nil { | ||
| return hErr | ||
| } | ||
| if noTransmission { |
There was a problem hiding this comment.
Would it be helpful to add a debug log here?
There was a problem hiding this comment.
For now the only ErrorHandler we have implemented is the MetaErrorHandler which already logs a message if noTransmission is true, so I think we already get the information from that.
dimriou
force-pushed
the
oev-1176_upgrade_handler
branch
from
5cb2762 to
156bdfc
Compare
dimriou
force-pushed
the
oev-1176_upgrade_handler
branch
from
156bdfc to
37483bb
Compare
2 participants
This PR upgrades error handler for TXMv2. We no longer throw an error if the handler decides there was no transmission, which results in a better observability. Additionally, if the error handler fails to parse the error, txm will check if the pending nonce increased instead of returning an error, which follows the standard flow.