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

Add New constructor for the DecCoin #5449

Merged
merged 17 commits into from
Jan 3, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions types/coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,11 @@ func (coins Coins) AmountOf(denom string) Int {
}
}

// GetDenomByIndex returns the Denom of the certain coin to make the findDup generic
func (coins Coins) GetDenomByIndex(i int) string {
return coins[i].Denom
}

// IsAllPositive returns true if there is at least one coin and all currencies
// have a positive value.
func (coins Coins) IsAllPositive() bool {
Expand Down Expand Up @@ -669,18 +674,23 @@ func ParseCoins(coinsStr string) (Coins, error) {
return coins, nil
}

type findDupDescriptor interface {
GetDenomByIndex(int) string
Len() int
}

// findDup works on the assumption that coins is sorted
func findDup(coins Coins) int {
if len(coins) <= 1 {
func findDup(coins findDupDescriptor) int {
if coins.Len() <= 1 {
return -1
}

prevDenom := coins[0].Denom
for i := 1; i < len(coins); i++ {
if coins[i].Denom == prevDenom {
prevDenom := coins.GetDenomByIndex(0)
for i := 1; i < coins.Len(); i++ {
if coins.GetDenomByIndex(i) == prevDenom {
return i
}
prevDenom = coins[i].Denom
prevDenom = coins.GetDenomByIndex(i)
}

return -1
Expand Down
37 changes: 35 additions & 2 deletions types/dec_coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,32 @@ func (coin DecCoin) IsValid() bool {
// DecCoins defines a slice of coins with decimal values
type DecCoins []DecCoin

// NewDecCoins constructs a new coin set with decimal values
// NewDecCoins constructs a new coin set with with decimal values
// from DecCoins.
func NewDecCoins(decCoins ...DecCoin) DecCoins {
// remove zeroes
newDecCoins := removeZeroDecCoins(DecCoins(decCoins))
if len(newDecCoins) == 0 {
return DecCoins{}
}

newDecCoins.Sort()

// detect duplicate Denoms
if dupIndex := findDup(newDecCoins); dupIndex != -1 {
panic(fmt.Errorf("find duplicate denom: %s", newDecCoins[dupIndex]))
}

if !newDecCoins.IsValid() {
panic(fmt.Errorf("invalid coin set: %s", newDecCoins))
}

return newDecCoins
}

// NewDecCoinsFromCoin constructs a new coin set with decimal values
// from regular Coins.
func NewDecCoins(coins Coins) DecCoins {
func NewDecCoinsFromCoin(coins ...Coin) DecCoins {
PumpkinSeed marked this conversation as resolved.
Show resolved Hide resolved
decCoins := make(DecCoins, len(coins))
newCoins := NewCoins(coins...)
for i, coin := range newCoins {
Expand All @@ -169,6 +192,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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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'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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return append(coins, decCoin)
}

// String implements the Stringer interface for DecCoins. It returns a
// human-readable representation of decimal coins.
func (coins DecCoins) String() string {
Expand Down Expand Up @@ -311,6 +339,11 @@ func (coins DecCoins) Intersect(coinsB DecCoins) DecCoins {
return removeZeroDecCoins(res)
}

// GetDenomByIndex returns the Denom to make the findDup generic
func (coins DecCoins) GetDenomByIndex(i int) string {
return coins[i].Denom
}

// IsAnyNegative returns true if there is at least one coin whose amount
// is negative; returns false otherwise. It returns false if the DecCoins set
// is empty too.
Expand Down
76 changes: 74 additions & 2 deletions types/dec_coin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func TestSubDecCoins(t *testing.T) {
msg string
}{
{
NewDecCoins(Coins{NewCoin("mytoken", NewInt(10)), NewCoin("btc", NewInt(20)), NewCoin("eth", NewInt(30))}),
NewDecCoinsFromCoin(Coins{NewCoin("mytoken", NewInt(10)), NewCoin("btc", NewInt(20)), NewCoin("eth", NewInt(30))}...),
PumpkinSeed marked this conversation as resolved.
Show resolved Hide resolved
true,
"sorted coins should have passed",
},
Expand All @@ -188,7 +188,7 @@ func TestSubDecCoins(t *testing.T) {
},
}

decCoins := NewDecCoins(Coins{NewCoin("btc", NewInt(10)), NewCoin("eth", NewInt(15)), NewCoin("mytoken", NewInt(5))})
decCoins := NewDecCoinsFromCoin(Coins{NewCoin("btc", NewInt(10)), NewCoin("eth", NewInt(15)), NewCoin("mytoken", NewInt(5))}...)
PumpkinSeed marked this conversation as resolved.
Show resolved Hide resolved

for _, tc := range tests {
tc := tc
Expand Down Expand Up @@ -421,3 +421,75 @@ func TestDecCoinsQuoDecTruncate(t *testing.T) {
}
}
}

func TestNewDecCoinsWithIsValid(t *testing.T) {
fake1 := append(NewDecCoins(NewDecCoin("mytoken", NewInt(10))), DecCoin{Denom: "BTC", Amount: NewDec(10)})
fake2 := append(NewDecCoins(NewDecCoin("mytoken", NewInt(10))), DecCoin{Denom: "BTC", Amount: NewDec(-10)})

tests := []struct {
coin DecCoins
expectPass bool
msg string
}{
{
NewDecCoins(NewDecCoin("mytoken", NewInt(10))),
true,
"valid coins should have passed",
},
{
fake1,
false,
"invalid denoms",
},
{
fake2,
false,
"negative amount",
},
}

for _, tc := range tests {
tc := tc
if tc.expectPass {
require.True(t, tc.coin.IsValid(), tc.msg)
} else {
require.False(t, tc.coin.IsValid(), tc.msg)
}
}
}

func TestDecCoins_AddDecCoinWithIsValid(t *testing.T) {
lengthTestDecCoins := NewDecCoins().AddDecCoin(NewDecCoin("mytoken", NewInt(10))).AddDecCoin(DecCoin{Denom: "BTC", Amount: NewDec(10)})
require.Equal(t, 2, len(lengthTestDecCoins), "should be 2")

tests := []struct {
coin DecCoins
expectPass bool
msg string
}{
{
NewDecCoins().AddDecCoin(NewDecCoin("mytoken", NewInt(10))),
true,
"valid coins should have passed",
},
{
NewDecCoins().AddDecCoin(NewDecCoin("mytoken", NewInt(10))).AddDecCoin(DecCoin{Denom: "BTC", Amount: NewDec(10)}),
false,
"invalid denoms",
},
{
NewDecCoins().AddDecCoin(NewDecCoin("mytoken", NewInt(10))).AddDecCoin(DecCoin{Denom: "BTC", Amount: NewDec(-10)}),
false,
"negative amount",
},
}

for _, tc := range tests {
tc := tc
if tc.expectPass {
require.True(t, tc.coin.IsValid(), tc.msg)
} else {
require.False(t, tc.coin.IsValid(), tc.msg)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
}
}

2 changes: 1 addition & 1 deletion x/auth/types/stdtx.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func (fee StdFee) Bytes() []byte {
// originally part of the submitted transaction because the fee is computed
// as fee = ceil(gasWanted * gasPrices).
func (fee StdFee) GasPrices() sdk.DecCoins {
return sdk.NewDecCoins(fee.Amount).QuoDec(sdk.NewDec(int64(fee.Gas)))
return sdk.NewDecCoinsFromCoin(fee.Amount...).QuoDec(sdk.NewDec(int64(fee.Gas)))
}

//__________________________________________________________
Expand Down
2 changes: 1 addition & 1 deletion x/crisis/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func createTestApp() (*simapp.SimApp, sdk.Context, []sdk.AccAddress) {
app.CrisisKeeper.RegisterRoute(testModuleName, dummyRouteWhichFails.Route, dummyRouteWhichFails.Invar)

feePool := distr.InitialFeePool()
feePool.CommunityPool = sdk.NewDecCoins(sdk.NewCoins(constantFee))
feePool.CommunityPool = sdk.NewDecCoinsFromCoin(sdk.NewCoins(constantFee)...)
app.DistrKeeper.SetFeePool(ctx, feePool)
app.SupplyKeeper.SetSupply(ctx, supply.NewSupply(sdk.Coins{}))

Expand Down
2 changes: 1 addition & 1 deletion x/distribution/keeper/allocation.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func (k Keeper) AllocateTokens(
// (and distributed to the previous proposer)
feeCollector := k.supplyKeeper.GetModuleAccount(ctx, k.feeCollectorName)
feesCollectedInt := feeCollector.GetCoins()
feesCollected := sdk.NewDecCoins(feesCollectedInt)
feesCollected := sdk.NewDecCoinsFromCoin(feesCollectedInt...)

// transfer collected fees to the distribution module account
err := k.supplyKeeper.SendCoinsFromModuleToModule(ctx, k.feeCollectorName, types.ModuleName, feesCollectedInt)
Expand Down
2 changes: 1 addition & 1 deletion x/distribution/keeper/fee_pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ func (k Keeper) DistributeFromFeePool(ctx sdk.Context, amount sdk.Coins, receive
// NOTE the community pool isn't a module account, however its coins
// are held in the distribution module account. Thus the community pool
// must be reduced separately from the SendCoinsFromModuleToAccount call
newPool, negative := feePool.CommunityPool.SafeSub(sdk.NewDecCoins(amount))
newPool, negative := feePool.CommunityPool.SafeSub(sdk.NewDecCoinsFromCoin(amount...))
if negative {
return types.ErrBadDistribution(k.codespace)
}
Expand Down
4 changes: 2 additions & 2 deletions x/distribution/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func (k Keeper) WithdrawValidatorCommission(ctx sdk.Context, valAddr sdk.ValAddr

// update outstanding
outstanding := k.GetValidatorOutstandingRewards(ctx, valAddr)
k.SetValidatorOutstandingRewards(ctx, valAddr, outstanding.Sub(sdk.NewDecCoins(commission)))
k.SetValidatorOutstandingRewards(ctx, valAddr, outstanding.Sub(sdk.NewDecCoinsFromCoin(commission...)))

if !commission.IsZero() {
accAddr := sdk.AccAddress(valAddr)
Expand Down Expand Up @@ -160,7 +160,7 @@ func (k Keeper) FundCommunityPool(ctx sdk.Context, amount sdk.Coins, sender sdk.
}

feePool := k.GetFeePool(ctx)
feePool.CommunityPool = feePool.CommunityPool.Add(sdk.NewDecCoins(amount))
feePool.CommunityPool = feePool.CommunityPool.Add(sdk.NewDecCoinsFromCoin(amount...))
k.SetFeePool(ctx, feePool)

return nil
Expand Down
2 changes: 1 addition & 1 deletion x/distribution/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,6 @@ func TestFundCommunityPool(t *testing.T) {
err := keeper.FundCommunityPool(ctx, amount, delAddr1)
assert.Nil(t, err)

assert.Equal(t, initPool.CommunityPool.Add(sdk.NewDecCoins(amount)), keeper.GetFeePool(ctx).CommunityPool)
assert.Equal(t, initPool.CommunityPool.Add(sdk.NewDecCoinsFromCoin(amount...)), keeper.GetFeePool(ctx).CommunityPool)
assert.Empty(t, bk.GetCoins(ctx, delAddr1))
}
2 changes: 1 addition & 1 deletion x/distribution/proposal_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestProposalHandlerPassed(t *testing.T) {
accountKeeper.SetAccount(ctx, account)

feePool := keeper.GetFeePool(ctx)
feePool.CommunityPool = sdk.NewDecCoins(amount)
feePool.CommunityPool = sdk.NewDecCoinsFromCoin(amount...)
keeper.SetFeePool(ctx, feePool)

tp := testProposal(recipient, amount)
Expand Down