From e5ffb813c624772051b636f3634aac6dabb2a409 Mon Sep 17 00:00:00 2001 From: Sishir Giri Date: Mon, 18 Jul 2022 15:45:43 -0700 Subject: [PATCH] refactor(x/mint, x/gamm errors): move errors in mint and gamm keeper to errors.go (#2067) * moved mint errors * errors formatted * rom and bez feedback * rebased * addressed all comments --- x/gamm/pool-models/balancer/amm.go | 4 +-- x/gamm/pool-models/balancer/export_test.go | 6 ++-- x/gamm/pool-models/balancer/pool.go | 32 +++++++++++----------- x/gamm/types/errors.go | 1 + x/mint/keeper/export_test.go | 4 --- x/mint/keeper/keeper.go | 14 +++------- x/mint/keeper/keeper_test.go | 12 ++++---- x/mint/types/errors.go | 11 ++++++++ 8 files changed, 43 insertions(+), 41 deletions(-) create mode 100644 x/mint/types/errors.go diff --git a/x/gamm/pool-models/balancer/amm.go b/x/gamm/pool-models/balancer/amm.go index d24714d5a2c..150a8595d2a 100644 --- a/x/gamm/pool-models/balancer/amm.go +++ b/x/gamm/pool-models/balancer/amm.go @@ -151,7 +151,7 @@ func getPoolAssetsByDenom(poolAssets []PoolAsset) (map[string]PoolAsset, error) for _, poolAsset := range poolAssets { _, ok := poolAssetsByDenom[poolAsset.Token.Denom] if ok { - return nil, fmt.Errorf(errMsgFormatRepeatingPoolAssetsNotAllowed, poolAsset.Token.Denom) + return nil, fmt.Errorf(formatRepeatingPoolAssetsNotAllowedErrFormat, poolAsset.Token.Denom) } poolAssetsByDenom[poolAsset.Token.Denom] = poolAsset @@ -174,7 +174,7 @@ func updateIntermediaryPoolAssetsLiquidity(liquidity sdk.Coins, poolAssetsByDeno for _, coin := range liquidity { poolAsset, ok := poolAssetsByDenom[coin.Denom] if !ok { - return fmt.Errorf(errMsgFormatFailedInterimLiquidityUpdate, coin.Denom) + return fmt.Errorf(failedInterimLiquidityUpdateErrFormat, coin.Denom) } poolAsset.Token.Amount = poolAssetsByDenom[coin.Denom].Token.Amount.Add(coin.Amount) diff --git a/x/gamm/pool-models/balancer/export_test.go b/x/gamm/pool-models/balancer/export_test.go index 75677b7e559..5f65f9c2c0e 100644 --- a/x/gamm/pool-models/balancer/export_test.go +++ b/x/gamm/pool-models/balancer/export_test.go @@ -3,12 +3,12 @@ package balancer import sdk "github.com/cosmos/cosmos-sdk/types" const ( - ErrMsgFormatRepeatingPoolAssetsNotAllowed = errMsgFormatRepeatingPoolAssetsNotAllowed - ErrMsgFormatNoPoolAssetFound = errMsgFormatNoPoolAssetFound + ErrMsgFormatRepeatingPoolAssetsNotAllowed = formatRepeatingPoolAssetsNotAllowedErrFormat + ErrMsgFormatNoPoolAssetFound = formatNoPoolAssetFoundErrFormat ) var ( - ErrMsgFormatFailedInterimLiquidityUpdate = errMsgFormatFailedInterimLiquidityUpdate + ErrMsgFormatFailedInterimLiquidityUpdate = failedInterimLiquidityUpdateErrFormat CalcPoolSharesOutGivenSingleAssetIn = calcPoolSharesOutGivenSingleAssetIn CalcSingleAssetInGivenPoolSharesOut = calcSingleAssetInGivenPoolSharesOut diff --git a/x/gamm/pool-models/balancer/pool.go b/x/gamm/pool-models/balancer/pool.go index f0c887b7bd8..51e82bd64d6 100644 --- a/x/gamm/pool-models/balancer/pool.go +++ b/x/gamm/pool-models/balancer/pool.go @@ -17,14 +17,14 @@ import ( //nolint:deadcode const ( - errMsgFormatSharesAmountNotPositive = "shares amount must be positive, was %d" - errMsgFormatTokenAmountNotPositive = "token amount must be positive, was %d" - errMsgFormatTokensLargerThanMax = "%d resulted tokens is larger than the max amount of %d" - errMsgFormatSharesLargerThanMax = "%d resulted shares is larger than the max amount of %d" - errMsgFormatFailedInterimLiquidityUpdate = "failed to update interim liquidity - pool asset %s does not exist" - errMsgFormatRepeatingPoolAssetsNotAllowed = "repeating pool assets not allowed, found %s" - errMsgFormatNoPoolAssetFound = "can't find the PoolAsset (%s)" - errMsgFormatInvalidInputDenoms = "input denoms must already exist in the pool (%s)" + nonPostiveSharesAmountErrFormat = "shares amount must be positive, was %d" + nonPostiveTokenAmountErrFormat = "token amount must be positive, was %d" + sharesLargerThanMaxErrFormat = "%d resulted shares is larger than the max amount of %d" + invalidInputDenomsErrFormat = "input denoms must already exist in the pool (%s)" + + failedInterimLiquidityUpdateErrFormat = "failed to update interim liquidity - pool asset %s does not exist" + formatRepeatingPoolAssetsNotAllowedErrFormat = "repeating pool assets not allowed, found %s" + formatNoPoolAssetFoundErrFormat = "can't find the PoolAsset (%s)" ) var ( @@ -223,7 +223,7 @@ func (p Pool) getPoolAssetAndIndex(denom string) (int, PoolAsset, error) { } if len(p.PoolAssets) == 0 { - return -1, PoolAsset{}, sdkerrors.Wrapf(types.ErrDenomNotFoundInPool, fmt.Sprintf(errMsgFormatNoPoolAssetFound, denom)) + return -1, PoolAsset{}, sdkerrors.Wrapf(types.ErrDenomNotFoundInPool, fmt.Sprintf(formatNoPoolAssetFoundErrFormat, denom)) } i := sort.Search(len(p.PoolAssets), func(i int) bool { @@ -234,11 +234,11 @@ func (p Pool) getPoolAssetAndIndex(denom string) (int, PoolAsset, error) { }) if i < 0 || i >= len(p.PoolAssets) { - return -1, PoolAsset{}, sdkerrors.Wrapf(types.ErrDenomNotFoundInPool, fmt.Sprintf(errMsgFormatNoPoolAssetFound, denom)) + return -1, PoolAsset{}, sdkerrors.Wrapf(types.ErrDenomNotFoundInPool, fmt.Sprintf(formatNoPoolAssetFoundErrFormat, denom)) } if p.PoolAssets[i].Token.Denom != denom { - return -1, PoolAsset{}, sdkerrors.Wrapf(types.ErrDenomNotFoundInPool, fmt.Sprintf(errMsgFormatNoPoolAssetFound, denom)) + return -1, PoolAsset{}, sdkerrors.Wrapf(types.ErrDenomNotFoundInPool, fmt.Sprintf(formatNoPoolAssetFoundErrFormat, denom)) } return i, p.PoolAssets[i], nil @@ -692,7 +692,7 @@ func (p *Pool) CalcJoinPoolShares(ctx sdk.Context, tokensIn sdk.Coins, swapFee s for _, coin := range tokensIn { _, ok := poolAssetsByDenom[coin.Denom] if !ok { - return sdk.ZeroInt(), sdk.NewCoins(), fmt.Errorf(errMsgFormatInvalidInputDenoms, coin.Denom) + return sdk.ZeroInt(), sdk.NewCoins(), sdkerrors.Wrapf(types.ErrDenomAlreadyInPool, invalidInputDenomsErrFormat, coin.Denom) } } @@ -827,7 +827,7 @@ func (p *Pool) CalcTokenInShareAmountOut( ).Ceil().TruncateInt() if !tokenInAmount.IsPositive() { - return sdk.Int{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, errMsgFormatTokenAmountNotPositive, tokenInAmount.Int64()) + return sdk.Int{}, sdkerrors.Wrapf(types.ErrNotPositiveRequireAmount, nonPostiveTokenAmountErrFormat, tokenInAmount.Int64()) } return tokenInAmount, nil @@ -854,7 +854,7 @@ func (p *Pool) JoinPoolTokenInMaxShareAmountOut( ).TruncateInt() if !tokenInAmount.IsPositive() { - return sdk.Int{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, errMsgFormatTokenAmountNotPositive, tokenInAmount.Int64()) + return sdk.Int{}, sdkerrors.Wrapf(types.ErrNotPositiveRequireAmount, nonPostiveTokenAmountErrFormat, tokenInAmount.Int64()) } poolAssetIn.Token.Amount = poolAssetIn.Token.Amount.Add(tokenInAmount) @@ -886,11 +886,11 @@ func (p *Pool) ExitSwapExactAmountOut( ).TruncateInt() if !sharesIn.IsPositive() { - return sdk.Int{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, errMsgFormatSharesAmountNotPositive, sharesIn.Int64()) + return sdk.Int{}, sdkerrors.Wrapf(types.ErrNotPositiveRequireAmount, nonPostiveSharesAmountErrFormat, sharesIn.Int64()) } if sharesIn.GT(shareInMaxAmount) { - return sdk.Int{}, sdkerrors.Wrapf(types.ErrLimitMaxAmount, errMsgFormatSharesLargerThanMax, sharesIn.Int64(), shareInMaxAmount.Uint64()) + return sdk.Int{}, sdkerrors.Wrapf(types.ErrLimitMaxAmount, sharesLargerThanMaxErrFormat, sharesIn.Int64(), shareInMaxAmount.Uint64()) } if err := p.exitPool(ctx, sdk.NewCoins(tokenOut), sharesIn); err != nil { diff --git a/x/gamm/types/errors.go b/x/gamm/types/errors.go index faa26df35dc..dcf6dc70cdf 100644 --- a/x/gamm/types/errors.go +++ b/x/gamm/types/errors.go @@ -15,6 +15,7 @@ var ( ErrAlreadyInvalidPool = sdkerrors.Register(ModuleName, 9, "destruction on already invalid pool") ErrInvalidPool = sdkerrors.Register(ModuleName, 10, "attempting to create an invalid pool") ErrDenomNotFoundInPool = sdkerrors.Register(ModuleName, 11, "denom does not exist in pool") + ErrDenomAlreadyInPool = sdkerrors.Register(ModuleName, 12, "denom already exists in the pool") ErrEmptyRoutes = sdkerrors.Register(ModuleName, 21, "routes not defined") ErrEmptyPoolAssets = sdkerrors.Register(ModuleName, 22, "PoolAssets not defined") diff --git a/x/mint/keeper/export_test.go b/x/mint/keeper/export_test.go index 888411f125f..6c27f1aebbb 100644 --- a/x/mint/keeper/export_test.go +++ b/x/mint/keeper/export_test.go @@ -15,10 +15,6 @@ const ( ) var ( - ErrAmountCannotBeNilOrZero = errAmountCannotBeNilOrZero - ErrDevVestingModuleAccountAlreadyCreated = errDevVestingModuleAccountAlreadyCreated - ErrDevVestingModuleAccountNotCreated = errDevVestingModuleAccountNotCreated - GetProportions = getProportions ) diff --git a/x/mint/keeper/keeper.go b/x/mint/keeper/keeper.go index 93dab130363..cdcde523f81 100644 --- a/x/mint/keeper/keeper.go +++ b/x/mint/keeper/keeper.go @@ -1,7 +1,6 @@ package keeper import ( - "errors" "fmt" "github.com/tendermint/tendermint/libs/log" @@ -11,6 +10,7 @@ import ( "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" paramtypes "github.com/cosmos/cosmos-sdk/x/params/types" ) @@ -36,12 +36,6 @@ func (e invalidRatioError) Error() string { return fmt.Sprintf("mint allocation ratio (%s) is greater than 1", e.ActualRatio) } -var ( - errAmountCannotBeNilOrZero = errors.New("amount cannot be nil or zero") - errDevVestingModuleAccountAlreadyCreated = fmt.Errorf("%s module account already exists", types.DeveloperVestingModuleAcctName) - errDevVestingModuleAccountNotCreated = fmt.Errorf("%s module account does not exist", types.DeveloperVestingModuleAcctName) -) - // NewKeeper creates a new mint Keeper instance. func NewKeeper( cdc codec.BinaryCodec, key sdk.StoreKey, paramSpace paramtypes.Subspace, @@ -79,7 +73,7 @@ func NewKeeper( // queries. The method returns an error if current height in ctx is greater than the v7 upgrade height. func (k Keeper) SetInitialSupplyOffsetDuringMigration(ctx sdk.Context) error { if !k.accountKeeper.HasAccount(ctx, k.accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName)) { - return errDevVestingModuleAccountNotCreated + return sdkerrors.Wrapf(types.ErrModuleDoesnotExist, "%s vesting module account doesnot exist", types.DeveloperVestingModuleAcctName) } moduleAccBalance := k.bankKeeper.GetBalance(ctx, k.accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName), k.GetParams(ctx).MintDenom) @@ -96,10 +90,10 @@ func (k Keeper) SetInitialSupplyOffsetDuringMigration(ctx sdk.Context) error { // - developer vesting module account is already created prior to calling this method. func (k Keeper) CreateDeveloperVestingModuleAccount(ctx sdk.Context, amount sdk.Coin) error { if amount.IsNil() || amount.Amount.IsZero() { - return errAmountCannotBeNilOrZero + return sdkerrors.Wrap(types.ErrAmountNilOrZero, "amount cannot be nil or zero") } if k.accountKeeper.HasAccount(ctx, k.accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName)) { - return errDevVestingModuleAccountAlreadyCreated + return sdkerrors.Wrapf(types.ErrModuleAccountAlreadyExist, "%s vesting module account already exist", types.DeveloperVestingModuleAcctName) } moduleAcc := authtypes.NewEmptyModuleAccount( diff --git a/x/mint/keeper/keeper_test.go b/x/mint/keeper/keeper_test.go index 12eb528e3b1..ecde05b23c7 100644 --- a/x/mint/keeper/keeper_test.go +++ b/x/mint/keeper/keeper_test.go @@ -315,18 +315,18 @@ func (suite *KeeperTestSuite) TestCreateDeveloperVestingModuleAccount() { }, "nil amount": { blockHeight: 0, - expectedError: keeper.ErrAmountCannotBeNilOrZero, + expectedError: sdkerrors.Wrap(types.ErrAmountNilOrZero, "amount cannot be nil or zero"), }, "zero amount": { blockHeight: 0, amount: sdk.NewCoin("stake", sdk.NewInt(0)), - expectedError: keeper.ErrAmountCannotBeNilOrZero, + expectedError: sdkerrors.Wrap(types.ErrAmountNilOrZero, "amount cannot be nil or zero"), }, "module account is already created": { blockHeight: 0, amount: sdk.NewCoin("stake", sdk.NewInt(keeper.DeveloperVestingAmount)), isDeveloperModuleAccountCreated: true, - expectedError: keeper.ErrDevVestingModuleAccountAlreadyCreated, + expectedError: sdkerrors.Wrapf(types.ErrModuleAccountAlreadyExist, "%s vesting module account already exist", types.DeveloperVestingModuleAcctName), }, } @@ -340,7 +340,7 @@ func (suite *KeeperTestSuite) TestCreateDeveloperVestingModuleAccount() { if tc.expectedError != nil { suite.Error(actualError) - suite.Equal(actualError, tc.expectedError) + suite.ErrorIs(actualError, tc.expectedError) return } suite.NoError(actualError) @@ -361,7 +361,7 @@ func (suite *KeeperTestSuite) TestSetInitialSupplyOffsetDuringMigration() { }, "dev vesting module account does not exist": { blockHeight: 1, - expectedError: keeper.ErrDevVestingModuleAccountNotCreated, + expectedError: sdkerrors.Wrapf(types.ErrModuleDoesnotExist, "%s vesting module account doesnot exist", types.DeveloperVestingModuleAcctName), }, } @@ -380,7 +380,7 @@ func (suite *KeeperTestSuite) TestSetInitialSupplyOffsetDuringMigration() { if tc.expectedError != nil { suite.Error(actualError) - suite.Equal(actualError, tc.expectedError) + suite.ErrorIs(actualError, tc.expectedError) suite.Equal(supplyWithOffsetBefore.Amount, bankKeeper.GetSupplyWithOffset(ctx, sdk.DefaultBondDenom).Amount) suite.Equal(supplyOffsetBefore, bankKeeper.GetSupplyOffset(ctx, sdk.DefaultBondDenom)) diff --git a/x/mint/types/errors.go b/x/mint/types/errors.go new file mode 100644 index 00000000000..6f721cb74a7 --- /dev/null +++ b/x/mint/types/errors.go @@ -0,0 +1,11 @@ +package types + +import ( + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +var ( + ErrAmountNilOrZero = sdkerrors.Register(ModuleName, 2, "amount cannot be nil or zero") + ErrModuleAccountAlreadyExist = sdkerrors.Register(ModuleName, 3, "module account already exists") + ErrModuleDoesnotExist = sdkerrors.Register(ModuleName, 4, "module account does not exist") +)