Skip to content
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 ecocredit server to submodules #995

Merged
merged 27 commits into from
Apr 26, 2022

Conversation

technicallyty
Copy link
Contributor

@technicallyty technicallyty commented Apr 7, 2022

Description

Closes: #728

  • begins removal of some legacy code (mainly deleting msg_server, query_server)
  • removes registration of old v1alpha types
  • cleanups submodule Keeper instantiation
  • removes redundant integration tests (everything in testsuite/tx.go and testsuite/query.go was already covered by keeper tests).
  • cleans up scenario tests in testsuite/suite.go.
  • comments out genesis tests (will be updated by update ecocredit genesis to use v1 types #976)
  • changes module registration to use v1 params

there is more to remove/cleanup, but figured i'd stop here to keep the diff manageable. Main concern is with integration tests. Might be helpful to discuss a better approach to these tests in an extended call?


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

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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)

@technicallyty technicallyty mentioned this pull request Apr 7, 2022
8 tasks
@technicallyty
Copy link
Contributor Author

technicallyty commented Apr 7, 2022

looks like theres still a CI failures, but its related to x/group requiring ecocredit.MsgServer 🙄, not sure if worth fixing?

@technicallyty technicallyty marked this pull request as ready for review April 7, 2022 22:20
@technicallyty technicallyty added the Scope: x/ecocredit Issues that update the x/ecocredit module label Apr 7, 2022
@technicallyty technicallyty added this to the v4.0 - Llangorse Upgrade milestone Apr 7, 2022
Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Given the amount of code commented out in relation to the genesis updates, I think we should focus on getting #977 reviewed and merged first.

app/testsuite/modules_test.go Outdated Show resolved Hide resolved
x/ecocredit/server/genesis.go Outdated Show resolved Hide resolved
x/ecocredit/server/genesis.go Outdated Show resolved Hide resolved
@ryanchristo ryanchristo changed the title chore: update integration tests/cleanup legacy code refactor(x/ecocredit): update integration tests / clean up legacy code Apr 8, 2022
@ryanchristo
Copy link
Member

I think we're good to go on updating and including these changes now when you get a chance.

@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #995 (9d648bf) into master (002a876) will increase coverage by 9.43%.
The diff coverage is 98.88%.

@@            Coverage Diff             @@
##           master     #995      +/-   ##
==========================================
+ Coverage   59.61%   69.05%   +9.43%     
==========================================
  Files         230      223       -7     
  Lines       23289    21283    -2006     
==========================================
+ Hits        13884    14697     +813     
+ Misses       8063     5272    -2791     
+ Partials     1342     1314      -28     
Flag Coverage Δ
experimental-codecov 68.95% <98.88%> (+9.43%) ⬆️
stable-codecov 62.97% <98.88%> (+9.70%) ⬆️

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

@ryanchristo
Copy link
Member

@technicallyty is this ready for another review? Can you fix the simulation tests?

Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Just a couple comments.

app/testsuite/modules_test.go Outdated Show resolved Hide resolved
x/group/server/server.go Outdated Show resolved Hide resolved
Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Maybe we should update the title of the pull request to something like:

refactor(x/ecocredit): migrate ecocredit server to submodules

The pull request does more than "update integration tests". Also "clean up legacy code" is implied by the migration.

Comment on lines 32 to 35
baskettypes "github.com/regen-network/regen-ledger/x/ecocredit/basket"
"github.com/regen-network/regen-ledger/x/ecocredit/client"
"github.com/regen-network/regen-ledger/x/ecocredit/core"
coretypes "github.com/regen-network/regen-ledger/x/ecocredit/core"
marketplacetypes "github.com/regen-network/regen-ledger/x/ecocredit/marketplace"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use basket, core, and marketplace rather than adding types to each but ok to leave as is.

@technicallyty technicallyty changed the title refactor(x/ecocredit): update integration tests / clean up legacy code refactor(x/ecocredit): migrate ecocredit server to submodules Apr 25, 2022
Copy link
Contributor

@aleem1314 aleem1314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ryanchristo ryanchristo merged commit 5de0d92 into master Apr 26, 2022
@ryanchristo ryanchristo deleted the ty/728-integration_tests branch April 26, 2022 14:09
@ryanchristo ryanchristo removed this from the v4.0 - Llangorse Upgrade milestone Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: x/ecocredit Issues that update the x/ecocredit module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ecocredit refactor follow-ups
3 participants