-
Notifications
You must be signed in to change notification settings - Fork 31
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
fix: fix the compatibility issues with daphne #539
Conversation
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.
Thank you for your review @ryu1-sakai, I've added my replies.
After pushing my works, I will apply your opinion.
Codecov Report
@@ Coverage Diff @@
## main #539 +/- ##
==========================================
+ Coverage 57.59% 57.69% +0.09%
==========================================
Files 793 793
Lines 86584 87013 +429
==========================================
+ Hits 49866 50198 +332
- Misses 33558 33658 +100
+ Partials 3160 3157 -3
|
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.
@0Tech, I wrote two questions and one comment about event.proto
.
proto/lbm/token/v1/query.proto
Outdated
} | ||
|
||
// Authorization queries authorization on a given operator holder pair. | ||
// if no authorization found for the request, it would return an error. |
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.
Could you write the comment to explain what kind of error clients will receive when no authorization is found?
Client need to distinguish such a not-found error from other kind of errors.
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.
I updated the comment.
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.
Thank you, I confirmed the comments! 👍
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.
LGTM
* Charge gas for custom event attributes * Introduce gas register for gas costs * Review feedback * Tests and minor updates * Godoc
Description
ref: #412
Motivation and context
The module was not compatible with the legacy one, and we concluded to maintain the most part of the interface.
Checklist:
CHANGELOG.md
client/docs/swagger-ui/swagger.yaml