feat(x/ecocredit): credit type gov handler #1015

technicallyty · 2022-04-13T01:46:26Z

Description

adds and registers gov handler for credit type proposals
implements CLI cmd for creating proposals
moves some code around that was causing import cycles (namely between x/ecocredit and x/ecocredit/coref

To reduce diff footprint, i've added follow ups here: #1016

Closes: #1005

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)

app/app.go

x/ecocredit/client/core/proposal.go

x/ecocredit/expected_keepers.go

x/ecocredit/module/module.go

technicallyty · 2022-04-13T02:37:44Z

--- FAIL: TestSimAppExportAndBlockedAddrs (0.22s)
panic: parameter uploadAccess not registered [recovered]
	panic: parameter uploadAccess not registered

😱 not sure what this is??

codecov · 2022-04-13T02:43:41Z

Codecov Report

Merging #1015 (00001e9) into master (5819dcb) will increase coverage by 0.10%.
The diff coverage is 64.42%.

@@            Coverage Diff             @@
##           master    #1015      +/-   ##
==========================================
+ Coverage   59.74%   59.85%   +0.10%     
==========================================
  Files         226      231       +5     
  Lines       22765    22840      +75     
==========================================
+ Hits        13601    13670      +69     
- Misses       7843     7844       +1     
- Partials     1321     1326       +5

Flag	Coverage Δ
experimental-codecov	`59.70% <64.07%> (+0.05%)`	⬆️
stable-codecov	`53.41% <64.07%> (+0.11%)`	⬆️

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

aleem1314 · 2022-04-13T13:00:47Z

--- FAIL: TestSimAppExportAndBlockedAddrs (0.22s)
panic: parameter uploadAccess not registered [recovered]
	panic: parameter uploadAccess not registered

scream not sure what this is??

Looks like something to do with cosmwasm. Might be related to.

proto/regen/ecocredit/v1/types.proto

robert-zaremba

I suggest to better separate integration tests from unit tests, and to the latter at the unit level.

x/ecocredit/server/abci_test.go

x/ecocredit/expected_keepers.go

x/ecocredit/server/abci_test.go

x/ecocredit/core/msg_credit_type.go

x/ecocredit/server/core/new_credit_type.go

x/ecocredit/server/expected_keepers.go

x/ecocredit/server/core/proposal_handler.go

x/ecocredit/core/msg_credit_type_proposal.go

ryanchristo · 2022-04-18T18:10:22Z

x/ecocredit/core/msg_credit_type.go

+	if len(m.Unit) == 0 {
+		return sdkerrors.ErrInvalidRequest.Wrap("unit cannot be empty")
+	}


I think #429 would be nice to have alongside these updates but this could be added as a followup.

Co-authored-by: Ryan Christoffersen <[email protected]>

…regen-network/regen-ledger into ty/1005-credit_type_gov_handler

app/stable_appconfig.go

proto/regen/ecocredit/v1/types.proto

x/ecocredit/client/core/proposal.go

aaronc · 2022-04-19T19:44:20Z

x/ecocredit/client/core/proposal.go

+
+func TxCreditTypeProposalCmd() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "credit-type-proposal [proposal-title] [proposal-description] [credit_type_abbreviation] [credit_type_name] [units] [precision] [flags]",


This seems like a lot of positional arguments. I think we shouldn't even implement this and just let people submit a JSON proposal. I don't expect this to be called more than 5-10 times ever in production

is that how most folks submit proposals? i could change it to a single argument (json file)

I think aaron is referring to the gov command:

regen tx gov submit-proposal --proposal="path/to/proposal.json" --from mykey

Where proposal.json includes:

{ "title": "Add Biochar Credit Type", "description": "Add Biochar credit type to the list of allowed credit types", "type": "CreditTypeProposal", "credit_type": { "abbreviation": "BC", "name": "biochar", "unit": "tonnes of organic carbon", "precision": 6, }, "deposit": "10regen" }

I'm not sure if the above works and it should be tested before removing the command implemented here. If not, I think a json or yaml file might be nice.

ah ok didn't know of that one. removed the cli command - fa42e6d

Does it work?

didnt see your last comment 🥲, wasn't able to get this working. maybe we can take a look tomorrows standup? can just revert the removal if we cant get it going

Sounds good. Let's try tomorrow and then revert if we can't get it going.

x/ecocredit/core/msg_credit_type.go

Co-authored-by: Aaron Craelius <[email protected]>

…regen-network/regen-ledger into ty/1005-credit_type_gov_handler

x/ecocredit/server/core/add_credit_type.go

This reverts commit fa42e6d.

ryanchristo · 2022-04-20T23:15:27Z

~~Can we add a unit test for the command as it looks like we might migrate away from integration tests? #1041~~

Moving forward, we will treat cli tests as a last priority and as a nice to have in the backlog.

technicallyty added 8 commits April 11, 2022 14:46

chore: gen api

ddfe66c

wip: proposal handler

6393445

wip: wires

2dd5ecc

feat: final gov handler

0c0ddbd

chore: cleanup dead code

b04bb2e

chore: cleanup code

195b51e

Merge branch 'master' into ty/1005-credit_type_gov_handler

bd10a55

chore: fix app error

6d2f020

technicallyty mentioned this pull request Apr 13, 2022

Remove credit types from Params #1016

Closed

4 tasks

technicallyty added 3 commits April 12, 2022 19:17

chore: add dummy REST proposal handler

c2e72a9

chore: regen mocks

8690a08

chore: not invalid request

f99cd38

technicallyty commented Apr 13, 2022

View reviewed changes

app/app.go Outdated Show resolved Hide resolved

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

x/ecocredit/expected_keepers.go Show resolved Hide resolved

x/ecocredit/module/module.go Show resolved Hide resolved

technicallyty changed the title ~~feat: introduce gov handler for credit types~~ feat: credit type gov handler Apr 13, 2022

technicallyty marked this pull request as ready for review April 13, 2022 03:58

technicallyty requested review from ryanchristo, aleem1314, robert-zaremba, aaronc and amaury1093 April 13, 2022 03:58

aaronc reviewed Apr 13, 2022

View reviewed changes

proto/regen/ecocredit/v1/types.proto Show resolved Hide resolved

robert-zaremba reviewed Apr 13, 2022

View reviewed changes

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

x/ecocredit/expected_keepers.go Show resolved Hide resolved

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

technicallyty added 5 commits April 13, 2022 08:50

chore: clean test

0fcb39a

refactor: move proposal handler code into server/core

9cb5970

chore: add tests for new types

e7ab09c

refactor: use setCustomModuleBasics

e10b59b

fix: restore setCustomKeepers

834d836

ryanchristo changed the title ~~feat: credit type gov handler~~ feat(x/ecocredit): credit type gov handler Apr 17, 2022

ryanchristo reviewed Apr 18, 2022

View reviewed changes

technicallyty and others added 4 commits April 18, 2022 17:33

Update x/ecocredit/server/expected_keepers.go

8f40361

Co-authored-by: Ryan Christoffersen <[email protected]>

chore: assert credit type name length

af4f04f

refactor: NewCreditType -> AddCreditType

975f386

Merge branch 'ty/1005-credit_type_gov_handler' of https://github.com/…

7cafa8d

…regen-network/regen-ledger into ty/1005-credit_type_gov_handler

technicallyty requested a review from ryanchristo April 19, 2022 00:39

aaronc approved these changes Apr 19, 2022

View reviewed changes

technicallyty and others added 6 commits April 19, 2022 14:11

Update app/stable_appconfig.go

567325b

Co-authored-by: Aaron Craelius <[email protected]>

chore: import alias

549d53e

Merge branch 'ty/1005-credit_type_gov_handler' of https://github.com/…

d6bf5be

…regen-network/regen-ledger into ty/1005-credit_type_gov_handler

Merge branch 'master' into ty/1005-credit_type_gov_handler

8dfbe96

fix: remove duplicate credit type validate method

eb1b2f5

chore: remove proposal cli method

fa42e6d

aleem1314 reviewed Apr 20, 2022

View reviewed changes

x/ecocredit/server/core/add_credit_type.go Outdated Show resolved Hide resolved

technicallyty added 2 commits April 20, 2022 09:07

Revert "chore: remove proposal cli method"

dddd634

This reverts commit fa42e6d.

chore: revert cli removal

582fec6

ryanchristo approved these changes Apr 21, 2022

View reviewed changes

technicallyty added 3 commits April 21, 2022 14:59

Merge branch 'master' into ty/1005-credit_type_gov_handler

3f57cab

chore: update error type

eaaf563

Merge branch 'master' into ty/1005-credit_type_gov_handler

00001e9

technicallyty merged commit 0ad686d into master Apr 21, 2022

technicallyty deleted the ty/1005-credit_type_gov_handler branch April 21, 2022 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x/ecocredit): credit type gov handler #1015

feat(x/ecocredit): credit type gov handler #1015

technicallyty commented Apr 13, 2022 •

edited by zube bot

Loading

technicallyty commented Apr 13, 2022 •

edited

Loading

codecov bot commented Apr 13, 2022 •

edited

Loading

aleem1314 commented Apr 13, 2022 •

edited

Loading

robert-zaremba left a comment

ryanchristo Apr 18, 2022

aaronc Apr 19, 2022

technicallyty Apr 19, 2022

ryanchristo Apr 19, 2022 •

edited

Loading

ryanchristo Apr 19, 2022

technicallyty Apr 19, 2022

ryanchristo Apr 19, 2022

technicallyty Apr 20, 2022

ryanchristo Apr 20, 2022

ryanchristo commented Apr 20, 2022 •

edited

Loading

feat(x/ecocredit): credit type gov handler #1015

feat(x/ecocredit): credit type gov handler #1015

Conversation

technicallyty commented Apr 13, 2022 • edited by zube bot Loading

Description

Author Checklist

Reviewers Checklist

technicallyty commented Apr 13, 2022 • edited Loading

codecov bot commented Apr 13, 2022 • edited Loading

Codecov Report

aleem1314 commented Apr 13, 2022 • edited Loading

robert-zaremba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo commented Apr 20, 2022 • edited Loading

technicallyty commented Apr 13, 2022 •

edited by zube bot

Loading

technicallyty commented Apr 13, 2022 •

edited

Loading

codecov bot commented Apr 13, 2022 •

edited

Loading

aleem1314 commented Apr 13, 2022 •

edited

Loading

ryanchristo Apr 19, 2022 •

edited

Loading

ryanchristo commented Apr 20, 2022 •

edited

Loading