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

types: unnecessary code inside removeZeroCoins adding extra runtime and less readability #13958

Closed
odeke-em opened this issue Nov 21, 2022 · 2 comments · Fixed by #13967
Closed

Comments

@odeke-em
Copy link
Collaborator

Summary of Bug

Noticed while auditing cosmos/gaia that if we examine the code inside

cosmos-sdk/types/coin.go

Lines 782 to 803 in 8cce748

func removeZeroCoins(coins Coins) Coins {
for i := 0; i < len(coins); i++ {
if coins[i].IsZero() {
break
} else if i == len(coins)-1 {
return coins
}
}
var result []Coin
if len(coins) > 0 {
result = make([]Coin, 0, len(coins)-1)
}
for _, coin := range coins {
if !coin.IsZero() {
result = append(result, coin)
}
}
return result
}
we can see that the first for loop of code tries to check if at least one coin is zero then exit else if at the end return. However in the next loop, it performs the iteration and checks for coins that are non-zero which thereby renders the prior loop useless.
image

Suggestion

Simply remove the first loop and the other unnecessary checks then only add coins that are non-zero

func removeZeroCoins(coins Coins) Coins {
	nonZeros := make([]Coin, 0, len(coins))

	for _, coin := range coins {
		if !coin.IsZero() {
			nonZeros = append(nonZeros, coin)
		}
	}

	return nonZeros
}

/cc @elias-orijtech

@tac0turtle
Copy link
Member

want to submit a pr?

odeke-em added a commit that referenced this issue Nov 22, 2022
Simplifies and makes clearer the code in removeZeroCoins by
removing unnecessary checks that boiled down to still running
in the same final for loop.

Fixes #13958
@odeke-em
Copy link
Collaborator Author

Sure @tac0turtle I've mailed PR #13967 though ideally on a code binge we open a bunch of issues firstly then we shall mail fixes :-)

tac0turtle pushed a commit that referenced this issue Nov 22, 2022
…3967)

Simplifies and makes clearer the code in removeZeroCoins by
removing unnecessary checks that boiled down to still running
in the same final for loop.

Fixes #13958
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

Successfully merging a pull request may close this issue.

2 participants