TruncateDecimal() output is not correct

[dauTT]

Summary of Bug

The output of TruncateDecimal is not correct because the first output (truncatedCoins) is a slice of coins whose ordering very much dependent from the coins name. This creates a dependency with the name of the coins which is not good.

Version

master branch: latest commit

Steps to Reproduce

for more details we can examine this test:

func TestTruncate(t *testing.T) {
	valCommission1 := sdk.DecCoins{
		sdk.NewDecCoinFromDec("tokenxxx", sdk.NewDec(5).Quo(sdk.NewDec(2))),
		sdk.NewDecCoinFromDec("stake", sdk.NewDec(1).Quo(sdk.NewDec(1))),
	}

	commission1, remainder1 := valCommission1.TruncateDecimal()

	fmt.Println("commission1: ", commission1)
	fmt.Println("remainder1: ", remainder1)
	// output:
	// commission1:  1stake,2tokenxxx
        // remainder1:  0.500000000000000000tokenxxx

	valCommission2 := sdk.DecCoins{
		sdk.NewDecCoinFromDec("atoken", sdk.NewDec(5).Quo(sdk.NewDec(2))),
		sdk.NewDecCoinFromDec("stake", sdk.NewDec(1).Quo(sdk.NewDec(1))),
	}

	commission2, remainder2 := valCommission2.TruncateDecimal()

	fmt.Println("commission2: ", commission2)
	fmt.Println("remainder2: ", remainder2)
	// output:
	// commission2:  2atoken,1stake
	// remainder2:  0.500000000000000000atoken
}

in the above case we believe the ordering of commission1 is wrong and the output should look something like this:

fmt.Println("commission1: ", commission1)
// output:
// commission1:  2tokenxxx,1stake

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

I'm not sure what you mean by dependency? Coins are always returned in sorted order, regardless of the API.

[dauTT]

There is a test case in #6764, which is failing only because I change the name of the token.

func TestSimulateMsgWithdrawValidatorCommission(t *testing.T) {
	testSimulateMsgWithdrawValidatorCommission(t, "atoken")
	// TODO: Fix following test: It is failing because of issue #6763
	// testSimulateMsgWithdrawValidatorCommission(t, "tokenxxx")
}

[alexanderbez]

Can you elaborate on how it's failing? Also, I've commented on that PR, you have to use NewDecCoins, please don't use Coins/DecCoins literals, use their constructors. We created them for this reason.

[dauTT]

At some point we are subtracting the commission to the outstanding. The operation Sub seems to be dependent from the ordering of the coins slice ([]DecCoin). It the ordering of the commission changes the operation failed. But TruncateDecimal does not seems to preserve the ordering of commission correctly. When we run testSimulateMsgWithdrawValidatorCommission(t, "atoken") the ordering of commision is correct and the test pass. But when we run testSimulateMsgWithdrawValidatorCommission(t, "tokenxxx") the ordering of commission is inverted and the test panic.

[dauTT]

@alexanderbez ,

using NewDecCoins will fix this issue. 😄

[alexanderbez]

Awesome! Can we close this issue?

[dauTT]

Yes, please.