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.

Sign up for GitHub

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

feat: add Coins.Find method #11959

Merged
merged 5 commits into from
May 18, 2022
Merged

feat: add Coins.Find method #11959

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
27ed0b2
feat: add Coins.Find method
robert-zaremba May 13, 2022
ecb1cb9
changelog
robert-zaremba May 13, 2022
1fcc44b
Merge branch 'main' into robert/coins-find
julienrbrt May 16, 2022
fb39fa8
Merge branch 'main' into robert/coins-find
robert-zaremba May 18, 2022
2d30c29
use Coin{}
robert-zaremba May 18, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (types) [\#10948](https://github.com/cosmos/cosmos-sdk/issues/10948) Add `app-db-backend` to the `app.toml` config to replace the compile-time `types.DBbackend` variable.
* (authz)[\#11060](https://github.com/cosmos/cosmos-sdk/pull/11060) Support grant with no expire time.
* (rosetta) [\#11590](https://github.com/cosmos/cosmos-sdk/pull/11590) Add fee suggestion for rosetta and enable offline mode. Also force set events about Fees to Success to pass reconciliation test.
* (types) [\#11959](https://github.com/cosmos/cosmos-sdk/pull/11959) Added `sdk.Coins.Find` helper method to find a coin by denom.

### API Breaking Changes

Expand Down
21 changes: 14 additions & 7 deletions types/coin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,28 +703,35 @@ func (coins Coins) AmountOf(denom string) Int {
// AmountOfNoDenomValidation returns the amount of a denom from coins
// without validating the denomination.
func (coins Coins) AmountOfNoDenomValidation(denom string) Int {
_, c := coins.Find(denom)
return c.Amount
}

// Find returns true and coin if the denom exists in coins. Otherwise it returns false
// and a zero coin. Uses binary search.
// CONTRACT: coins must be valid (sorted).
func (coins Coins) Find(denom string) (bool, Coin) {
switch len(coins) {
case 0:
return ZeroInt()
return false, Coin{"", ZeroInt()}

case 1:
coin := coins[0]
if coin.Denom == denom {
return coin.Amount
return true, coin
}
return ZeroInt()
return false, Coin{"", ZeroInt()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just the empty Coin{} literal? The caller knows based of the bool that it's found or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Coin{} has a nil amount. ok=false is not an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not following. You'd never use the coin if ok=false, so I don't think it matters if coin actually contains and we shouldn't allocate a ZeroInt() when we don't have to IMO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AmountOfNoDenomValidation and AmountOf uses it.
I agree, that it creates an allocation which is not always needed. If you really think it's better to avoid that allocation, then I will update. lmk

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'd rather just return Coin{} IMO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, updated


default:
// Binary search the amount of coins remaining
midIdx := len(coins) / 2 // 2:1, 3:1, 4:2
coin := coins[midIdx]
switch {
case denom < coin.Denom:
return coins[:midIdx].AmountOfNoDenomValidation(denom)
return coins[:midIdx].Find(denom)
case denom == coin.Denom:
return coin.Amount
return true, coin
default:
return coins[midIdx+1:].AmountOfNoDenomValidation(denom)
return coins[midIdx+1:].Find(denom)
}
}
}
Expand Down
42 changes: 34 additions & 8 deletions types/coin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,7 +931,8 @@ func (s *coinTestSuite) TestSortCoins() {
}
}

func (s *coinTestSuite) TestAmountOf() {
func (s *coinTestSuite) TestSearch() {
require := s.Require()
case0 := sdk.Coins{}
case1 := sdk.Coins{
sdk.NewInt64Coin("gold", 0),
Expand All @@ -949,7 +950,7 @@ func (s *coinTestSuite) TestAmountOf() {
sdk.NewInt64Coin("gas", 8),
}

cases := []struct {
amountOfCases := []struct {
coins sdk.Coins
amountOf int64
amountOfSpace int64
Expand All @@ -964,13 +965,38 @@ func (s *coinTestSuite) TestAmountOf() {
{case4, 0, 0, 8, 0, 0},
}

for _, tc := range cases {
s.Require().Equal(sdk.NewInt(tc.amountOfGAS), tc.coins.AmountOf("gas"))
s.Require().Equal(sdk.NewInt(tc.amountOfMINERAL), tc.coins.AmountOf("mineral"))
s.Require().Equal(sdk.NewInt(tc.amountOfTREE), tc.coins.AmountOf("tree"))
}
s.Run("AmountOf", func() {
for i, tc := range amountOfCases {
require.Equal(sdk.NewInt(tc.amountOfGAS), tc.coins.AmountOf("gas"), i)
require.Equal(sdk.NewInt(tc.amountOfMINERAL), tc.coins.AmountOf("mineral"), i)
require.Equal(sdk.NewInt(tc.amountOfTREE), tc.coins.AmountOf("tree"), i)
}
require.Panics(func() { amountOfCases[0].coins.AmountOf("10Invalid") })
})

s.Require().Panics(func() { cases[0].coins.AmountOf("10Invalid") })
zeroCoin := sdk.Coin{"", sdk.ZeroInt()}
findCases := []struct {
coins sdk.Coins
denom string
expectedOk bool
expectedCoin sdk.Coin
}{
{case0, "any", false, zeroCoin},
{case1, "other", false, zeroCoin},
{case1, "gold", true, case1[0]},
{case4, "gas", true, case4[0]},
{case2, "gas", true, case2[0]},
{case2, "mineral", true, case2[1]},
{case2, "tree", true, case2[2]},
{case2, "other", false, zeroCoin},
}
s.Run("Find", func() {
for i, tc := range findCases {
ok, c := tc.coins.Find(tc.denom)
require.Equal(tc.expectedOk, ok, i)
require.Equal(tc.expectedCoin, c, i)
}
})
}

func (s *coinTestSuite) TestCoinsIsAnyGTE() {
Expand Down