feat(x/ecocredit): add query methods for orm params #1423

aleem1314 · 2022-08-29T08:03:09Z

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...

Reviewers 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...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

proto/regen/ecocredit/v1/query.proto

x/ecocredit/client/query.go

x/ecocredit/genesis/genesis.go

x/ecocredit/server/core/query_credit_class_fees_test.go

ryanchristo

Looking good.

CHANGELOG.md

proto/regen/ecocredit/v1/query.proto

x/ecocredit/client/query.go

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

codecov · 2022-08-30T13:23:44Z

Codecov Report

Merging #1423 (f7bd8c9) into main (2ac14ac) will increase coverage by 0.03%.
The diff coverage is 76.06%.

❗ Current head f7bd8c9 differs from pull request most recent head 28cd0e6. Consider uploading reports for the commit 28cd0e6 to get more accurate results

@@            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

Impacted Files	Coverage Δ
x/ecocredit/server/genesis.go	`53.84% <ø> (+9.66%)`	⬆️
x/ecocredit/genesis/genesis.go	`41.19% <35.71%> (-0.40%)`	⬇️
x/ecocredit/server/core/query_credit_class_fees.go	`50.00% <50.00%> (ø)`
...credit/server/core/query_allowed_class_creators.go	`53.84% <53.84%> (ø)`
x/ecocredit/server/core/query_allowlist_enabled.go	`62.50% <62.50%> (ø)`
x/ecocredit/server/core/query_params.go	`55.73% <66.66%> (+4.57%)`	⬆️
x/ecocredit/client/query.go	`62.26% <71.69%> (+3.60%)`	⬆️
x/ecocredit/module/module.go	`56.42% <75.00%> (-0.93%)`	⬇️
x/ecocredit/basket/default.go	`100.00% <100.00%> (ø)`
x/ecocredit/client/testsuite/query.go	`99.51% <100.00%> (+0.03%)`	⬆️
... and 83 more

ryanchristo

Looks good. Nice work! One quick question/suggestion.

ryanchristo · 2022-08-30T15:51:25Z

proto/regen/ecocredit/v1/query.proto

+// 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;
+}


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?

Initially I tried using AllowedDenom from the marketplace, but that is creating import cycles.

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.

shouldn't this query actually go in proto/regen/ecocredit/marketplace/v1?

It's the Params query that will be deprecated but Params is in ecocredit.v1.

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.

I think this is ok for now. I can also clean up param proto messages a bit when updating the param names.

x/ecocredit/server/core/query_params.go

ryanchristo

tACK

technicallyty

LGTM

aleem1314 and others added 4 commits August 29, 2022 13:32

feat: add proto messages

5d11234

feat: add cli tests

411cdd9

cleanup

1a1ac8a

Merge branch 'main' into aleem/1402-params-query

ae249e3

aleem1314 marked this pull request as ready for review August 29, 2022 14:35

aleem1314 requested a review from a team August 29, 2022 14:35

technicallyty reviewed Aug 29, 2022

View reviewed changes

ryanchristo changed the title ~~feat(ecocredit): add query methods for orm params~~ feat(x/ecocredit): add query methods for orm params Aug 29, 2022

ryanchristo reviewed Aug 30, 2022

View reviewed changes

aleem1314 and others added 10 commits August 30, 2022 11:14

Update x/ecocredit/client/query.go

a847d7d

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

Update proto/regen/ecocredit/v1/query.proto

16aff5e

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

Update proto/regen/ecocredit/v1/query.proto

e244878

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

Update CHANGELOG.md

92e0e79

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

Update x/ecocredit/server/core/query_credit_class_fees_test.go

823555b

Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

Update x/ecocredit/server/core/query_credit_class_fees_test.go

92a901f

Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

Update x/ecocredit/client/query.go

7480b99

Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

chore: review changes

e84556e

Merge branch 'main' into aleem/1402-params-query

8d017ea

chore: fix lint errors

f7bd8c9

aleem1314 requested review from ryanchristo and technicallyty August 30, 2022 14:30

ryanchristo reviewed Aug 30, 2022

View reviewed changes

ryanchristo approved these changes Aug 30, 2022

View reviewed changes

technicallyty approved these changes Aug 30, 2022

View reviewed changes

ryanchristo added 2 commits August 30, 2022 10:22

Merge branch 'main' into aleem/1402-params-query

eef0f80

Update x/ecocredit/server/core/query_params.go

28cd0e6

ryanchristo merged commit ac52762 into main Aug 30, 2022

ryanchristo deleted the aleem/1402-params-query branch August 30, 2022 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x/ecocredit): add query methods for orm params #1423

feat(x/ecocredit): add query methods for orm params #1423

aleem1314 commented Aug 29, 2022 •

edited by aaronc

Loading

ryanchristo left a comment

codecov bot commented Aug 30, 2022 •

edited

Loading

ryanchristo left a comment

ryanchristo Aug 30, 2022

aleem1314 Aug 30, 2022

ryanchristo Aug 30, 2022

technicallyty Aug 30, 2022

ryanchristo Aug 30, 2022

ryanchristo Aug 30, 2022

ryanchristo Aug 30, 2022

ryanchristo left a comment

technicallyty left a comment

feat(x/ecocredit): add query methods for orm params #1423

feat(x/ecocredit): add query methods for orm params #1423

Conversation

aleem1314 commented Aug 29, 2022 • edited by aaronc Loading

Description

Author Checklist

Reviewers Checklist

ryanchristo left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 30, 2022 • edited Loading

Codecov Report

ryanchristo left a comment

Choose a reason for hiding this comment

ryanchristo Aug 30, 2022

Choose a reason for hiding this comment

aleem1314 Aug 30, 2022

Choose a reason for hiding this comment

ryanchristo Aug 30, 2022

Choose a reason for hiding this comment

technicallyty Aug 30, 2022

Choose a reason for hiding this comment

ryanchristo Aug 30, 2022

Choose a reason for hiding this comment

ryanchristo Aug 30, 2022

Choose a reason for hiding this comment

ryanchristo Aug 30, 2022

Choose a reason for hiding this comment

ryanchristo left a comment

Choose a reason for hiding this comment

technicallyty left a comment

Choose a reason for hiding this comment

aleem1314 commented Aug 29, 2022 •

edited by aaronc

Loading

codecov bot commented Aug 30, 2022 •

edited

Loading