types: removeZeroCoins algorithm mutates the original slice

[odeke-em]

Noticed via code audit, there is a bunch of code that mutates slices in place, but really does mutate underlying slices for example

cosmos-sdk/types/coin.go

Lines 547 to 561 in 13b5a8d

	// removeZeroCoins removes all zero coins from the given coin set in-place.
	func removeZeroCoins(coins Coins) Coins {
	i, l := 0, len(coins)
	for i < l {
	if coins[i].IsZero() {
	// remove coin
	coins = append(coins[:i], coins[i+1:]...)
	l--
	} else {
	i++
	}
	}
	return coins[:i]
	}

that's a bug!

That code mutates the underlying slice, but really we should create a copy first, and then mutate that one or simply remove the extra state and make it direct to use as

func removeZeroCoins(coins Coins) Coins {
	rmc := make(Coins, 0, len(coins))
	for _, coin := range coins {
		if !coin.IsZero() {
			rmc = append(rmc, coin)
		}
	}
	return rmc
}

and here is a demonstration of the bug https://play.golang.org/p/F8TnUkqqs_Z
or inlined below

package main

import (
	"fmt"
)

type Coin int

func (c Coin) isZero() bool { return c <= 0 }

func badRemoveZeroCoins(coins []Coin) []Coin {
	i, l := 0, len(coins)
	for i < l {
		if coins[i].isZero() {
			coins = append(coins[:i], coins[i+1:]...)
			l--
		} else {
			i++
		}
	}
	return coins[:i]
}

func goodRemoveZeroCoins(coins []Coin) []Coin {
	rmc := make([]Coin, 0, len(coins))
	for _, coin := range coins {
		if !coin.isZero() {
			rmc = append(rmc, coin)
		}
	}
	return rmc
}

func main() {
	orig1 := []Coin{0, 10, 20, 400, 0, 29}
	fmt.Printf("Orig before call: %#v\n", orig1)
	dedupe1 := badRemoveZeroCoins(orig1)
	fmt.Printf("Orig: after call: %#v\nGot:  %#v\n", orig1, dedupe1)

	orig2 := []Coin{0, 10, 20, 400, 0, 29}
	fmt.Printf("\n\nOrig before call: %#v\n", orig2)
	dedupe2 := goodRemoveZeroCoins(orig2)
	fmt.Printf("Orig: after call: %#v\nGot:  %#v\n", orig2, dedupe2)
}

which prints out

Orig before call: []main.Coin{0, 10, 20, 400, 0, 29}
Orig: after call: []main.Coin{10, 20, 400, 29, 29, 29}
Got:  []main.Coin{10, 20, 400, 29}


Orig before call: []main.Coin{0, 10, 20, 400, 0, 29}
Orig: after call: []main.Coin{0, 10, 20, 400, 0, 29}
Got:  []main.Coin{10, 20, 400, 29}

---

For Admin Use

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

[odeke-em]

Ditto for removeZeroDecCoins

cosmos-sdk/types/dec_coin.go

Lines 558 to 571 in 3123d07

	func removeZeroDecCoins(coins DecCoins) DecCoins {
	i, l := 0, len(coins)
	for i < l {
	if coins[i].IsZero() {
	// remove coin
	coins = append(coins[:i], coins[i+1:]...)
	l--
	} else {
	i++
	}
	}
	return coins[:i]
	}

We really perhaps should be aiming for correctness than cleverness in reducing memory. The code with the extra state has more state, isn't readable. The suggestion to keep it simple is more legible, correct and efficient.

[iramiller]

This is an interesting and subtle bug. Easily fixed by adding a couple unit tests that correctly show the fault, then the changes outlined above to resolve. I can submit a PR for this.