-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(x/ecocredit): migrate abci to module and clean up module #1452
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1452 +/- ##
==========================================
- Coverage 78.48% 78.45% -0.03%
==========================================
Files 238 238
Lines 18439 18448 +9
==========================================
+ Hits 14471 14473 +2
- Misses 3126 3130 +4
- Partials 842 845 +3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT - preapproving w/ small nits
CHANGELOG.md
Outdated
@@ -65,11 +65,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
- [#1337](https://github.com/regen-network/regen-ledger/pull/1337) The `AddCreditType` proposal handler has been removed. | |||
- [#1342](https://github.com/regen-network/regen-ledger/pull/1342) The `NewKeeper` method in `ecocredit/marketplace` requires an `authority` address. | |||
- [#1342](https://github.com/regen-network/regen-ledger/pull/1342) The `AllowedDenom` proposal handler has been removed. | |||
|
|||
- [#1452](https://github.com/regen-network/regen-ledger/pull/1452) The `NewModule` method in `ecocredit/module` requires an `authority` address and store `key`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it might be better to describe what the breaking change was (position of args changed), i was confused at first thinking "i thought we already required those things" 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are new since the last release and not recorded anywhere else in the change log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not sure whether I should retroactively fix or just add a new entry and went with the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was being lazy. I updated the changelog. No entry for changing the order necessary though if these are new. Added the two pull requests for when they were added. 👍
Description
Closes: n/a
This pull request migrates the abci method to the module package per sdk building module guidelines, removes the deprecated
RegisterRESTRoutes
method, and reorganizes the module so that related methods are grouped together.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.md
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...
!
in the type prefix if API or client breaking change