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

Jump to bottom

refactor: remove usage of global basedenom #18268

Merged
merged 8 commits into from
Oct 30, 2023
Merged

refactor: remove usage of global basedenom #18268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ec74856
use param denom in staking instead of global
tac0turtle Oct 26, 2023
66b74db
depreacte global
tac0turtle Oct 26, 2023
f3641ed
fix build
tac0turtle Oct 26, 2023
5f1db5a
remove global basedenom
tac0turtle Oct 26, 2023
835146b
add changelog
tac0turtle Oct 26, 2023
eb43914
Merge branch 'main' into marko/remove_bonddenom_global
tac0turtle Oct 30, 2023
c4940a0
clean up code
tac0turtle Oct 30, 2023
dd5957e
further cleanup
tac0turtle Oct 30, 2023
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 @@ -184,6 +184,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/mint) [#18283](https://github.com/cosmos/cosmos-sdk/pull/18283) Mint module was moved to its own go.mod `cosmossdk.io/x/mint`
* (x/consensus) [#18041](https://github.com/cosmos/cosmos-sdk/pull/18041) `ToProtoConsensusParams()` returns an error
* (x/slashing) [#18115](https://github.com/cosmos/cosmos-sdk/pull/18115) `NewValidatorSigningInfo` takes strings instead of `sdk.AccAddress`
* (types) [#18268](https://github.com/cosmos/cosmos-sdk/pull/18268) Remove global setting of basedenom. Use the staking module parameter instead

### CLI Breaking Changes

Expand Down
20 changes: 19 additions & 1 deletion types/coin.go
View file
Open in desktop
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,8 @@ func ParseCoinNormalized(coinStr string) (coin Coin, err error) {
return Coin{}, err
}

coin, _ = NormalizeDecCoin(decCoin).TruncateDecimal()
coin, _ = NewDecCoinFromDec(decCoin.Denom, decCoin.Amount).TruncateDecimal()

Comment on lines +884 to +885
Copy link
Contributor

Choose a reason for hiding this comment

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

The error returned by NewDecCoinFromDec().TruncateDecimal() is ignored. It's a good practice to handle all errors to prevent unexpected behavior. Consider adding error handling.

- coin, _ = NewDecCoinFromDec(decCoin.Denom, decCoin.Amount).TruncateDecimal()
+ coin, err = NewDecCoinFromDec(decCoin.Denom, decCoin.Amount).TruncateDecimal()
+ if err != nil {
+     // handle error
+ }
Commitable suggestion (Beta)
Suggested change
coin, _ = NewDecCoinFromDec(decCoin.Denom, decCoin.Amount).TruncateDecimal()
coin, err := NewDecCoinFromDec(decCoin.Denom, decCoin.Amount).TruncateDecimal()
if err != nil {
// handle error
}

return coin, nil
}

Expand All @@ -900,3 +901,20 @@ func ParseCoinsNormalized(coinStr string) (Coins, error) {
}
return NormalizeCoins(coins), nil
}

// ----------------------------------------------------------------------------

// NormalizeCoins normalize and truncate a list of decimal coins
func NormalizeCoins(coins []DecCoin) Coins {
if coins == nil {
return nil
}
result := make([]Coin, 0, len(coins))

for _, coin := range coins {
newCoin, _ := NewDecCoinFromDec(coin.Denom, coin.Amount).TruncateDecimal()
result = append(result, newCoin)
}

return result
}
Comment on lines +907 to +920
Copy link
Contributor

Choose a reason for hiding this comment

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

The function NormalizeCoins is well implemented. It checks if the input is nil and returns nil in that case. It also preallocates the slice result with the length of coins which is a good practice for performance. However, similar to the previous comment, the error returned by NewDecCoinFromDec().TruncateDecimal() is ignored. Consider adding error handling.

- newCoin, _ := NewDecCoinFromDec(coin.Denom, coin.Amount).TruncateDecimal()
+ newCoin, err := NewDecCoinFromDec(coin.Denom, coin.Amount).TruncateDecimal()
+ if err != nil {
+     // handle error
+ }
Commitable suggestion (Beta)
Suggested change
// NormalizeCoins normalize and truncate a list of decimal coins
func NormalizeCoins(coins []DecCoin) Coins {
if coins == nil {
return nil
}
result := make([]Coin, 0, len(coins))
for _, coin := range coins {
newCoin, _ := NewDecCoinFromDec(coin.Denom, coin.Amount).TruncateDecimal()
result = append(result, newCoin)
}
return result
}
// NormalizeCoins normalize and truncate a list of decimal coins
func NormalizeCoins(coins []DecCoin) Coins {
if coins == nil {
return nil
}
result := make([]Coin, 0, len(coins))
for _, coin := range coins {
newCoin, err := NewDecCoinFromDec(coin.Denom, coin.Amount).TruncateDecimal()
if err != nil {
// handle error
}
result = append(result, newCoin)
}
return result
}

160 changes: 0 additions & 160 deletions types/denom.go
View file
Open in desktop

This file was deleted.

Loading