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

R4R: module inter-dependency cleanup #3621

Merged
merged 8 commits into from
Feb 13, 2019
Merged

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Feb 12, 2019

This is preperation PR for #2935 which effectively moves invariance checks from simulation testing to core functionality. As a part of this I wanted to remove inter-module dependencies (sometimes we've been more lax when it comes to testing). During this process I also noticed that outside of testing there were intermodule dependencies, so I took the liberty of clearing these up - as a part of this introducing some new expected keepers within modules which lacked that design.

Notes on where I attempted to remove inter-dependency:

  • ignoring use within _test files
  • ignoring use of params within other modules (requires params interface - which we should probably create)
  • ignoring use of auth within other modules (require auth interfaces - which we should probably create)
  • ignoring use of mock within simulation directories besides invariance (in prep for Broken-Invariant-Evidence Tx #2935)

There is still more cleanup due (see above points!) - but this PR is a good first step in de-spagetting the modules

REF: #3556

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • [na] Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Feb 12, 2019

Codecov Report

Merging #3621 into develop will decrease coverage by 0.08%.
The diff coverage is 53.08%.

@@             Coverage Diff             @@
##           develop    #3621      +/-   ##
===========================================
- Coverage    60.99%   60.91%   -0.09%     
===========================================
  Files          187      188       +1     
  Lines        14069    14084      +15     
===========================================
- Hits          8582     8579       -3     
- Misses        4943     4961      +18     
  Partials       544      544

@rigelrozanski rigelrozanski changed the title WIP: module inter-dependency cleanup R4R: module inter-dependency cleanup Feb 12, 2019
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

These changes seem reasonable to me, but I wonder if we should start using subpackages in types/ for generic types or functions associated with modules - e.g. for the staking code for default denomination & power conversions - the types/ package is pretty wide-ranging at the moment.

@rigelrozanski
Copy link
Contributor Author

rigelrozanski commented Feb 12, 2019

@cwgoes I'd gladly refactor the types package to include subpackages as a part of this PR


Edit: maybe actually we should wait to perform as a part of this major restructure: #1123 (which should be done in a separate PR - but probably done soon)

types/stake.go Outdated Show resolved Hide resolved
types/stake.go Outdated Show resolved Hide resolved
x/distribution/keeper/for_expected_keepers.go Outdated Show resolved Hide resolved
x/ibc/keepers.go Outdated Show resolved Hide resolved
x/staking/keeper/sdk_types.go Show resolved Hide resolved
@jackzampolin
Copy link
Member

@rigelrozanski maybe in a followup PR? This is getting pretty big already.


// return all delegations used during genesis dump
// TODO: remove this func, change all usage for iterate functionality
func (k Keeper) GetAllSDKDelegations(ctx sdk.Context) (delegations []sdk.Delegation) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have SDK in this function name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions for alternative names? I agree this is less than ideal - however I've marked this function for removal (while addressing #3386) so I'm not too too concerned about getting it perfect. Basically, this function is the same as GetAllDelegations except returning in sdk.Delegation instead os staking.Delegations

x/ibc/keepers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM except for the GetSDK* function names, not sure what it should be called, but I don't think we should have SDK in the method/function sigs.

Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jackzampolin
Copy link
Member

Looks like there is a conflict here @rigelrozanski. Also read through this in detail and it didn't appear to have anything breaking. Can you confirm thats the case?

@rigelrozanski
Copy link
Contributor Author

@jackzampolin the only thing which is api breaking is the rename from Bonds to Delegations for genesis staking staking.GenesisState.Bonds -> Delegations

There are no state-machine breaking changes

@jackzampolin
Copy link
Member

Awesome! Thank you for the rebase @rigelrozanski

@jackzampolin jackzampolin merged commit d66db6a into develop Feb 13, 2019
@jackzampolin jackzampolin deleted the rigel/module-dep-cleanup branch February 13, 2019 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants