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

fix DecCoins constructor #5410

Merged
merged 5 commits into from
Dec 16, 2019
Merged

fix DecCoins constructor #5410

merged 5 commits into from
Dec 16, 2019

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented Dec 16, 2019

Closes: #5408

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

for i, coin := range coins {
dcs[i] = NewDecCoinFromCoin(coin)
decCoins := make(DecCoins, len(coins))
newCoins := NewCoins(coins...)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is where coins are sorted

@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

Merging #5410 into master will increase coverage by 0.24%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5410      +/-   ##
=========================================
+ Coverage   54.55%   54.8%   +0.24%     
=========================================
  Files         311     311              
  Lines       18756   18761       +5     
=========================================
+ Hits        10233   10282      +49     
+ Misses       7744    7701      -43     
+ Partials      779     778       -1
Impacted Files Coverage Δ
types/dec_coin.go 78.59% <100%> (+15.38%) ⬆️
x/mock/app.go 64.18% <0%> (+1.35%) ⬆️

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 -- I think it needs an additional test case for invalid coins.

types/dec_coin_test.go Show resolved Hide resolved
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

ACKed

@fedekunze fedekunze changed the title fix DecCoins fix DecCoins constructor Dec 16, 2019
@fedekunze fedekunze merged commit fb0a2c4 into master Dec 16, 2019
@fedekunze fedekunze deleted the fedekunze/5408-decCoin branch December 16, 2019 22:10
alexanderbez pushed a commit that referenced this pull request Dec 17, 2019
* fix decCoins

* minor changes for standardization

* changelog

* add test cases
@alexanderbez alexanderbez mentioned this pull request Dec 17, 2019
11 tasks
xiangjianmeng pushed a commit to xiangjianmeng/cosmos-sdk that referenced this pull request Dec 18, 2019
* fix decCoins

* minor changes for standardization

* changelog

* add test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected "negative coin amount" in DecCoin's Sub, Suggest sort before sub.
3 participants