Add transaction status function for external components to use #13040
Conversation
amit-momin
force-pushed
the
BCI-3055-Add-new-TXM-method-to-allow-products-to-query-transaction-status
branch
from
febcf21 to
42c0eb5
Compare
common/client/errors.go
Outdated
|
|
||
| // Provides error classification to external components in a chain agnostic way | ||
| // Only exposes the error types that could be set in the transaction error field | ||
| type TxError interface { |
There was a problem hiding this comment.
I would personally think of another way to return status.
Maybe we discuss this in the ChainWriter design?
There was a problem hiding this comment.
Based on the ChainWriter discussions, we wouldn't return the status using this interface but I think I would still need it internally to classify errors between the Failed and Fatal state in the common TXM code
There was a problem hiding this comment.
Although to avoid adding a new file, I don't think there's a reason for this to be in client. I can move this to the common/txmgr/types/tx file instead.
amit-momin
force-pushed
the
BCI-3055-Add-new-TXM-method-to-allow-products-to-query-transaction-status
branch
from
0d3366f to
607700a
Compare
amit-momin
force-pushed
the
BCI-3055-Add-new-TXM-method-to-allow-products-to-query-transaction-status
branch
from
4ee2006 to
3e1c1f5
Compare
amit-momin
force-pushed
the
BCI-3055-Add-new-TXM-method-to-allow-products-to-query-transaction-status
branch
from
39bef86 to
9f43854
Compare
…ucts-to-query-transaction-status
…ucts-to-query-transaction-status
…ucts-to-query-transaction-status
|
|
||
| // Naming discrepancy is due to the generic transaction states introduced by ChainWriter | ||
| // Errors classified as terminally stuck are considered fatal since the transaction payload should NOT be retried by external callers | ||
| func (s *SendError) IsFatal() bool { |
There was a problem hiding this comment.
Not sure if this is a good idea. There is already a Fatal method at the top. Adding IsFatal is a bit confusing to me. Shouldn't we aggregate those? If the notion of Failed transactions for the CW includes transactions that can be retried, shouldn't we returned the retryable type there instead?
There was a problem hiding this comment.
I think there's a distinction between what's fatal/retryable at the TXM level and what's fatal/retryable(failed) at the product level. SendError classifies these errors for the TXM level so something that's classified as retryable here doesn't necessarily require any action from the product side. The only time they need to take action is once the transaction is in a completed state like fatal_error. The CW errors require us to split the reason for fatal_error into two. This is sort of where the confusion is introduced. What's fatal from the product's perspective is ZK overflow (terminally stuck) errors. The rest are considered failed which are classified as fatal in SendError.
I think I'm probably making things confusing here by trying to leverage SendError errors for CW error classification. I considered creating a wrapper but felt it was adding more bloat when we could already idenitfy ZK overflow through it.
There was a problem hiding this comment.
I think I could rename this and also update the ErrorClassifier type to make it less confusing. Would you be ok with IsFatalForProducts or I could make it CW specific with IsChainWriterFatal
There was a problem hiding this comment.
Hmm I see the distinction now. I think IsFatalForProducts is a bad name, as we would expose product level information to the underlying component. Same goes for IsChainWriterFatal. Personally, I would just keep the IsTerminallyStuck name, but I think once the txm gets degeneralize, we'll have to change that so it's not a big deal, you can keep it as it.
…ucts-to-query-transaction-status
|
jmank88
deleted the
BCI-3055-Add-new-TXM-method-to-allow-products-to-query-transaction-status
branch
Description
GetTransactionStatusmethod outlined in the ChainWriter docFinalizedstatus to tackle as separate work and discussion