feat(x/ecocredit)!: migrate core params to ORM #1354

aleem1314 · 2022-08-16T13:56:35Z

Description

Closes: #729

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/tx.proto

x/ecocredit/server/core/features/msg_allowed_class_creator.feature

x/ecocredit/server/core/features/msg_remove_class_creator.feature

x/ecocredit/server/core/msg_create_class.go

…into aleem/core-params-migration

…n-network/regen-ledger into aleem/core-params-migration

ryanchristo

Nice work! This is looking good. The main change I think we should make is adding and removing a single credit class creator rather than a list of credit class creators. Updating this list will not be frequent and each address added or removed warrants its own proposal. It is technically possible to include multiple updates with multiple messages but we don't want to encourage this behavior but rather one address per proposal.

proto/regen/ecocredit/v1/tx.proto

ryanchristo · 2022-08-22T16:26:13Z

proto/regen/ecocredit/v1/tx.proto

+  // creation fee.
+  repeated cosmos.base.v1beta1.Coin fees = 2 [
+    (gogoproto.nullable) = false,
+    (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"
+  ];


Ok for now but tagging #1359 for reference.

x/ecocredit/core/features/msg_add_class_creators.feature

x/ecocredit/server/core/features/msg_toggle_class_allowlist.feature

x/ecocredit/server/core/msg_create_class_test.go

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

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

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

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

ryanchristo

This is looking good. A lot to changes here. It would be nice to tackle updates like this in multiple pull requests in the future, maybe one for each new feature (in this case each param). I've added some more questions and suggestions.

CHANGELOG.md

ryanchristo · 2022-08-23T16:13:04Z

proto/regen/ecocredit/v1/tx.proto

+  // ToggleCreditClassAllowlist is a governance method that toggles the network
+  // allowlist to on or off. when on, the class creator allowlist is used to
+  // enforce which addresses may create classes. when off, any address may
+  // create classes. Since Revision 1
+  rpc ToggleCreditClassAllowlist(MsgToggleCreditClassAllowlist)
+      returns (MsgToggleCreditClassAllowlistResponse);


This should probably be ToggleClassCreatorAllowlist but we may also want to update based on #1387 so this is ok for now to keep things moving and I've added a comment in the issue #1387 (comment).

x/ecocredit/server/core/features/msg_remove_class_creator.feature

x/ecocredit/server/core/msg_create_class_test.go

x/ecocredit/server/core/msg_add_class_creator_test.go

x/ecocredit/server/core/msg_remove_class_creators_test.go

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

…n-network/regen-ledger into aleem/core-params-migration

ryanchristo

tACK. Very nice work!

ryanchristo · 2022-08-24T15:20:55Z

x/ecocredit/server/core/query_params.go

+	return &core.QueryParamsResponse{
+		Params: &core.Params{
+			AllowedClassCreators: creators,
+			AllowlistEnabled:     allowlistEnabled.Enabled,
+			CreditClassFee:       classFees1,
+			BasketFee:            basketFees1,
+		},


This is great. We should add individual queries for these params but also keep this for the time being. We may also want to consider adding the other params here so that this is not misleading, in which case we would need to update the param struct to include the other params but also deprecate it. I'll open a separate issue for tracking.

technicallyty

a few nits, preapproving

types/coin.go

x/ecocredit/server/core/features/msg_add_class_creator.feature

x/ecocredit/server/core/msg_create_class.go

x/ecocredit/server/core/msg_remove_class_creators.go

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

…n-network/regen-ledger into aleem/core-params-migration

aleem1314 added 5 commits August 16, 2022 19:25

feat: add proto msgs

5701c54

make proto-all

16513ea

feat: implement msgs

6fa8f07

wip

770b1e2

feat: add msg-server implementation

55d20d8

ryanchristo mentioned this pull request Aug 17, 2022

Granular updates to credit class fee and basket fee params #1359

Closed

4 tasks

aleem1314 added 2 commits August 18, 2022 15:50

wip: add tests

a2bd5fc

feat: add integration tests

3bdc505

aleem1314 marked this pull request as ready for review August 18, 2022 14:57

aleem1314 requested a review from a team August 18, 2022 14:57

Merge branch 'master' into aleem/core-params-migration

7a19b4a

technicallyty reviewed Aug 18, 2022

View reviewed changes

aleem1314 added 7 commits August 21, 2022 18:34

Merge branch 'main' of https://github.com/regen-network/regen-ledger …

f95b403

…into aleem/core-params-migration

chore: review changes

64b7aa3

Merge branch 'aleem/core-params-migration' of https://github.com/rege…

70e81cf

…n-network/regen-ledger into aleem/core-params-migration

fix: fix simularion tests

1317952

refactor: fix failing tests

8c30f45

chore: add changelog

4137f49

chore: add query params

e85f0c2

ryanchristo reviewed Aug 22, 2022

View reviewed changes

ryanchristo mentioned this pull request Aug 22, 2022

feat(x/ecocredit): introduce core params API #960

Closed

19 tasks

aleem1314 and others added 8 commits August 23, 2022 10:52

Update x/ecocredit/core/features/msg_add_class_creators.feature

2535112

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

Update x/ecocredit/server/core/features/msg_toggle_class_allowlist.fe…

2b128eb

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

Update x/ecocredit/server/core/features/msg_toggle_class_allowlist.fe…

7771dd1

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

Update x/ecocredit/core/features/msg_remove_class_creators.feature

cbe42db

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

Update x/ecocredit/core/features/msg_update_class_fees.feature

1c379d5

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

Update x/ecocredit/server/core/features/msg_toggle_class_allowlist.fe…

69b26f5

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

Merge branch 'main' into aleem/core-params-migration

f30b3f4

refactor: address review changes

165e47b

ryanchristo reviewed Aug 23, 2022

View reviewed changes

aleem1314 and others added 4 commits August 23, 2022 23:01

Update CHANGELOG.md

6b3ef4a

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

refactor: review changes

30ee073

Merge branch 'aleem/core-params-migration' of https://github.com/rege…

4ab023e

…n-network/regen-ledger into aleem/core-params-migration

Merge branch 'main' into aleem/core-params-migration

5e7276b

ryanchristo approved these changes Aug 24, 2022

View reviewed changes

ryanchristo requested a review from technicallyty August 24, 2022 16:09

technicallyty approved these changes Aug 24, 2022

View reviewed changes

aleem1314 and others added 4 commits August 25, 2022 16:32

Update x/ecocredit/server/core/features/msg_add_class_creator.feature

e469383

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

chore: review changes

ecaefcb

Merge branch 'aleem/core-params-migration' of https://github.com/rege…

4c13d3f

…n-network/regen-ledger into aleem/core-params-migration

Merge branch 'main' into aleem/core-params-migration

ee68612

ryanchristo merged commit 4951cc3 into main Aug 25, 2022

ryanchristo deleted the aleem/core-params-migration branch August 25, 2022 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x/ecocredit)!: migrate core params to ORM #1354

feat(x/ecocredit)!: migrate core params to ORM #1354

aleem1314 commented Aug 16, 2022 •

edited by aaronc

Loading

ryanchristo left a comment

ryanchristo Aug 22, 2022

ryanchristo left a comment •

edited

Loading

ryanchristo Aug 23, 2022

ryanchristo left a comment

ryanchristo Aug 24, 2022 •

edited

Loading

ryanchristo Aug 24, 2022

technicallyty left a comment

feat(x/ecocredit)!: migrate core params to ORM #1354

feat(x/ecocredit)!: migrate core params to ORM #1354

Conversation

aleem1314 commented Aug 16, 2022 • edited by aaronc Loading

Description

Author Checklist

Reviewers Checklist

ryanchristo left a comment

Choose a reason for hiding this comment

ryanchristo Aug 22, 2022

Choose a reason for hiding this comment

ryanchristo left a comment • edited Loading

Choose a reason for hiding this comment

ryanchristo Aug 23, 2022

Choose a reason for hiding this comment

ryanchristo left a comment

Choose a reason for hiding this comment

ryanchristo Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

ryanchristo Aug 24, 2022

Choose a reason for hiding this comment

technicallyty left a comment

Choose a reason for hiding this comment

aleem1314 commented Aug 16, 2022 •

edited by aaronc

Loading

ryanchristo left a comment •

edited

Loading

ryanchristo Aug 24, 2022 •

edited

Loading