-
Notifications
You must be signed in to change notification settings - Fork 586
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
improvement: added module names in channel callback errors #2387
improvement: added module names in channel callback errors #2387
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2387 +/- ##
=======================================
Coverage 79.56% 79.56%
=======================================
Files 175 175
Lines 12134 12134
=======================================
Hits 9655 9655
Misses 2050 2050
Partials 429 429
|
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.
Thanks! 🙏 Nice improvement
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.
Thanks, @alizahidraja!
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.
Thanks @alizahidraja! Unfortunately with middleware, these error messages can be misleading. If we have a middleware stack which is ICS29 and ICS20, if ICS20 returns an error in the application callback, the final error message will actually indicate that the module which failed was fee-middleware (which is incorrect).
I think it might make more sense to include the portID? We could also change the notion of the "module" variable to be a "middleware stack", though it'd be nice to think this approach out fully
Thanks for the insights @colin-axner! Will update the code to cater for the issue mentioned |
Closing PR; Don't have access to the merging repo, will open a new one in the morning |
Continued here |
Description
Included Module names in channel callback errors
closes: #1728
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
) (does not apply, only updated error msg)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes