feat(ecocredit): allow denom proposal handler #1072

technicallyty · 2022-05-02T02:32:40Z

Description

adds gov handler for ask denoms

Closes: #1045

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)

codecov · 2022-05-02T17:46:08Z

Codecov Report

Merging #1072 (419a93c) into master (8231250) will decrease coverage by 6.24%.
The diff coverage is 65.29%.

@@            Coverage Diff             @@
##           master    #1072      +/-   ##
==========================================
- Coverage   68.05%   61.81%   -6.25%     
==========================================
  Files         206      207       +1     
  Lines       20839    19063    -1776     
==========================================
- Hits        14182    11783    -2399     
- Misses       5352     6025     +673     
+ Partials     1305     1255      -50

Flag	Coverage Δ
experimental-codecov	`?`
stable-codecov	`61.81% <65.29%> (+0.11%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

proto/regen/ecocredit/v1/events.proto

x/ecocredit/client/marketplace/proposal.go

x/ecocredit/core/msg_credit_type.go

x/ecocredit/marketplace/msg_ask_denom.go

ryanchristo

Looking good. I didn't add suggestions for all the changes needed but we should consistently refer to this either "ask denom" or "allowed denom". I think I prefer the latter as this will be less work when we introduce buy orders and the proto state message is currently AllowedDenom.

proto/regen/ecocredit/marketplace/v1/types.proto

x/ecocredit/client/marketplace/proposal.go

x/ecocredit/marketplace/msg_allowed_denom_proposal.go

x/ecocredit/marketplace/msg_allowed_denom_proposal_test.go

x/ecocredit/marketplace/msg_allowed_denom_test.go

x/ecocredit/server/marketplace/add_ask_denom.go

aleem1314 · 2022-05-04T14:16:53Z

x/ecocredit/server/marketplace/add_ask_denom.go

+		return err
+	}
+	askDenom := p.AllowedDenom
+	if err := k.stateStore.AllowedDenomTable().Insert(sdk.WrapSDKContext(ctx), &api.AllowedDenom{


Do we need remove ask denom? With params, we could update KeyAllowedAskDenoms param to remove denom.

I think its preferred to have them in ORM even if we lose the ability to remove denoms. if its needed, i could add another handler for removing denoms, or we can wait til v0.46 where we will do msg_server based parameters management, and we can have more robust adding/removing/updating commands

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

x/ecocredit/marketplace/msg_allowed_denom_proposal.go

x/ecocredit/marketplace/msg_allowed_denom_proposal_test.go

x/ecocredit/marketplace/msg_allowed_denom_test.go

proto/regen/ecocredit/v1/events.proto

x/ecocredit/server/core/add_credit_type.go

This reverts commit 4499637.

ryanchristo

Not sure what's causing the following error:

--- FAIL: TestInitCmd (0.08s)
panic: allow-denom-proposal flag redefined: output [recovered]
	panic: allow-denom-proposal flag redefined: output

x/ecocredit/server/proposal.go

x/ecocredit/client/marketplace/proposal.go

ryanchristo · 2022-05-05T19:53:09Z

Was attempting to manually test and it looks like the command is not included in the list of available commands.

technicallyty · 2022-05-05T20:14:38Z

Was attempting to manually test and it looks like the command is not included in the list of available commands.

are you running the correct command? it's showing up for me when i do regen tx gov submit-proposal. should be the first one listed.

ryanchristo · 2022-05-05T22:07:54Z

are you running the correct command?

Whoops. I was not. Looks like the credit type proposal command is listed in regen tx ecocredit -h. I saw that and assumed it would be the same for allow denom proposal. The credit type proposal is in both at the moment. Testing now.

technicallyty · 2022-05-05T22:30:15Z

are you running the correct command?

Whoops. I was not. Looks like the credit type proposal command is listed in regen tx ecocredit -h. I saw that and assumed it would be the same for allow denom proposal. The credit type proposal is in both at the moment. Testing now.

ahh ok, i think having it show up in ecocredit's tx commands might cause errors.. if you are willing to test that lmk, i can remove if it bugs out

ryanchristo

Nice work! Tested with the updated example and again with a new denom. Would be nice to query allowed denoms from the CLI and to include those changes here.

submit a proposal (used updated example)
able to query proposal and confirmed fields
unable to confirm denom included in allowed denoms using the CLI
confirmed denom included in allowed denoms using gRPC endpoint

x/ecocredit/client/marketplace/proposal.go

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

technicallyty · 2022-05-05T22:41:03Z

Nice work! Tested with the updated example and again with a new denom. Would be nice to query allowed denoms from the CLI and to include those changes here.

submit a proposal (used updated example)

able to query proposal and confirmed fields

unable to confirm denom included in allowed denoms using the CLI

confirmed denom included in allowed denoms using gRPC endpoint

opened #1093 for the 3rd check. i would prefer to do in a follow up, might be overwhelming for an additional reviewer to consume in one go here imo

ryanchristo · 2022-05-05T22:46:20Z

tx ecocredit credit-type-proposal throws the error Error: unknown flag: --from and tx gov submit-proposal credit-type-proposal throws the error Error: unable to resolve type URL /regen.ecocredit.v1.CreditTypeProposal.

opened #1093 for the 3rd check. i would prefer to do in a follow up, might be overwhelming for an additional reviewer to consume in one go here imo

Can we maybe add fixes to credit type proposal to the issue and handle in one pull request then? Adding the query command will be a small amount of changes and I'm assuming these fixes would be too.

…osal

technicallyty · 2022-05-05T23:00:27Z

tx ecocredit credit-type-proposal throws the error Error: unknown flag: --from and tx gov submit-proposal credit-type-proposal throws the error Error: unable to resolve type URL /regen.ecocredit.v1.CreditTypeProposal.

opened #1093 for the 3rd check. i would prefer to do in a follow up, might be overwhelming for an additional reviewer to consume in one go here imo

Can we maybe add fixes to credit type proposal to the issue and handle in one pull request then? Adding the query command will be a small amount of changes and I'm assuming these fixes would be too.

just added those fixes in 6fd0058 👍🏻

ryanchristo · 2022-05-05T23:08:25Z

just added those fixes in 6fd0058 👍🏻

Tested. No longer listed in tx ecocredit and successfully added with tx gov submit-proposal.

ryanchristo

tACK. Nice work. 💪

aleem1314

lgtm

technicallyty added 12 commits April 28, 2022 16:21

wip: ask denom proposal

49506f7

chore: move proposal to core

b186457

feat: ask denom proposal handler

319c462

fix: add ask denom setup to integration tests

1949d60

fix: add ask denom to cli integration tests

5815042

chore: add proto message comment

710af25

fix: grpc test use default bond denom

69fd787

chore: cleanup

4c3fda4

feat: added event to credit type handler

912cfc1

chore: fix param changes test

962ab9c

perf: switch to single regexp match for credit type validation

d405de4

Merge branch 'master' into ty/1045-ask_denom_gov_handler

2d36690

technicallyty added 2 commits May 2, 2022 11:01

chore: fix error strings, cli doc

fc8834c

chore: cleanup

6484046

technicallyty commented May 2, 2022

View reviewed changes

technicallyty and others added 3 commits May 2, 2022 11:30

Merge branch 'master' into ty/1045-ask_denom_gov_handler

d013af7

Merge branch 'master' into ty/1045-ask_denom_gov_handler

7401937

Merge branch 'master' into ty/1045-ask_denom_gov_handler

c8ef25e

technicallyty marked this pull request as ready for review May 2, 2022 22:18

technicallyty added 4 commits May 2, 2022 17:38

fix: cli example str

0d82a5e

refactor: consolidate naming

6e1049d

chore: remove edit

412bbed

chore: cleanup

dec1443

technicallyty requested review from ryanchristo and aleem1314 May 3, 2022 01:04

ryanchristo reviewed May 4, 2022

View reviewed changes

aleem1314 reviewed May 4, 2022

View reviewed changes

technicallyty and others added 2 commits May 4, 2022 08:48

chore: apply suggestions from code review

7f2670a

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

chore: fix cli string, remove exponent check

c186699

technicallyty added 4 commits May 4, 2022 23:27

chore: address review comments

d701ec5

refactor: AllowedDenomProposal -> AllowDenomProposal

5e36330

refactor: file rename add_allowed_denom.go -> allow_denom.go

62115ce

refactor: change handle function name

19c1587

ryanchristo reviewed May 5, 2022

View reviewed changes

technicallyty added 3 commits May 5, 2022 08:27

chore: address review

10ed830

chore: use go install

4499637

Revert "chore: use go install"

436360e

This reverts commit 4499637.

ryanchristo reviewed May 5, 2022

View reviewed changes

x/ecocredit/server/proposal.go Outdated Show resolved Hide resolved

x/ecocredit/client/marketplace/proposal.go Outdated Show resolved Hide resolved

ryanchristo mentioned this pull request May 5, 2022

docs(x/ecocredit): update buf schema links #1077

Merged

13 tasks

ryanchristo changed the title ~~feat(ecocredit): ask denom proposal handler~~ feat(ecocredit): allow denom proposal handler May 5, 2022

technicallyty added 2 commits May 5, 2022 10:51

refactor: change handlefunc name

dab836c

chore: fix example string, remove flags

557a9ed

ryanchristo reviewed May 5, 2022

View reviewed changes

x/ecocredit/client/marketplace/proposal.go Outdated Show resolved Hide resolved

Update x/ecocredit/client/marketplace/proposal.go

0dafdc8

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

fix: add required gov proposal type registration for credit type prop…

6fd0058

…osal

ryanchristo approved these changes May 5, 2022

View reviewed changes

aleem1314 approved these changes May 6, 2022

View reviewed changes

Merge branch 'master' into ty/1045-ask_denom_gov_handler

419a93c

technicallyty enabled auto-merge (squash) May 6, 2022 14:59

technicallyty merged commit ea43f14 into master May 6, 2022

technicallyty deleted the ty/1045-ask_denom_gov_handler branch May 6, 2022 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecocredit): allow denom proposal handler #1072

feat(ecocredit): allow denom proposal handler #1072

technicallyty commented May 2, 2022 •

edited by zube bot

Loading

codecov bot commented May 2, 2022 •

edited

Loading

ryanchristo left a comment

aleem1314 May 4, 2022

technicallyty May 4, 2022 •

edited

Loading

ryanchristo left a comment

ryanchristo commented May 5, 2022 •

edited

Loading

technicallyty commented May 5, 2022

ryanchristo commented May 5, 2022

technicallyty commented May 5, 2022

ryanchristo left a comment

technicallyty commented May 5, 2022 •

edited

Loading

ryanchristo commented May 5, 2022 •

edited

Loading

technicallyty commented May 5, 2022

ryanchristo commented May 5, 2022

ryanchristo left a comment

aleem1314 left a comment

feat(ecocredit): allow denom proposal handler #1072

feat(ecocredit): allow denom proposal handler #1072

Conversation

technicallyty commented May 2, 2022 • edited by zube bot Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented May 2, 2022 • edited Loading

Codecov Report

ryanchristo left a comment

Choose a reason for hiding this comment

aleem1314 May 4, 2022

Choose a reason for hiding this comment

technicallyty May 4, 2022 • edited Loading

Choose a reason for hiding this comment

ryanchristo left a comment

Choose a reason for hiding this comment

ryanchristo commented May 5, 2022 • edited Loading

technicallyty commented May 5, 2022

ryanchristo commented May 5, 2022

technicallyty commented May 5, 2022

ryanchristo left a comment

Choose a reason for hiding this comment

technicallyty commented May 5, 2022 • edited Loading

ryanchristo commented May 5, 2022 • edited Loading

technicallyty commented May 5, 2022

ryanchristo commented May 5, 2022

ryanchristo left a comment

Choose a reason for hiding this comment

aleem1314 left a comment

Choose a reason for hiding this comment

technicallyty commented May 2, 2022 •

edited by zube bot

Loading

codecov bot commented May 2, 2022 •

edited

Loading

technicallyty May 4, 2022 •

edited

Loading

ryanchristo commented May 5, 2022 •

edited

Loading

technicallyty commented May 5, 2022 •

edited

Loading

ryanchristo commented May 5, 2022 •

edited

Loading