Add New constructor for the DecCoin #5449

PumpkinSeed · 2019-12-25T16:24:47Z

Closes: #5430

Description

Implements the proposal for new constructor of the DecCoins.

Rename the types.NewDecCoins to types.NewDecCoinsFromCoin
Add NewDecCoins with a new functionality creates NewDecCoins with the variadic function parameters.
Add AddDecCoin method for DecCoins.

If it fits the proposal let me know and I will fix the docs.

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)

golangcibot · 2019-12-25T16:27:34Z

types/dec_coin_test.go

+			require.False(t, tc.coin.IsValid(), tc.msg)
+		}
+	}
+}


File is not gofmt-ed with -s (from gofmt)

Suggested change

}

}

fedekunze

Thanks @PumpkinSeed! Some changes are required as we also need to perform a validation on the constructor functions

types/dec_coin.go

fedekunze · 2019-12-26T15:16:32Z

types/dec_coin.go

-// NewDecCoins constructs a new coin set with decimal values
+// NewDecCoins constructs a new coin set with with decimal values
+// from DecCoins.
+func NewDecCoins(decCoinArray ...DecCoin) DecCoins {


We need to add extra checks for the coins that are being created with the constructor:

remove empty (zero value) DecCoins

sort coins by denomination

panic on duplicated denomination

panic on invalid decCoin

You can check the NewCoins constructor for reference

I tried to make the findDup a bit more generic to use for Coins and DecCoins with the same function, but I'm not a big fan of interfaces for these solutions of reference types because of the heap allocations of them. But if you agree with that it's good to me.

fedekunze · 2019-12-26T15:23:49Z

types/dec_coin.go

@@ -169,6 +180,11 @@ func NewDecCoins(coins Coins) DecCoins {
 	return decCoins
 }

+// AddDecCoin adds a new decimal coin to the existed set of decimal coins.
+func (coins DecCoins) AddDecCoin(decCoin DecCoin) DecCoins {


This function needs to have the same checks as NewCoins. Technically it's duplicate of func (coins DecCoins) Add(coinsB DecCoins) DecCoins (L227) with the difference that this only appends a single coin without validation.

Ideally, it could be changed to Add(coinsB ...DecCoin) DecCoins to support both cases.

I'm good with the rewrite of the Add method and the remove of the AddDecCoin, but it can be a change on the library usage so it can break third-party systems who using this. I'm not familiar enough with this to make this decision.

I'm good with the rewrite of the Add method and the remove of the AddDecCoin, but it can be a change on the library usage so it can break third-party systems who using this.

It's fine as long as it's documented as an API breaking change on the CHANGELOG

types/dec_coin.go

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

types/dec_coin_test.go

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

…os-sdk into I5430-deccoins-proposal

fedekunze

ACK

alexanderbez

Thanks @PumpkinSeed! Two minor nitpicks, but otherwise LGTM 🎉

types/dec_coin.go

alexanderbez · 2020-01-02T13:48:02Z

types/dec_coin.go

@@ -208,7 +231,7 @@ func (coins DecCoins) TruncateDecimal() (truncatedCoins Coins, changeCoins DecCo
 //
 // CONTRACT: Add will never return Coins where one Coin has a non-positive
 // amount. In otherwords, IsValid will always return true.
-func (coins DecCoins) Add(coinsB DecCoins) DecCoins {
+func (coins DecCoins) Add(coinsB ...DecCoin) DecCoins {


This is different than the coins variant which takes a slice literal. I'd rather be consistent here and accept the slice literal instead of variadic args. What was the rationale for this @fedekunze?

I will wait for the response of @fedekunze and based on that I will do the necessary changes.

What was the rationale for this @fedekunze?

see #5449 (comment)

I don't see the rationale there. The API is still inconsistent. Change this method or also change Coins#Add.

If I can have my own opinion, I would change the Coins#Add because the variadic args opens up more clear ways to pass one or more data or slice of data.

@alexanderbez I changed Coins#Add, also marked in the API Breaking section. If you wanted to move to the other direction and revert the changes on the DecCoins#Add please let me know.

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

codecov · 2020-01-03T16:55:22Z

Codecov Report

Merging #5449 into master will decrease coverage by 0.02%.
The diff coverage is 68.57%.

@@            Coverage Diff             @@
##           master    #5449      +/-   ##
==========================================
- Coverage   54.41%   54.38%   -0.03%     
==========================================
  Files         313      313              
  Lines       18836    18850      +14     
==========================================
+ Hits        10249    10252       +3     
- Misses       7802     7811       +9     
- Partials      785      787       +2

Impacted Files	Coverage Δ
x/distribution/keeper/hooks.go	`30.23% <0%> (ø)`	⬆️
x/distribution/keeper/invariants.go	`0% <0%> (ø)`	⬆️
x/distribution/genesis.go	`0% <0%> (ø)`	⬆️
simapp/export.go	`7.59% <0%> (ø)`	⬆️
x/distribution/keeper/fee_pool.go	`0% <0%> (ø)`	⬆️
x/mock/app.go	`64.18% <0%> (ø)`	⬆️
x/mock/types.go	`11.76% <0%> (ø)`	⬆️
simapp/test_helpers.go	`0% <0%> (ø)`	⬆️
x/auth/types/stdtx.go	`61.53% <0%> (ø)`	⬆️
x/bank/internal/types/msgs.go	`97.22% <100%> (ø)`	⬆️
... and 15 more

alexanderbez

ACK

Add New constructor for the DecCoin

ae01084

PumpkinSeed requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners December 25, 2019 16:24

golangcibot reviewed Dec 25, 2019

View reviewed changes

fedekunze suggested changes Dec 26, 2019

View reviewed changes

fedekunze added R4R C:Types labels Dec 26, 2019

alexanderbez reviewed Dec 26, 2019

View reviewed changes

types/dec_coin.go Outdated Show resolved Hide resolved

PumpkinSeed and others added 6 commits December 27, 2019 12:09

Update types/dec_coin.go

2f3a063

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Update types/dec_coin.go

2c996aa

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

Make findDup generic and and checks for NewDecCoins

5fb0356

Fix stdtx's NewDecCoinsFromCoin call

c8944f6

Fix build errors caused by NewDecCoinsFromCoin

e2b5182

Add fix for the NewDecCoins test

2ad69b7

PumpkinSeed requested a review from fedekunze December 28, 2019 09:24

fedekunze reviewed Dec 30, 2019

View reviewed changes

types/dec_coin_test.go Outdated Show resolved Hide resolved

types/dec_coin_test.go Outdated Show resolved Hide resolved

PumpkinSeed and others added 5 commits December 31, 2019 13:44

Update types/dec_coin_test.go

cdcf3b0

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

Update types/dec_coin_test.go

4c13d4e

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

Change parameter list of DecCoins.Add

ebdac69

Merge branch 'I5430-deccoins-proposal' of github.com:PumpkinSeed/cosm…

36eb163

…os-sdk into I5430-deccoins-proposal

Merge branch 'master' into I5430-deccoins-proposal

9149a11

PumpkinSeed requested a review from fedekunze December 31, 2019 13:18

fedekunze approved these changes Jan 1, 2020

View reviewed changes

PumpkinSeed requested a review from alexanderbez January 1, 2020 13:17

alexanderbez suggested changes Jan 2, 2020

View reviewed changes

PumpkinSeed and others added 2 commits January 2, 2020 15:47

Update types/dec_coin.go

8e1d950

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Add fixes based on the suggested change

0248204

fedekunze and others added 2 commits January 2, 2020 12:34

Merge branch 'master' into I5430-deccoins-proposal

8de2070

Change Coins#Add paramter to ...Coin

b12f429

alexanderbez approved these changes Jan 3, 2020

View reviewed changes

Merge branch 'master' into I5430-deccoins-proposal

7731da7

alexanderbez merged commit 066dd11 into cosmos:master Jan 3, 2020

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

Merge PR cosmos#5449: Add New constructor for the DecCoin

018bea4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add New constructor for the DecCoin #5449

Add New constructor for the DecCoin #5449

PumpkinSeed commented Dec 25, 2019

golangcibot Dec 25, 2019

fedekunze left a comment

fedekunze Dec 26, 2019

PumpkinSeed Dec 27, 2019

fedekunze Dec 26, 2019

PumpkinSeed Dec 27, 2019

fedekunze Dec 30, 2019

fedekunze left a comment

alexanderbez left a comment

alexanderbez Jan 2, 2020

PumpkinSeed Jan 2, 2020

fedekunze Jan 2, 2020

alexanderbez Jan 3, 2020

PumpkinSeed Jan 3, 2020

PumpkinSeed Jan 3, 2020

codecov bot commented Jan 3, 2020 •

edited

Loading

alexanderbez left a comment

Add New constructor for the DecCoin #5449

Add New constructor for the DecCoin #5449

Conversation

PumpkinSeed commented Dec 25, 2019

Description

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 3, 2020 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 3, 2020 •

edited

Loading