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

WIP: Refactor crisis using internal package #4597

Closed
wants to merge 2 commits into from

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Jun 20, 2019

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]
  • 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)

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

the tags pkg should be deleted and tags.go moved to types as well as the expected_keepers.go. The keeper pkg should be in internal as well.

genesis.go needs to be split and one should remain in internal/types whereas the other should be in the root of the module

feeCollectionKeeper FeeCollectionKeeper
distrKeeper distrKeeper
bankKeeper bankKeeper
feeCollectionKeeper feeCollectionKeeper
}

// NewKeeper creates a new Keeper object
func NewKeeper(paramSpace params.Subspace, invCheckPeriod uint,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we want the keeper to also be internal? Currently, this is inconsistent with the changes made on bank.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move it across internal

@codecov
Copy link

codecov bot commented Jun 20, 2019

Codecov Report

Merging #4597 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4597      +/-   ##
==========================================
+ Coverage   52.75%   52.77%   +0.02%     
==========================================
  Files         264      264              
  Lines       16464    16464              
==========================================
+ Hits         8685     8689       +4     
+ Misses       7130     7126       -4     
  Partials      649      649

@alessio
Copy link
Contributor Author

alessio commented Jun 20, 2019

@fedekunze and I had a conversation about this, and we agreed this be on hold until supply PR is merged in. Hence blocking this for now.

@alessio alessio added the S:blocked Status: Blocked label Jun 20, 2019
@alessio alessio changed the title Refactor crisis using internal package WIP: Refactor crisis using internal package Jun 20, 2019
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.

ACK pending supply module merge.

@fedekunze fedekunze removed the S:blocked Status: Blocked label Jun 28, 2019
@fedekunze
Copy link
Collaborator

removed blocked label

@alexanderbez
Copy link
Contributor

@alessio feel free to pick back up on this again 👍

@alessio
Copy link
Contributor Author

alessio commented Jul 1, 2019

Superseded by #4649

@alessio alessio closed this Jul 1, 2019
@alessio alessio deleted the alessio/crisis-internal branch July 1, 2019 03:43
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.

3 participants