Skip to content

R4R: x/auth and x/bank review results#3554

Merged
cwgoes merged 4 commits intodevelopfrom
auth-bank-review
Feb 8, 2019
Merged

R4R: x/auth and x/bank review results#3554
cwgoes merged 4 commits intodevelopfrom
auth-bank-review

Conversation

@jackzampolin
Copy link
Copy Markdown
Contributor

This is the results of a code review of the x/auth and x/bank modules by @sunnya97 and @jackzampolin

@cwgoes
Copy link
Copy Markdown
Contributor

cwgoes commented Feb 8, 2019

Fails test_cover.

Copy link
Copy Markdown
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.

One possible typo - that may be why CI is failing - otherwise LGTM.

func InitGenesis(ctx sdk.Context, ak AccountKeeper, fck FeeCollectionKeeper, data GenesisState) {
ak.SetParams(ctx, data.Params)
fck.setCollectedFees(ctx, data.CollectedFees)
fck.setCollectedFees(ctx, sdk.Coins{})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't look intentional? We definitely want to set from genesis.

@cwgoes cwgoes added this to the v0.31.0 (Launch RC) milestone Feb 8, 2019
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 8, 2019

Codecov Report

Merging #3554 into develop will decrease coverage by 0.1%.
The diff coverage is 48.57%.

@@             Coverage Diff             @@
##           develop    #3554      +/-   ##
===========================================
- Coverage    57.24%   57.14%   -0.11%     
===========================================
  Files          179      179              
  Lines        14105    14135      +30     
===========================================
+ Hits          8075     8077       +2     
- Misses        5545     5578      +33     
+ Partials       485      480       -5

@cwgoes cwgoes merged commit 2c9a5bc into develop Feb 8, 2019
@cwgoes cwgoes deleted the auth-bank-review branch February 8, 2019 21:35
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