fix: change error&error codes returned from /transaction/* related endpoints #1312
Conversation
| cause, | ||
| stack, | ||
| }; | ||
| throw new BadRequest('Failed to parse transaction.'); |
There was a problem hiding this comment.
is there a way we can continue including/returning the transaction/stack information as part of the error? This info can be useful when looking into specific issues/debugging.
There was a problem hiding this comment.
Plus one to this, otherwise it looks really great!
There was a problem hiding this comment.
Yes exactly! 💯 Thank you both for pointing this out 🙏 I forgot to mention it in the PR description.
I tried to find a way to keep the stack and cause from extractCauseAndStack for this exact reason that you mention but I was still getting a 500 error at the end. I will check some more to see how I can override the StatusCode returned. Thank YOU both for your time on reviewing ! 🙏
There was a problem hiding this comment.
Thank you @TarikGul and @marshacb 🙏 to :
- point out the fact that we prefer keeping the debugging info and
- point in the direction of the
middlewarefiles for the error handling
Based on those sugegstions, I removed the BadRequest solution (and also did not push the CustomBadRequest solution which I was planning) and opted for the solution of passing the status code in the txErrorMiddleware.
This implied the following changes :
- Reverted to the previous error handling with
extractCauseAndStack - Added the
codeinerrand passing it in thetxErrorMiddleware - Reverted the changes to the test
.specfiles - Updated the mock files with the error code returned since now
erralso includes thecode - Updated
ITxLegacyError - Reverted changes in docs and rebuilt
Important Note
-
I am not setting a default value for
codein thetxErrorMiddlewareso if it is not set in the service file or from wherever it is called then it will result in a500status code and it will output a not so prettyerror: Invalid status code: undefined. Maybe we would like to handle this more gracefully ? -
TODO from my side : Update the description of this PR to reflect the final implementation.
…rrorMiddleware` - Reverted to the previous error handling with `extractCauseAndStack` - Added the `code` in error thrown and passed it in the `txErrorMiddleware` - Reverted the changes to the test `.spec` files - Updated the mock files with the error code returned since now `err` also includes the `code` - Updated `ITxLegacyError` - Reverted changes in docs and rebuilt
| @@ -22,6 +22,7 @@ export function extractCauseAndStack(err: unknown): { | |||
| cause: string | unknown; | |||
| stack: string | undefined; | |||
| } { | |||
| // const code = err instanceof Error ? err : ''; | |||
There was a problem hiding this comment.
Just need to either remove or handle this commented out code.
Relates: #1311
Description
In the
transactionrelated endpoints :/transaction/transaction/dry-run/transaction/fee-estimatethe errors are handled :
extractCauseAndStackmethod (to extract thecauseandstackof the error)txErrorMiddleware.tsfile and throws a hardcoded500error here (also hardcoded in theinfohere)With the changes in the current PR, we are now handling the errors in the following way :
extractCauseAndStackmethodtxErrorMiddleware.tsfile400instead of500txErrorMiddleware.tsfile again but now with the error code set already.This change was mentioned in the #1311 issue and it was concerning only the
/transactionendpoint. However, I think it also fits in the rest of thetransactionrelated endpoints since the logic is the same, meaning If we decide to throw a400error if a tx fails to be submitted then we should throw the same error if a tx fails to be dry-run. And also if we fail to retrieve the fee info.Return Errors & Error Codes
Example Returned Error before the change
Example Returned Error after the change
Returned Errors Comparison
So with this change :
400instead of500) andcodeerrortransactioncausestackExample Request that will/should trigger a 400 error
while ofc a sidecar instance (checked out in the current branch) is running locally.
Credits to @TarikGul and @marshacb for the very insightful observations/reviews on this PR so we can choose the best solution/implementation.