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

cmd/quicksilverd: AddGenesisAccountCmd has a redundant condition checking if a balance is zero and if the vesting amount is greater but it really should check that the coalesced vesting amounts are not greater than the coalesced balances in case there are duplicate denomination entries in the coins #188

Closed
odeke-em opened this issue Aug 25, 2022 · 1 comment

Comments

@odeke-em
Copy link
Contributor

odeke-em commented Aug 25, 2022

If we look at this code https://github.com/ingenuity-build/quicksilver/blob/4434d8801040205d86a68a3ca681c69f94ba5815/cmd/quicksilverd/genaccounts.go#L101-L102 really if someone accidentally entered a duplicate denomination entry those checks could fail for example with {500qck, 400qck} for vestingAmount then {600qck} for balance, that check doesn't complain yet technically the total balance is less than the collective vesting amount. Unfortunately the cosmos-sdk's code doesn't support coalescing but we should perhaps apply this coalescing

// coalesceAndSort takes in coins and coalesces them by denomination
// and adds those values, then later sorts them and returns the result.
func coalesceAndSort(cL sdk.Coins) (filtered sdk.Coins) {
	uniqCoins := make(map[string]sdk.Coins)
	anyDups := false
	for _, c := range cL {
		uniqCoins[c.Denom] = append(uniqCoins[c.Denom], c)
		if len(uniqCoins[c.Denom]) > 1 {
			anyDups = true
		}
	}
	if !anyDups {
		return cL
	}

	for denom, cL := range uniqCoins {
		unitCoin := sdk.Coin{Denom: denom, Amount: sdk.NewInt(0)}
		for _, c := range cL {
			unitCoin = unitCoin.Add(c)
		}
		filtered = append(filtered, unitCoin)
	}
	return filtered.Sort()
}

which will catch those checks after being applied to each sdk.Coins values.

/cc @elias-orijtech

@odeke-em
Copy link
Contributor Author

I fixed this in cosmos-sdk per cosmos/cosmos-sdk#13234 with PR cosmos/cosmos-sdk#13265

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

No branches or pull requests

1 participant