feat(x/ecocredit): add query methods for orm params #1423
Conversation
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #1423 +/- ##
==========================================
+ Coverage 78.54% 78.58% +0.03%
==========================================
Files 236 240 +4
Lines 18109 18360 +251
==========================================
+ Hits 14224 14428 +204
- Misses 3076 3104 +28
- Partials 809 828 +19
|
| // AllowedDenom represents the information for an allowed ask denom. | ||
| message AllowedDenomInfo { | ||
| // denom is the bank denom to allow (ex. ibc/GLKHDSG423SGS) | ||
| string bank_denom = 1; | ||
|
|
||
| // display_denom is the denom to display to the user and is informational. | ||
| // Because the denom is likely an IBC denom, this should be chosen by | ||
| // governance to represent the consensus trusted name of the denom. | ||
| string display_denom = 2; | ||
|
|
||
| // exponent is the exponent that relates the denom to the display_denom and is | ||
| // informational | ||
| uint32 exponent = 3; | ||
| } |
There was a problem hiding this comment.
All the values provided in the AllowedDenom state message are already human-readable (i.e. there are no byte addresses that need to be converted to strings). Any reason for defining a separate message here?
There was a problem hiding this comment.
Initially I tried using AllowedDenom from the marketplace, but that is creating import cycles.
There was a problem hiding this comment.
Ah, ok. I see. Yea, this seems ok. We could also embed this within the QueryParamsResponse message given this is not native to the ecocredit.v1 package but we still need to define it separately. Ok to leave as is though.
There was a problem hiding this comment.
shouldn't this query actually go in proto/regen/ecocredit/marketplace/v1?
There was a problem hiding this comment.
It's the Params query that will be deprecated but Params is in ecocredit.v1.
There was a problem hiding this comment.
The main issue is if we have a query for params, it should return all params. The alternative is removing it and requiring users to use individual queries but keeping the params query for one more release seemed nice to have.
There was a problem hiding this comment.
I think this is ok for now. I can also clean up param proto messages a bit when updating the param names.
Description
Closes: #1402
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change