test(sims): Add sims function for token factory module #1802
Conversation
WalkthroughThis update integrates the Changes
Related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
GNaD13
changed the title
![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: 4
Configuration used: CodeRabbit UI
Files selected for processing (6)
- app/keepers.go (1 hunks)
- x/tokenfactory/module.go (2 hunks)
- x/tokenfactory/module_test.go (1 hunks)
- x/tokenfactory/simulation/operations.go (1 hunks)
- x/tokenfactory/simulation/proposals.go (1 hunks)
- x/tokenfactory/types/tx_msgs.go (7 hunks)
Additional comments: 10
x/tokenfactory/simulation/proposals.go (1)
- 31-43: The function
SimulateMsgUpdateParamsgenerates a randomMsgUpdateModuleParamsmessage for simulation purposes. It uses the default governance module account address as the authority and sets a random value forDenomCreationGasConsume. This approach is effective for simulating various scenarios with different gas consumption values. However, it's important to ensure that the range of random values generated forDenomCreationGasConsumealigns with realistic and meaningful test scenarios.x/tokenfactory/module_test.go (1)
- 26-26: The addition of
bapp.BankKeeperas a parameter toNewAppModulein the test setup aligns with the changes made to theAppModuleconstructor to accommodate the newBankKeeperdependency. This ensures that the module is properly initialized with all necessary dependencies for the tests to run correctly. It's a good practice to update test setups in accordance with changes to the codebase to maintain test accuracy and relevance.x/tokenfactory/module.go (2)
- 118-131: The addition of the
bkfield of typetypes.BankKeeperto theAppModulestruct and its inclusion in theNewAppModulefunction signature are necessary changes to integrate the bank keeper into the token factory module. This allows the module to interact with the bank keeper, facilitating operations involving token and financial transactions. It's important to ensure that all references and usages of theAppModulethroughout the codebase are updated to include the newBankKeeperdependency.- 183-201: The addition of simulation functions (
GenerateGenesisState,ProposalMsgs,RegisterStoreDecoder, andWeightedOperations) to theAppModulestruct enhances the module's capabilities by introducing simulation functionalities. These changes are aligned with the PR's objectives to integrate simulation operations and functionalities into the token factory module. It's crucial to ensure that these simulation functions are properly tested to validate their behavior and effectiveness in simulating real-world scenarios.x/tokenfactory/types/tx_msgs.go (3)
- 9-15: The introduction of constants for message types (
MsgCreateDenomType,MsgChangeAdminType,MsgMintType,MsgBurnType,MsgSetDenomMetadataType) is a good practice for maintaining consistency and readability in the code. Using these constants in message type retrieval functions helps avoid hard-coded strings, reducing the potential for typos and making the code easier to maintain.- 143-161: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [110-157]
The type assertions for
MsgMintto ensure it implements bothsdk.Msgandlegacytx.LegacyMsginterfaces are crucial for ensuring compatibility and proper handling of the message within the Cosmos SDK framework. Similarly, the updates to the routing and type functions to use the newly introduced constants improve code maintainability and readability. It's important to ensure that all message types within the module follow this pattern for consistency.
- 202-213: The updates made to the
MsgBurnmessage, including the type assertion and updates to routing and type functions, follow the same pattern asMsgMint. These changes ensure thatMsgBurnis properly integrated within the Cosmos SDK framework and that the code remains consistent and maintainable. As withMsgMint, it's important to apply these practices to all message types within the module.x/tokenfactory/simulation/operations.go (2)
- 48-77: The
BuildOperationInputfunction constructs anOperationInputobject. However, theCdcfield of theOperationInputstruct is always set tonil. This might be intentional, but if the codec is required for serialization or deserialization within the simulation operations, this could lead to runtime errors. Please verify if the codec is needed and, if so, provide it appropriately.- 397-407: The
SimulateMsgSetDenomMetadatafunction generates random metadata for a denomination, including description, name, and symbol. While using random data is typical in simulations, ensure that the generated strings meet any expected format or validation rules that might apply in a real-world scenario. This is to ensure the simulation accurately reflects possible outcomes.app/keepers.go (1)
- 617-617: The modification to pass
app.BankKeeperas an argument totokenfactory.NewAppModuleis consistent with the PR's objective to enhance the token factory module by integrating simulation operations and functionalities. This change suggests an increased interaction between the token factory module and the bank keeper, likely to facilitate operations involving token and financial transactions. Ensure that theBankKeeperis properly initialized before this call to avoid any null pointer exceptions or initialization issues. Overall, this change appears to be logically sound and correctly implemented.
| // Get sims account | ||
| simAccount, _ := simtypes.RandomAcc(r, accs) | ||
|
|
||
| // TODO: Check if sims account enough create fee when CreateDenom Msg charge |
There was a problem hiding this comment.
The TODO comment at line 201 indicates a pending task to check if the simulation account has enough create fee when CreateDenom message charges a fee. It's important to address TODOs before merging to ensure that all intended functionality is complete and tested. If this check is crucial for the simulation's accuracy, it should be implemented.
Would you like assistance in implementing this check or should it be tracked via a GitHub issue?
| // Check if admin account balance = 0 | ||
| accountBalance := bk.GetBalance(ctx, adminAccount.Address, denom) | ||
| if accountBalance.Amount.LTE(sdk.ZeroInt()) { | ||
| return simtypes.NoOpMsg(types.ModuleName, types.MsgBurn{}.Type(), "sim account have no balance"), nil, nil | ||
| } |
There was a problem hiding this comment.
In the SimulateMsgBurn function, there's a check to ensure the admin account has a balance before attempting to burn tokens. This is a good practice for preventing simulation errors. However, it might be beneficial to log or return more detailed information about why the operation cannot proceed if the balance is zero, especially for debugging purposes.
Consider enhancing the feedback for this scenario to aid in debugging and clarity.
| const ( | ||
| DefaultWeightMsgUpdateModuleParams int = 100 | ||
|
|
||
| OpWeightMsgUpdateModuleParams = "op_weight_msg_update_module_params" //nolint:gosec |
There was a problem hiding this comment.
The use of nolint:gosec to suppress the Go security linter for the OpWeightMsgUpdateModuleParams constant might be necessary due to the linter flagging it as a potential security issue. However, it's good practice to add a comment explaining why this suppression is needed to ensure future maintainers understand the reasoning.
- OpWeightMsgUpdateModuleParams = "op_weight_msg_update_module_params" //nolint:gosec
+ OpWeightMsgUpdateModuleParams = "op_weight_msg_update_module_params" //nolint:gosec // Explanation for why this suppression is necessary.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const ( | |
| DefaultWeightMsgUpdateModuleParams int = 100 | |
| OpWeightMsgUpdateModuleParams = "op_weight_msg_update_module_params" //nolint:gosec | |
| const ( | |
| DefaultWeightMsgUpdateModuleParams int = 100 | |
| OpWeightMsgUpdateModuleParams = "op_weight_msg_update_module_params" //nolint:gosec // Explanation for why this suppression is necessary. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/tokenfactory/simulation/operations.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/tokenfactory/simulation/operations.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Instead of adding the BankKeeper twice, you should use the one on the TokenFactoryKeeper.
// Keeper of this module maintains collections of feeshares for contracts
// registered to receive Nibiru Chain gas fees.
type Keeper struct {
storeKey storetypes.StoreKey
cdc codec.BinaryCodec
Store StoreAPI
// interfaces with other modules
bankKeeper tftypes.BankKeeper // 👈👈👈👈👈👈👈👈👈👈👈👈
accountKeeper tftypes.AccountKeeper
communityPoolKeeper tftypes.CommunityPoolKeeper
// the address capable of executing a MsgUpdateParams message. Typically,
// this should be the x/gov module account.
authority string
}The change needed here for sims is to make this the bank keeper a public field
Unique-Divine
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/tokenfactory/simulation/operations.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/tokenfactory/simulation/operations.go
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1802 +/- ##
==========================================
+ Coverage 73.07% 74.02% +0.94%
==========================================
Files 196 196
Lines 15564 12713 -2851
==========================================
- Hits 11374 9411 -1963
+ Misses 3526 2637 -889
- Partials 664 665 +1
|
Hi @Unique-Divine, thanks for your reviewed. I've updated the code |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- x/tokenfactory/keeper/grpc_query.go (1 hunks)
- x/tokenfactory/keeper/keeper.go (2 hunks)
- x/tokenfactory/keeper/msg_server.go (4 hunks)
- x/tokenfactory/module.go (1 hunks)
- x/tokenfactory/simulation/operations.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/tokenfactory/module.go
- x/tokenfactory/simulation/operations.go
Additional comments: 10
x/tokenfactory/keeper/keeper.go (2)
- 26-26: The renaming of
bankKeepertoBankKeeperimproves consistency and clarity in the codebase. Ensure all references to this field within the package are updated accordingly.- 64-64: The assignment of the
bkparameter to theBankKeeperfield in theNewKeeperfunction is correctly implemented. Verify the usage ofNewKeeperfunction across the codebase to ensure compatibility.x/tokenfactory/keeper/grpc_query.go (1)
- 67-67: Updating the reference from
k.bankKeepertok.BankKeeperin theQueryDenomInfofunction aligns with the updated field name in theKeeperstruct. Ensure to test theQueryDenomInfofunction to verify its correct behavior post-update.x/tokenfactory/keeper/msg_server.go (7)
- 153-153: Updating the reference to
BankKeeperin themintfunction aligns with the updated field name in theKeeperstruct. Ensure to thoroughly test themintfunction to verify its correct behavior post-update.- 163-163: The reference to
BankKeeper.BlockedAddrin themintfunction correctly uses the updated field name. Ensure to test this functionality to confirm it behaves as expected after the update.- 168-168: The use of
BankKeeper.SendCoinsFromModuleToAccountin themintfunction is correctly updated. Testing this functionality is crucial to ensure it operates correctly post-update.- 228-228: The reference to
BankKeeper.BlockedAddrin theburnfunction correctly uses the updated field name. It's important to test this functionality to confirm its correct behavior after the update.- 234-234: The use of
BankKeeper.SendCoinsFromAccountToModulein theburnfunction is correctly updated. Testing this functionality is crucial to ensure it operates correctly post-update.- 240-240: The reference to
BankKeeper.BurnCoinsin theburnfunction correctly uses the updated field name. Ensure to test this functionality to confirm its correct behavior after the update.- 268-268: The use of
BankKeeper.SetDenomMetaDatain theSetDenomMetadatafunction is correctly updated. Testing this functionality is crucial to ensure it operates correctly post-update.
Purpose / Abstract
Summary by CodeRabbit
New Features
Refactor
AppModulestructure to include a newBankKeeperfield and adjusted initialization to accommodate this change.Tests
BankKeeperand new simulation operations.