refactor: remove usage of global basedenom #18268
Conversation
WalkthroughThe codebase underwent a significant refactor, primarily focusing on deprecating certain functions and variables in favor of using the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- types/denom.go (4} hunks)
- x/distribution/keeper/delegation.go (1} hunks)
- x/distribution/testutil/expected_keepers_mocks.go (26} hunks)
- x/distribution/types/expected_keepers.go (1} hunks)
- x/evidence/testutil/expected_keepers_mocks.go (14} hunks)
- x/gov/testutil/expected_keepers_mocks.go (62} hunks)
- x/slashing/testutil/expected_keepers_mocks.go (27} hunks)
- x/staking/testutil/expected_keepers_mocks.go (37} hunks)
Files skipped from review due to trivial changes (5)
- x/distribution/keeper/delegation.go
- x/distribution/testutil/expected_keepers_mocks.go
- x/distribution/types/expected_keepers.go
- x/gov/testutil/expected_keepers_mocks.go
- x/slashing/testutil/expected_keepers_mocks.go
Additional comments: 38
types/denom.go (4)
14-19: The
baseDenomvariable andRegisterDenomfunction have been deprecated and the recommendation is to useBaseDenomfrom the staking module instead. Ensure that all calls to these throughout the codebase have been updated to match the new usage. Also, consider removing these deprecated items in a future version to avoid confusion and maintain clean code.36-42: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [39-43]
The
GetDenomUnitfunction has been deprecated and the recommendation is to useBaseDenomfrom the staking module instead. Ensure that all calls to this function throughout the codebase have been updated to match the new usage. Also, consider removing this deprecated function in a future version to avoid confusion and maintain clean code.
- 52-58: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [55-59]
The
SetBaseDenomfunction has been deprecated and the recommendation is to useBaseDenomfrom the staking module instead. Ensure that all calls to this function throughout the codebase have been updated to match the new usage. Also, consider removing this deprecated function in a future version to avoid confusion and maintain clean code.
- 63-69: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [66-70]
The
GetBaseDenomfunction has been deprecated and the recommendation is to useBaseDenomfrom the staking module instead. Ensure that all calls to this function throughout the codebase have been updated to match the new usage. Also, consider removing this deprecated function in a future version to avoid confusion and maintain clean code.x/staking/testutil/expected_keepers_mocks.go (20)
14-16: The import paths for
typesandtypes0have been swapped. Ensure that this change is intentional and that it does not break the existing functionality. Also, verify that the new import paths are correct and the packages exist at those locations.58-61: The function
GetAccountnow usestypes0.AccAddressandtypes0.AccountIinstead oftypes.AccAddressandtypes.AccountI. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.72-75: The function
GetModuleAccountnow usestypes0.ModuleAccountIinstead oftypes.ModuleAccountI. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.86-89: The function
GetModuleAddressnow usestypes0.AccAddressinstead oftypes.AccAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.100-100: The function
IterateAccountsnow usestypes0.AccountIinstead oftypes.AccountI. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.112-112: The function
SetModuleAccountnow usestypes0.ModuleAccountIinstead oftypes.ModuleAccountI. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.147-147: The function
BurnCoinsnow usestypes0.Coinsinstead oftypes.Coins. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.161-161: The function
DelegateCoinsFromAccountToModulenow usestypes0.AccAddressandtypes0.Coinsinstead oftypes.AccAddressandtypes.Coins. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.175-178: The function
GetAllBalancesnow usestypes0.AccAddressandtypes0.Coinsinstead oftypes.AccAddressandtypes.Coins. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.189-192: The function
GetBalancenow usestypes0.AccAddressandtypes0.Coininstead oftypes.AccAddressandtypes.Coin. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.203-206: The function
GetSupplynow usestypes0.Coininstead oftypes.Coin. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.217-220: The function
LockedCoinsnow usestypes0.AccAddressandtypes0.Coinsinstead oftypes.AccAddressandtypes.Coins. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.231-231: The function
SendCoinsFromModuleToModulenow usestypes0.Coinsinstead oftypes.Coins. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.245-248: The function
SpendableCoinsnow usestypes0.AccAddressandtypes0.Coinsinstead oftypes.AccAddressandtypes.Coins. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.259-259: The function
UndelegateCoinsFromModuleToAccountnow usestypes0.AccAddressandtypes0.Coinsinstead oftypes.AccAddressandtypes.Coins. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.296-299: The function
Delegationnow usestypes0.AccAddressandtypes0.ValAddressinstead oftypes.AccAddressandtypes.ValAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.311-311: The function
GetPubKeyByConsAddrnow usestypes0.ConsAddressinstead oftypes.ConsAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.326-326: The function
IterateBondedValidatorsByPowernow usestypes.ValidatorIinstead oftypes0.ValidatorI. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.340-340: The function
IterateValidatorsnow usestypes.ValidatorIinstead oftypes0.ValidatorI. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.354-354: The function
Jailnow usestypes0.ConsAddressinstead oftypes.ConsAddress. Ensure thatx/evidence/testutil/expected_keepers_mocks.go (14)
15-17: The import paths have been updated to reflect the new package paths. Ensure that these changes are consistent across the entire codebase and that the new paths are correct.
59-62: The function signature for
ValidatorByConsAddrhas been updated. The type ofarg1has been changed fromtypes0.ConsAddresstotypes1.ConsAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.97-100: The function signature for
GetPubkeyhas been updated. The type ofarg1has been changed fromtypes.Addresstotypes0.Address. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.112-113: The function signature for
HasValidatorSigningInfohas been updated. The type ofarg1has been changed fromtypes0.ConsAddresstotypes1.ConsAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.126-127: The function signature for
IsTombstonedhas been updated. The type ofarg1has been changed fromtypes0.ConsAddresstotypes1.ConsAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.140-141: The function signature for
Jailhas been updated. The type ofarg1has been changed fromtypes0.ConsAddresstotypes1.ConsAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.154-155: The function signature for
JailUntilhas been updated. The type ofarg1has been changed fromtypes0.ConsAddresstotypes1.ConsAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.168-169: The function signature for
Slashhas been updated. The type ofarg1has been changed fromtypes0.ConsAddresstotypes1.ConsAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.197-198: The function signature for
SlashWithInfractionReasonhas been updated. The type ofarg1has been changed fromtypes0.ConsAddresstotypes1.ConsAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.211-212: The function signature for
Tombstonehas been updated. The type ofarg1has been changed fromtypes0.ConsAddresstotypes1.ConsAddress. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.248-249: The function signature for
SetAccounthas been updated. The type ofacchas been changed fromtypes0.AccountItotypes1.AccountI. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.283-286: The function signature for
GetAllBalanceshas been updated. The types ofctxandaddrhave been changed fromtypes0.Contextandtypes0.AccAddresstotypes1.Contextandtypes1.AccAddressrespectively. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.297-298: The function signature for
MintCoinshas been updated. The types ofctxandamthave been changed fromtypes0.Contextandtypes0.Coinstotypes1.Contextandtypes1.Coinsrespectively. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.311-312: The function signature for
SendCoinsFromModuleToAccounthas been updated. The types ofctx,recipientAddrandamthave been changed fromtypes0.Context,types0.AccAddressandtypes0.Coinstotypes1.Context,types1.AccAddressandtypes1.Coinsrespectively. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
|
should we just remove instead of deprecating as its something that should not be used what so ever |
github-actions
bot
added
C:x/staking
C:x/gov
C:x/slashing
C:x/distribution
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- x/distribution/keeper/delegation.go (1} hunks)
- x/distribution/keeper/delegation_test.go (1} hunks)
- x/distribution/testutil/expected_keepers_mocks.go (26} hunks)
- x/distribution/types/expected_keepers.go (1} hunks)
Files skipped from review due to trivial changes (3)
- x/distribution/keeper/delegation_test.go
- x/distribution/testutil/expected_keepers_mocks.go
- x/distribution/types/expected_keepers.go
Additional comments: 1
x/distribution/keeper/delegation.go (1)
- 304-306: The new code retrieves the
baseDenomvalue directly from thestakingKeeperinstead of using the globalsdk.GetBaseDenom()function. This is a good practice as it reduces global state and makes the code more modular and easier to test. However, the old code had a fallback tosdk.DefaultBondDenomifbaseDenomwas an empty string. If there's a possibility thatk.stakingKeeper.BondDenom(ctx)could return an empty string, you might want to add a similar fallback mechanism to ensure the function behaves as expected in all scenarios.+ baseDenom, err := k.stakingKeeper.BondDenom(ctx) + if err != nil { + return nil, err + } + if baseDenom == "" { + baseDenom = sdk.DefaultBondDenom + }Committable suggestion (Beta)
baseDenom, err := k.stakingKeeper.BondDenom(ctx) if err != nil { return nil, err } if baseDenom == "" { baseDenom = sdk.DefaultBondDenom }
|
Can we get a changelog? |
ill make a pr to 0.50 on deprecation |
tac0turtle
force-pushed
the
marko/remove_bonddenom_global
branch
from
98ae28e
to
835146b
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1} hunks)
- types/coin.go (2} hunks)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Additional comments: 3
types/coin.go (3)
903-974: The new functions
normalizeDecCoin,NormalizeCoins,convertDecCoin, andgetDenomUnitare added for coin normalization and conversion. These functions are used to convert coins to their smallest registered unit. If the conversion fails, the original coin is returned. ThedenomUnitsmap is used to store the unit multipliers for each denomination. ThegetDenomUnitfunction retrieves the unit multiplier for a given denomination. If the denomination is not registered, it returns zero and false. These changes improve the flexibility of coin handling by allowing coins to be converted to different denominations.932-955: The function
convertDecCoinattempts 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. This function improves the flexibility of coin handling by allowing coins to be converted to different denominations.958-974: The
denomUnitsmap and thegetDenomUnitfunction are used to store and retrieve the unit multipliers for each denomination. If the denomination is not registered,getDenomUnitreturns zero and false. This allows for flexible handling of different coin denominations.
types/coin.go
Outdated
| // NormalizeDecCoin try to convert a decimal coin to the smallest unit registered, | ||
| // returns original one if failed. | ||
| func normalizeDecCoin(coin DecCoin) DecCoin { | ||
| newCoin, err := convertDecCoin(coin, coin.Denom) | ||
| if err != nil { | ||
| return coin | ||
| } | ||
| return newCoin | ||
| } |
There was a problem hiding this comment.
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.
types/coin.go
Outdated
|
|
||
| srcUnit, ok := getDenomUnit(coin.Denom) | ||
| if !ok { | ||
| return DecCoin{}, fmt.Errorf("source denom not registered: %s", coin.Denom) |
There was a problem hiding this comment.
Nit, please quote the value with %q
types/coin.go
Outdated
|
|
||
| // denomUnits contains a mapping of denomination mapped to their respective unit | ||
| // multipliers (e.g. 1atom = 10^-6uatom). | ||
| var denomUnits = map[string]math.LegacyDec{} |
There was a problem hiding this comment.
Where is denomUnits being set? I only see it being used to lookup
There was a problem hiding this comment.
thank you, it was related to the global, it is not needed anymore
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- x/distribution/keeper/delegation_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/distribution/keeper/delegation_test.go
types/coin.go
Outdated
| // normalizeDecCoin try to convert a decimal coin to the smallest unit registered, | ||
| // returns original one if failed. | ||
| func normalizeDecCoin(coin DecCoin) DecCoin { | ||
| newCoin, err := convertDecCoin(coin) | ||
| if err != nil { | ||
| return coin | ||
| } | ||
| return newCoin | ||
| } |
There was a problem hiding this comment.
The function normalizeDecCoin is marked as being used for testing purposes, but it's being used in production code. If it's meant to be used in production code, the comment should be updated to reflect this.
There was a problem hiding this comment.
@tac0turtle kindly check out this comment.
There was a problem hiding this comment.
Looks like humans are talking. I'll be quiet for now. If you need me again, just tag @coderabbitai in a new comment.
types/coin.go
Outdated
| func convertDecCoin(coin DecCoin) (DecCoin, error) { | ||
| if err := ValidateDenom(coin.Denom); err != nil { | ||
| return DecCoin{}, err | ||
| } | ||
|
|
||
| return NewDecCoinFromDec(coin.Denom, coin.Amount), nil | ||
| } |
There was a problem hiding this comment.
The function convertDecCoin doesn't seem to be doing any conversion. It's just creating a new DecCoin with the same denomination and amount as the input coin. If there's no conversion happening, consider renaming the function to better reflect its purpose.
types/coin.go
Outdated
| // normalizeDecCoin try to convert a decimal coin to the smallest unit registered, | ||
| // returns original one if failed. | ||
| func normalizeDecCoin(coin DecCoin) DecCoin { | ||
| newCoin, err := convertDecCoin(coin) | ||
| if err != nil { | ||
| return coin | ||
| } | ||
| return newCoin | ||
| } |
There was a problem hiding this comment.
@tac0turtle kindly check out this comment.
| coin, _ = NewDecCoinFromDec(decCoin.Denom, decCoin.Amount).TruncateDecimal() | ||
|
|
There was a problem hiding this comment.
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)
| coin, _ = NewDecCoinFromDec(decCoin.Denom, decCoin.Amount).TruncateDecimal() | |
| coin, err := NewDecCoinFromDec(decCoin.Denom, decCoin.Amount).TruncateDecimal() | |
| if err != nil { | |
| // handle error | |
| } |
| // 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 | ||
| } |
There was a problem hiding this comment.
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)
| // 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 | |
| } |
|
[Cosmos SDK - Distribution] Kudos, SonarCloud Quality Gate passed!
|
Description
this was brought to my attention by celestia team, i moved to deprecate the global as its not used any longer
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
Refactor:
types/denom.goto encourage the use ofBaseDenomfrom the staking module.baseDenomvalue inx/distribution/keeper/delegation.gofor improved code readability.New Features:
BondDenom()to theStakingKeeperinterface inx/distribution/types/expected_keepers.go.types/coin.gofor coin normalization and conversion.Tests:
x/distribution/keeper/delegation_test.goto include expectations for the newBondDenomfunction.Chores:
Documentation:
CHANGELOG.mdto reflect changes in the "types" module and other modifications.