-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(x/mint): remove staking as a required module #21858
Changes from 15 commits
27494e8
e9d7c1d
dd4603f
0d771e4
3954247
a16eea6
bad6264
321ea18
246e4e1
870debd
b55219b
ab8fc1d
f21026b
dbde391
3154eb4
3d0fe07
ace9fd4
da93369
c74d409
1b006bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
tac0turtle marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,14 @@ | ||
package keeper_test | ||
|
||
import ( | ||
"context" | ||
"testing" | ||
|
||
"github.com/golang/mock/gomock" | ||
"github.com/stretchr/testify/suite" | ||
|
||
"cosmossdk.io/collections" | ||
"cosmossdk.io/core/appmodule" | ||
"cosmossdk.io/log" | ||
"cosmossdk.io/math" | ||
storetypes "cosmossdk.io/store/types" | ||
|
@@ -51,14 +53,17 @@ func (s *GenesisTestSuite) SetupTest() { | |
s.sdkCtx = testCtx.Ctx | ||
s.key = key | ||
|
||
stakingKeeper := minttestutil.NewMockStakingKeeper(ctrl) | ||
accountKeeper := minttestutil.NewMockAccountKeeper(ctrl) | ||
bankKeeper := minttestutil.NewMockBankKeeper(ctrl) | ||
s.accountKeeper = accountKeeper | ||
accountKeeper.EXPECT().GetModuleAddress(minterAcc.Name).Return(minterAcc.GetAddress()) | ||
accountKeeper.EXPECT().GetModuleAccount(s.sdkCtx, minterAcc.Name).Return(minterAcc) | ||
|
||
s.keeper = keeper.NewKeeper(s.cdc, runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger()), stakingKeeper, accountKeeper, bankKeeper, "", "") | ||
s.keeper = keeper.NewKeeper(s.cdc, runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger()), accountKeeper, bankKeeper, "", "") | ||
err := s.keeper.SetMintFn(func(ctx context.Context, env appmodule.Environment, minter *types.Minter, epochId string, epochNumber int64) error { | ||
return nil | ||
}) | ||
s.NoError(err) | ||
Comment on lines
+63
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enhance test coverage for While the Suggestions:
Example test case: func (s *GenesisTestSuite) TestSetMintFn() {
called := false
err := s.keeper.SetMintFn(func(ctx context.Context, env appmodule.Environment, minter *types.Minter, epochId string, epochNumber int64) error {
called = true
return nil
})
s.NoError(err)
// Trigger a mint operation
// ...
s.True(called, "SetMintFn was not called during minting operation")
} |
||
} | ||
|
||
func (s *GenesisTestSuite) TestImportExportGenesis() { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,6 +2,7 @@ package keeper | |||||||||||||||||||
|
||||||||||||||||||||
import ( | ||||||||||||||||||||
"context" | ||||||||||||||||||||
"errors" | ||||||||||||||||||||
"fmt" | ||||||||||||||||||||
|
||||||||||||||||||||
"cosmossdk.io/collections" | ||||||||||||||||||||
|
@@ -20,7 +21,6 @@ type Keeper struct { | |||||||||||||||||||
appmodule.Environment | ||||||||||||||||||||
|
||||||||||||||||||||
cdc codec.BinaryCodec | ||||||||||||||||||||
stakingKeeper types.StakingKeeper | ||||||||||||||||||||
bankKeeper types.BankKeeper | ||||||||||||||||||||
feeCollectorName string | ||||||||||||||||||||
// the address capable of executing a MsgUpdateParams message. Typically, this | ||||||||||||||||||||
|
@@ -30,13 +30,17 @@ type Keeper struct { | |||||||||||||||||||
Schema collections.Schema | ||||||||||||||||||||
Params collections.Item[types.Params] | ||||||||||||||||||||
Minter collections.Item[types.Minter] | ||||||||||||||||||||
|
||||||||||||||||||||
// mintFn is used to mint new coins during BeginBlock. This function is in charge of | ||||||||||||||||||||
// minting new coins based on arbitrary logic, previously done through InflationCalculationFn. | ||||||||||||||||||||
// If mintFn is nil, the default minting logic is used. | ||||||||||||||||||||
mintFn types.MintFn | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clarify the behavior when 'mintFn' is nil to avoid inconsistency The comment states: "If |
||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// NewKeeper creates a new mint Keeper instance | ||||||||||||||||||||
func NewKeeper( | ||||||||||||||||||||
cdc codec.BinaryCodec, | ||||||||||||||||||||
env appmodule.Environment, | ||||||||||||||||||||
sk types.StakingKeeper, | ||||||||||||||||||||
ak types.AccountKeeper, | ||||||||||||||||||||
bk types.BankKeeper, | ||||||||||||||||||||
feeCollectorName string, | ||||||||||||||||||||
|
@@ -51,7 +55,6 @@ func NewKeeper( | |||||||||||||||||||
k := Keeper{ | ||||||||||||||||||||
Environment: env, | ||||||||||||||||||||
cdc: cdc, | ||||||||||||||||||||
stakingKeeper: sk, | ||||||||||||||||||||
bankKeeper: bk, | ||||||||||||||||||||
feeCollectorName: feeCollectorName, | ||||||||||||||||||||
authority: authority, | ||||||||||||||||||||
|
@@ -67,21 +70,20 @@ func NewKeeper( | |||||||||||||||||||
return k | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// GetAuthority returns the x/mint module's authority. | ||||||||||||||||||||
func (k Keeper) GetAuthority() string { | ||||||||||||||||||||
return k.authority | ||||||||||||||||||||
} | ||||||||||||||||||||
// SetMintFn is used to mint new coins during BeginBlock. The mintFn function is in charge of | ||||||||||||||||||||
// minting new coins based on arbitrary logic, previously done through InflationCalculationFn. | ||||||||||||||||||||
func (k *Keeper) SetMintFn(mintFn types.MintFn) error { | ||||||||||||||||||||
if mintFn == nil { | ||||||||||||||||||||
return errors.New("mintFn cannot be nil") | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// StakingTokenSupply implements an alias call to the underlying staking keeper's | ||||||||||||||||||||
// StakingTokenSupply to be used in BeginBlocker. | ||||||||||||||||||||
func (k Keeper) StakingTokenSupply(ctx context.Context) (math.Int, error) { | ||||||||||||||||||||
return k.stakingKeeper.StakingTokenSupply(ctx) | ||||||||||||||||||||
k.mintFn = mintFn | ||||||||||||||||||||
return nil | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// BondedRatio implements an alias call to the underlying staking keeper's | ||||||||||||||||||||
// BondedRatio to be used in BeginBlocker. | ||||||||||||||||||||
func (k Keeper) BondedRatio(ctx context.Context) (math.LegacyDec, error) { | ||||||||||||||||||||
return k.stakingKeeper.BondedRatio(ctx) | ||||||||||||||||||||
// GetAuthority returns the x/mint module's authority. | ||||||||||||||||||||
func (k Keeper) GetAuthority() string { | ||||||||||||||||||||
return k.authority | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// MintCoins implements an alias call to the underlying supply keeper's | ||||||||||||||||||||
|
@@ -101,20 +103,26 @@ func (k Keeper) AddCollectedFees(ctx context.Context, fees sdk.Coins) error { | |||||||||||||||||||
return k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, k.feeCollectorName, fees) | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
func (k Keeper) DefaultMintFn(ic types.InflationCalculationFn) types.MintFn { | ||||||||||||||||||||
func (k Keeper) MintFn(ctx context.Context, minter *types.Minter, epochId string, epochNumber int64) error { | ||||||||||||||||||||
return k.mintFn(ctx, k.Environment, minter, epochId, epochNumber) | ||||||||||||||||||||
} | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add nil check in 'MintFn' method to prevent potential panics The Apply this diff to add a nil check: func (k Keeper) MintFn(ctx context.Context, minter *types.Minter, epochId string, epochNumber int64) error {
+ if k.mintFn == nil {
+ return errors.New("mintFn is not set; please ensure SetMintFn is called")
+ }
return k.mintFn(ctx, k.Environment, minter, epochId, epochNumber)
} Committable suggestion
Suggested change
|
||||||||||||||||||||
|
||||||||||||||||||||
// DefaultMintFn returns a default mint function. It requires the Staking module and the mint keeper. | ||||||||||||||||||||
// The default Mintfn has a requirement on staking as it uses bond to calculate inflation. | ||||||||||||||||||||
func DefaultMintFn(ic types.InflationCalculationFn, staking types.StakingKeeper, k *Keeper) types.MintFn { | ||||||||||||||||||||
return func(ctx context.Context, env appmodule.Environment, minter *types.Minter, epochId string, epochNumber int64) error { | ||||||||||||||||||||
// the default mint function is called every block, so we only check if epochId is "block" which is | ||||||||||||||||||||
// a special value to indicate that this is not an epoch minting, but a regular block minting. | ||||||||||||||||||||
if epochId != "block" { | ||||||||||||||||||||
return nil | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
stakingTokenSupply, err := k.StakingTokenSupply(ctx) | ||||||||||||||||||||
stakingTokenSupply, err := staking.StakingTokenSupply(ctx) | ||||||||||||||||||||
if err != nil { | ||||||||||||||||||||
return err | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
bondedRatio, err := k.BondedRatio(ctx) | ||||||||||||||||||||
bondedRatio, err := staking.BondedRatio(ctx) | ||||||||||||||||||||
if err != nil { | ||||||||||||||||||||
return err | ||||||||||||||||||||
} | ||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve format and detail of the changelog entry.
This entry lacks the standard PR number reference and could benefit from more detail.
Consider the following improvements:
Suggested revision:
Replace
#XXXXX
with the actual PR number for this change.Committable suggestion