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

refactor: remove usage of global basedenom #18268

Merged
merged 8 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/staking) [#18257](https://github.com/cosmos/cosmos-sdk/pull/18257) Staking module was moved to its own go.mod `cosmossdk.io/x/staking`
* (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
74 changes: 73 additions & 1 deletion types/coin.go
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,7 @@ func ParseCoinNormalized(coinStr string) (coin Coin, err error) {
return Coin{}, err
}

coin, _ = NormalizeDecCoin(decCoin).TruncateDecimal()
coin, _ = normalizeDecCoin(decCoin).TruncateDecimal()
odeke-em marked this conversation as resolved.
Show resolved Hide resolved
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
return coin, nil
}

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

// ----------------------------------------------------------------------------
// Functions used for testing purposes.

// NormalizeDecCoin try to convert a decimal coin to the smallest unit registered,
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
// returns original one if failed.
func normalizeDecCoin(coin DecCoin) DecCoin {
newCoin, err := convertDecCoin(coin, coin.Denom)
if err != nil {
return coin
}
return newCoin
}
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 normalizeDecCoin tries to convert a decimal coin to the smallest unit registered and returns the original coin if the conversion fails. However, it's not clear what the failure conditions are. It would be helpful to document under what conditions the conversion might fail.


// 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, _ := normalizeDecCoin(coin).TruncateDecimal()
result = append(result, newCoin)
}
odeke-em marked this conversation as resolved.
Show resolved Hide resolved
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved

return result
}

// ConvertDecCoin attempts to convert a decimal coin to a given denomination. If the given
// denomination is invalid or if neither denomination is registered, an error
// is returned.
func convertDecCoin(coin DecCoin, denom string) (DecCoin, error) {
if err := ValidateDenom(denom); err != nil {
return DecCoin{}, err
}

srcUnit, ok := getDenomUnit(coin.Denom)
if !ok {
return DecCoin{}, fmt.Errorf("source denom not registered: %s", coin.Denom)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit, please quote the value with %q

}

dstUnit, ok := getDenomUnit(denom)
if !ok {
return DecCoin{}, fmt.Errorf("destination denom not registered: %s", denom)
}

if srcUnit.Equal(dstUnit) {
return NewDecCoinFromDec(denom, coin.Amount), nil
}

return NewDecCoinFromDec(denom, coin.Amount.Mul(srcUnit).Quo(dstUnit)), nil
}

// denomUnits contains a mapping of denomination mapped to their respective unit
// multipliers (e.g. 1atom = 10^-6uatom).
var denomUnits = map[string]math.LegacyDec{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is denomUnits being set? I only see it being used to lookup

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you, it was related to the global, it is not needed anymore


// getDenomUnit returns a unit for a given denomination if it exists. A boolean
// is returned if the denomination is registered.
func getDenomUnit(denom string) (math.LegacyDec, bool) {
if err := ValidateDenom(denom); err != nil {
return math.LegacyZeroDec(), false
}

unit, ok := denomUnits[denom]
if !ok {
return math.LegacyZeroDec(), false
}

return unit, true
}
160 changes: 0 additions & 160 deletions types/denom.go

This file was deleted.

Loading