-
Notifications
You must be signed in to change notification settings - Fork 40
Return error if any in mempool CheckTx and fix callback
#311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
When call to the underlying (cosmos) `CheckTx` fails make sure that the error is returned from mempool `CheckTx`. Fix check for callback to assure result is non-nil before callback function is executed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 57.24% 57.27% +0.03%
==========================================
Files 257 257
Lines 34597 34596 -1
==========================================
+ Hits 19804 19814 +10
+ Misses 13205 13203 -2
+ Partials 1588 1579 -9
🚀 New features to boost your workflow:
|
| if err != nil { | ||
| removeHandler(true) | ||
| res.Log = txmp.AppendCheckTxErr(res.Log, err.Error()) | ||
| return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would break the existing broadcast logic if we return err instead of putting err in the response. Currently error is already being handled by adding to the res.log and passed into cb function back to the client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a second though, I think it makes more sense to return error to the client rather than putting error in the response log, so the change LGTM. Probably need to evaluate whether there's any risk/impact to the existing client or not.
| if err != nil { | ||
| removeHandler(true) | ||
| res.Log = txmp.AppendCheckTxErr(res.Log, err.Error()) | ||
| return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a second though, I think it makes more sense to return error to the client rather than putting error in the response log, so the change LGTM. Probably need to evaluate whether there's any risk/impact to the existing client or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems reasonable to me. We'll want to figure out a good way to confirm this is safe. I'm a little worried about unintended risk here (like some memory leak on failures or some underlying assumption for the current behavior)
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
When call to the underlying (cosmos)
CheckTxfails make sure that the error is returned from mempoolCheckTx.Fix check for callback to assure result is non-nil before callback function is executed.