tests(integration): port x/bank tests to server/v2 app #21912
Conversation
kocubinski
requested review from
a team,
cool-develope,
julienrbrt,
testinginprod and
hieuvubk
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (13)
codec/depinject.go (1)
19-24: LGTM! Consider adding a comment for clarity.The new
DefaultProvidersvariable is well-structured and follows Go conventions. It effectively aggregates multiple provider functions, which can simplify dependency setup elsewhere in the codebase.Consider adding a brief comment above the
DefaultProvidersdeclaration to explain its purpose and usage. For example:// DefaultProviders aggregates the default set of dependency injection providers // for codec-related functionality. var DefaultProviders = depinject.Provide( // ... (existing code) )This addition would enhance code readability and provide context for other developers.
runtime/v2/builder.go (4)
24-24: Consider documenting the impact of removed fields.The
configandstoreOptionsfields have been removed from theAppBuilderstruct. While this simplifies the struct, it may affect how configuration and store options are managed elsewhere in the application. Consider adding documentation or comments to explain how these aspects are now handled, ensuring that developers understand the new approach.
96-99: LGTM! Consider using errors.New for consistency.The addition of the
a.app.dbcheck is a good safeguard against potential null pointer dereferences. It ensures that the database is properly set before proceeding with the build process.For consistency with Go's error handling conventions, consider using
errors.Newinstead offmt.Errorffor this simple error message:if a.app.db == nil { return nil, errors.New("app.db is not set, it is required to build the app") }
Line range hint
1-231: Document the removal of AppBuilderWithStoreOptions.The
AppBuilderWithStoreOptionsfunction has been removed from this file. This change suggests that the ability to set store options externally has been deprecated. To ensure a smooth transition:
- Update any documentation or guides that reference this function.
- If there's a new recommended way to set store options, document it clearly.
- Consider adding a comment in the code explaining why this function was removed and what (if anything) replaces its functionality.
Line range hint
1-231: Consider updating documentation to reflect architectural changes.The changes in this file significantly simplify the
AppBuilderstruct and its associated methods. While this streamlining is generally positive, it may impact how developers interact with and configure the application. To ensure a smooth transition:
- Update any existing documentation that references the removed fields (
config,storeOptions) or theAppBuilderWithStoreOptionsfunction.- Consider adding inline comments or updating the package documentation to explain the new architecture and how configurations are now handled.
- If there are new recommended patterns for setting up the application or managing configurations, document these clearly for developers who may be updating their code to work with these changes.
These documentation updates will help maintain the project's ease of use and prevent confusion for both new and existing users of the SDK.
go.mod (1)
187-187: New replacement directive forcosmossdk.io/coremoduleThe addition of
cosmossdk.io/core => ./corealigns with the PR objectives, particularly the elevation of the store building process into a dependency injection Provider. This local replacement facilitates easier development and testing of changes in the core module.However, please consider the following:
This change may have implications for dependency management. Ensure that all developers working on the project are aware of this local replacement to maintain consistency across development environments.
It's recommended to document this change and its purpose, perhaps in a README or CONTRIBUTING file, to provide context for other developers and to ensure it's not accidentally removed in future updates.
Consider adding a TODO comment to remove or update this replacement once the changes in the core module are stable and released.
tests/integration/v2/services.go (1)
15-17: Consider using pointer receiver forcometServiceImplThe method
CometInfohas a value receiver oncometServiceImpl, which, while currently an empty struct, may benefit from a pointer receiver for consistency and future-proofing.Modify the method receiver:
-func (c cometServiceImpl) CometInfo(context.Context) comet.Info { +func (c *cometServiceImpl) CometInfo(context.Context) comet.Info {tests/integration/v2/genesis.go (1)
135-138: UnusedGenesisAccountstructThe
GenesisAccountstruct is defined but not used within this file. If it's not intended for future use, consider removing it to keep the codebase clean.runtime/v2/store.go (1)
67-70: Provide more detailed documentation forStoreBuilderThe current comment for
StoreBuilderis brief. Consider elaborating on howStoreBuilderis used to build astore/v2 RootStoreand how it satisfies theStoreinterface to improve code clarity and maintainability.tests/integration/v2/app.go (3)
295-295: Fix grammatical error in function commentThe comment for the
StateLatestContextmethod contains a grammatical error. It currently reads "creates returns," which should be "creates and returns" or simply "returns."Apply this diff to fix the comment:
-// StateLatestContext creates returns a new context from context.Background() with the latest state. +// StateLatestContext creates and returns a new context from context.Background() with the latest state.
376-377: Unnecessary error check afterGetTxcallIn lines 376-377,
require.NoError(t, err)is called immediately afterbuiltTx := txBuilder.GetTx(), butGetTx()does not return an error, anderrhas not been updated since the last error check. This redundant error check could be misleading.Consider removing the unnecessary error check to improve code clarity.
Apply this diff:
builtTx := txBuilder.GetTx() -require.NoError(t, err)
366-366: Address the TODO comment: "todo why fetch twice?"There is a TODO comment on line 366 indicating a potential inefficiency in the code. Fetching the transaction twice may be unnecessary and could be optimized.
Would you like assistance in resolving this TODO? I can help investigate why the transaction is fetched twice and suggest possible optimizations. Let me know if you'd like me to open a new GitHub issue for this task.
tests/integration/v2/bank/app_test.go (1)
133-133: Address the TODO comment regarding auto-advancing heightAt line 133, there is a TODO comment indicating the need to determine how to auto-advance the block height with the app v2 interface. Addressing this will improve the completeness of the test suite.
Do you want me to help implement the solution for auto-advancing the height with the app v2 interface or open a GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
🔇 Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
- codec/depinject.go (1 hunks)
- go.mod (1 hunks)
- runtime/v2/builder.go (2 hunks)
- runtime/v2/module.go (9 hunks)
- runtime/v2/store.go (2 hunks)
- server/v2/stf/core_header_service.go (0 hunks)
- simapp/go.mod (1 hunks)
- simapp/v2/app_di.go (3 hunks)
- store/v2/db/db.go (0 hunks)
- tests/go.mod (5 hunks)
- tests/integration/v2/app.go (1 hunks)
- tests/integration/v2/bank/app_test.go (1 hunks)
- tests/integration/v2/genesis.go (1 hunks)
- tests/integration/v2/services.go (1 hunks)
- testutil/configurator/configurator.go (2 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- server/v2/stf/core_header_service.go
- store/v2/db/db.go
🧰 Additional context used
📓 Path-based instructions (11)
codec/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/go.mod (1)
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/v2/app.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/v2/bank/app_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/v2/genesis.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/v2/services.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"testutil/configurator/configurator.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments not posted (19)
simapp/v2/app_di.go (2)
149-149: Verify proper configuration injection for appBuilder.Build()The removal of arguments from
appBuilder.Build()is consistent with the new approach of injecting configurations throughappConfig. However, it's crucial to ensure that all necessary configurations, previously passed asbuilderOpts, are now properly handled through the dependency injection system.To verify that all required configurations are correctly injected, review the
appBuilderimplementation and run the following command to check for any remaining direct uses of store options:#!/bin/bash # Search for direct uses of store options in the codebase rg 'store\.Options' --type goEnsure that the results don't include any cases where store options should now be handled through dependency injection.
129-136: 🛠️ Refactor suggestionImproved modularity in store options handling
The new approach to handling store options improves modularity by leveraging the dependency injection system. This change aligns well with the overall architecture and makes the configuration more consistent.
However, there are a couple of points to consider:
Error handling: The current implementation panics on unmarshal error. Consider using a more graceful error handling approach, such as logging the error and using default options, or returning an error from the
NewSimAppfunction.Removal of
builderOpts: Ensure that the removal ofbuilderOptsdoesn't impact other parts of the codebase that might have expected these options to be passed directly toappBuilder.Build(). Verify that all necessary configurations are properly injected through theappConfig.Consider refactoring the error handling to be more robust:
if sub := viper.Sub("store.options"); sub != nil { storeOptions := &root.Options{} err := sub.Unmarshal(storeOptions) if err != nil { - panic(err) + logger.Error("Failed to unmarshal store options", "error", err) + // Consider using default options or returning an error + // return nil, fmt.Errorf("failed to unmarshal store options: %w", err) } appConfig = depinject.Configs(appConfig, depinject.Supply(storeOptions)) }To ensure the removal of
builderOptsdoesn't cause issues elsewhere, run the following command:✅ Verification successful
Removal of
builderOptsVerifiedThe search for any remaining references to
builderOptsin Go files returned no results. This indicates thatbuilderOptshas been successfully removed from the codebase and should not impact other parts of the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to builderOpts in the codebase rg 'builderOpts' --type goLength of output: 223
Script:
#!/bin/bash # Correctly search for any remaining references to builderOpts in Go files rg 'builderOpts' --type goLength of output: 26
simapp/go.mod (1)
246-246: LGTM: Core module replacement added correctly.The addition of
cosmossdk.io/core => ../coreto the replace directives is appropriate and aligns with the PR objectives. This change ensures that the simapp uses the local version of the core package, which is crucial for integration testing of recent changes, particularly the elevated store building process mentioned in the PR summary.tests/go.mod (4)
71-74: New indirect dependencies are appropriate for server/v2 architecture.The addition of indirect dependencies
cosmossdk.io/errors/v2,cosmossdk.io/server/v2/appmanager, andcosmossdk.io/store/v2is consistent with the migration to the server/v2 architecture. These are likely transitive dependencies brought in by the new direct dependencies.
246-251: New local replacements added for development.New replacements have been added for
cosmossdk.io/runtime/v2,cosmossdk.io/server/v2/appmanager,cosmossdk.io/server/v2/stf, andcosmossdk.io/store/v2, pointing to local paths. This is appropriate for development and testing purposes.Remember to remove these local replacements before merging to the main branch, unless they are intended for long-term use in the tests.
171-171: Dependency versions have been updated.Several dependencies have been updated to newer versions, notably
github.com/mattn/go-sqlite3to v1.14.22. While keeping dependencies up-to-date is good practice, it's important to ensure these updates don't introduce breaking changes.Please run the integration tests to confirm that the updated dependencies don't cause any issues:
#!/bin/bash # Run integration tests go test ./tests/... -tags=integrationtests
36-37: New dependencies added align with PR objectives.The addition of
cosmossdk.io/runtime/v2,cosmossdk.io/server/v2/stf, andgitlab.com/yawning/secp256k1-voialigns with the PR's goal of porting tests to the server/v2 architecture. These dependencies are appropriate for the changes being made.Please confirm that all these new dependencies are necessary for the integration tests. Run the following command to check for any unused direct dependencies:
Also applies to: 57-57
runtime/v2/store.go (1)
97-97: Confirm the inclusion of"stf"inStoreKeysIn line 97,
"stf"is appended tostoreKeys:StoreKeys: append(storeKeys, "stf"),Please verify that adding
"stf"tostoreKeysis intentional and that it corresponds to a valid store key required for your application. Unintended store keys may lead to unexpected behavior.runtime/v2/module.go (10)
19-19: Import statement added correctlyThe addition of the
eventpackage import aligns with the module's functionality and follows proper import grouping conventions.
30-30: Importedrootstorepackage with aliasImporting
rootstorewith the alias at line 30 is appropriate and enhances code readability.
45-45: Corrected parameter name toregistrarThe parameter name in the
RegisterServicesfunction has been corrected fromregistartoregistrar, improving clarity and consistency.
51-51: Registered AutoCLI Query ServerRegistering
autoCliQueryServicewithautocliv1.RegisterQueryServerintegrates the AutoCLI query service correctly.
57-57: Registered Reflection Service ServerThe registration of
reflectionSvcwithreflectionv1.RegisterReflectionServiceServerappropriately enables reflection services over gRPC.
102-102: IncludedProvideStoreBuilderin module registrationAdding
ProvideStoreBuilderto theappconfig.Providelist ensures the store builder is properly injected into the application configuration.
200-200: AddedeventServiceparameter toProvideEnvironmentIncluding
eventService event.Serviceas a parameter enhances the environment setup by providing event handling capabilities to modules.
232-232: AssignedEventServiceinEnvironmentstructAssigning
EventService: eventServiceensures that modules have access to event services within the application environment.
277-277: InitializedeventServiceinDefaultServiceBindingsCreating an instance of
eventServiceusingstf.NewEventService()correctly sets up the event service for dependency injection.
283-283: SuppliedeventServicein dependency injectionIncluding
eventServicein thedepinject.Supplyensures it is properly injected into the application's dependency graph.tests/integration/v2/app.go (1)
225-231:⚠️ Potential issueVerify existence of fields in
DefaultConsensusParamsThe code attempts to access
DefaultConsensusParams.Abci,DefaultConsensusParams.Synchrony, andDefaultConsensusParams.Feature, but these fields are not defined inDefaultConsensusParams. This may lead to runtime errors due to nil pointer dereferences.Please verify whether these fields exist in
DefaultConsensusParams, or update theDefaultConsensusParamsto include these fields.Run the following script to check for the existence of these fields:
tests/integration/v2/services.go
Outdated
| state, err := iCtx.state.GetWriter(s.actor) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Handle errors instead of panicking in OpenKVStore method
Panicking on error can lead to unexpected crashes. It's recommended to handle the error appropriately, possibly by returning a default store or logging the error.
Since the method does not return an error, consider handling the error internally or logging it appropriately.
tests/integration/v2/services.go
Outdated
| func (s storeService) OpenKVStore(ctx context.Context) corestore.KVStore { | ||
| iCtx, ok := ctx.Value(contextKey).(integrationContext) | ||
| if !ok { | ||
| return s.executionService.OpenKVStore(ctx) | ||
| } | ||
|
|
||
| state, err := iCtx.state.GetWriter(s.actor) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return state | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use pointer receiver for storeService methods
The method OpenKVStore has a value receiver, which may cause unintended copying of the storeService struct. Since storeService contains fields and to avoid unnecessary copying, it's recommended to use a pointer receiver.
Consider changing the method receiver to a pointer:
-func (s storeService) OpenKVStore(ctx context.Context) corestore.KVStore {
+func (s *storeService) OpenKVStore(ctx context.Context) corestore.KVStore {📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s storeService) OpenKVStore(ctx context.Context) corestore.KVStore { | |
| iCtx, ok := ctx.Value(contextKey).(integrationContext) | |
| if !ok { | |
| return s.executionService.OpenKVStore(ctx) | |
| } | |
| state, err := iCtx.state.GetWriter(s.actor) | |
| if err != nil { | |
| panic(err) | |
| } | |
| return state | |
| } | |
| func (s *storeService) OpenKVStore(ctx context.Context) corestore.KVStore { | |
| iCtx, ok := ctx.Value(contextKey).(integrationContext) | |
| if !ok { | |
| return s.executionService.OpenKVStore(ctx) | |
| } | |
| state, err := iCtx.state.GetWriter(s.actor) | |
| if err != nil { | |
| panic(err) | |
| } | |
| return state | |
| } |
| default: | ||
| panic(fmt.Sprintf("unknown key: %s", key)) | ||
| } |
There was a problem hiding this comment.
Avoid using panic for handling unknown configuration keys
Using panic in the default case can cause the application to crash when an unknown key is provided. It's better to handle this case gracefully by returning a default value or an error.
Consider modifying the code as follows:
default:
- panic(fmt.Sprintf("unknown key: %s", key))
+ return ""Committable suggestion was skipped due to low confidence.
| delegations = append( | ||
| delegations, | ||
| stakingtypes.NewDelegation( | ||
| genAccs[0].GetAddress().String(), |
There was a problem hiding this comment.
Potential index out of range when accessing genAccs[0]
If genAccs is empty, accessing genAccs[0] will cause a panic due to an index out of range error. Please ensure that genAccs is not empty before accessing the first element, or add a check to handle empty slices.
| totalSupply := sdk.NewCoins() | ||
| for _, b := range balances { | ||
| // add genesis acc tokens to total supply | ||
| totalSupply = totalSupply.Add(b.Coins...) | ||
| } | ||
|
|
||
| for range delegations { | ||
| // add delegated tokens to total supply | ||
| totalSupply = totalSupply.Add( | ||
| sdk.NewCoin(sdk.DefaultBondDenom, bondAmt), | ||
| ) | ||
| } | ||
|
|
||
| // add bonded amount to bonded pool module account | ||
| balances = append(balances, banktypes.Balance{ | ||
| Address: authtypes.NewModuleAddress(stakingtypes.BondedPoolName). | ||
| String(), | ||
| Coins: sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, bondAmt)}, | ||
| }) |
There was a problem hiding this comment.
Incorrect total supply and bonded pool balance calculations
The calculation of totalSupply for bonded tokens might be incorrect. In the loop over delegations (lines 94-99), bondAmt is added to totalSupply for each delegation. However, since bondAmt represents the bonded amount per validator, the total bonded amount should be bondAmt multiplied by the number of delegations.
Similarly, when adding to the bonded pool module account balance (lines 102-106), only bondAmt is added, whereas it should be the total bonded amount to reflect all delegations.
To fix this, calculate the total bonded amount and use it when updating totalSupply and the bonded pool balance. Here's a suggested change:
+ totalBondedAmt := bondAmt.Mul(sdk.NewInt(int64(len(delegations))))
// Update totalSupply by adding the total bonded amount
- for range delegations {
- // add delegated tokens to total supply
- totalSupply = totalSupply.Add(
- sdk.NewCoin(sdk.DefaultBondDenom, bondAmt),
- )
- }
+ // Add total bonded tokens to total supply
+ totalSupply = totalSupply.Add(
+ sdk.NewCoin(sdk.DefaultBondDenom, totalBondedAmt),
+ )
// Update the bonded pool module account balance with the total bonded amount
- balances = append(balances, banktypes.Balance{
- Address: authtypes.NewModuleAddress(stakingtypes.BondedPoolName).String(),
- Coins: sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, bondAmt)},
- })
+ balances = append(balances, banktypes.Balance{
+ Address: authtypes.NewModuleAddress(stakingtypes.BondedPoolName).String(),
+ Coins: sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, totalBondedAmt)},
+ })📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| totalSupply := sdk.NewCoins() | |
| for _, b := range balances { | |
| // add genesis acc tokens to total supply | |
| totalSupply = totalSupply.Add(b.Coins...) | |
| } | |
| for range delegations { | |
| // add delegated tokens to total supply | |
| totalSupply = totalSupply.Add( | |
| sdk.NewCoin(sdk.DefaultBondDenom, bondAmt), | |
| ) | |
| } | |
| // add bonded amount to bonded pool module account | |
| balances = append(balances, banktypes.Balance{ | |
| Address: authtypes.NewModuleAddress(stakingtypes.BondedPoolName). | |
| String(), | |
| Coins: sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, bondAmt)}, | |
| }) | |
| totalSupply := sdk.NewCoins() | |
| for _, b := range balances { | |
| // add genesis acc tokens to total supply | |
| totalSupply = totalSupply.Add(b.Coins...) | |
| } | |
| totalBondedAmt := bondAmt.Mul(sdk.NewInt(int64(len(delegations)))) | |
| // Add total bonded tokens to total supply | |
| totalSupply = totalSupply.Add( | |
| sdk.NewCoin(sdk.DefaultBondDenom, totalBondedAmt), | |
| ) | |
| // Update the bonded pool module account balance with the total bonded amount | |
| balances = append(balances, banktypes.Balance{ | |
| Address: authtypes.NewModuleAddress(stakingtypes.BondedPoolName).String(), | |
| Coins: sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, totalBondedAmt)}, | |
| }) |
| // StartupConfig defines the startup configuration new a test application. | ||
| // | ||
| // ValidatorSet defines a custom validator set to be validating the app. | ||
| // BaseAppOption defines the additional operations that must be run on baseapp before app start. | ||
| // AtGenesis defines if the app started should already have produced block or not. |
There was a problem hiding this comment.
Correct struct field comments for 'StartupConfig'
The comments for the fields in the StartupConfig struct contain inconsistencies and grammatical errors. Specifically, the field AppOption is incorrectly referred to as BaseAppOption, and some comments are unclear.
Apply this diff to improve the comments:
-// StartupConfig defines the startup configuration new a test application.
+// StartupConfig defines the startup configuration for a new test application.
//
-// ValidatorSet defines a custom validator set to be validating the app.
-// BaseAppOption defines the additional operations that must be run on baseapp before app start.
-// AtGenesis defines if the app started should already have produced block or not.
+// ValidatorSet defines a custom validator set for validating the app.
+// AppOption defines additional operations to run on the app before starting.
+// AtGenesis indicates whether the app should start at genesis or not.📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // StartupConfig defines the startup configuration new a test application. | |
| // | |
| // ValidatorSet defines a custom validator set to be validating the app. | |
| // BaseAppOption defines the additional operations that must be run on baseapp before app start. | |
| // AtGenesis defines if the app started should already have produced block or not. | |
| // StartupConfig defines the startup configuration for a new test application. | |
| // | |
| // ValidatorSet defines a custom validator set for validating the app. | |
| // AppOption defines additional operations to run on the app before starting. | |
| // AtGenesis indicates whether the app should start at genesis or not. |
| &genesisTxCodec{txConfigOptions}, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed init genesiss: %w", err) |
There was a problem hiding this comment.
Typo in error message: "failed init genesiss"
There is a typographical error in the error message on line 259. The word "genesiss" should be "genesis".
Apply this diff to correct the typo:
- return nil, fmt.Errorf("failed init genesiss: %w", err)
+ return nil, fmt.Errorf("failed to initialize genesis: %w", err)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return nil, fmt.Errorf("failed init genesiss: %w", err) | |
| return nil, fmt.Errorf("failed to initialize genesis: %w", err) |
There was a problem hiding this comment.
@julienrbrt Great! I'm glad the typo has been corrected.
((^w^)/
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| func NewAppV2Config(opts ...ModuleOption) depinject.Config { | ||
| cfg := defaultConfig() | ||
| for _, opt := range opts { | ||
| opt(cfg) | ||
| } | ||
|
|
||
| preBlockers := make([]string, 0) | ||
| beginBlockers := make([]string, 0) | ||
| endBlockers := make([]string, 0) | ||
| initGenesis := make([]string, 0) | ||
| overrides := make([]*runtimev2.StoreKeyConfig, 0) | ||
|
|
||
| for _, s := range cfg.PreBlockersOrder { | ||
| if _, ok := cfg.ModuleConfigs[s]; ok { | ||
| preBlockers = append(preBlockers, s) | ||
| } | ||
| } | ||
|
|
||
| for _, s := range cfg.BeginBlockersOrder { | ||
| if _, ok := cfg.ModuleConfigs[s]; ok { | ||
| beginBlockers = append(beginBlockers, s) | ||
| } | ||
| } | ||
|
|
||
| for _, s := range cfg.EndBlockersOrder { | ||
| if _, ok := cfg.ModuleConfigs[s]; ok { | ||
| endBlockers = append(endBlockers, s) | ||
| } | ||
| } | ||
|
|
||
| for _, s := range cfg.InitGenesisOrder { | ||
| if _, ok := cfg.ModuleConfigs[s]; ok { | ||
| initGenesis = append(initGenesis, s) | ||
| } | ||
| } | ||
|
|
||
| if _, ok := cfg.ModuleConfigs[testutil.AuthModuleName]; ok { | ||
| overrides = append(overrides, &runtimev2.StoreKeyConfig{ModuleName: testutil.AuthModuleName, KvStoreKey: "acc"}) | ||
| } | ||
|
|
||
| runtimeConfig := &runtimev2.Module{ | ||
| AppName: "TestApp", | ||
| PreBlockers: preBlockers, | ||
| BeginBlockers: beginBlockers, | ||
| EndBlockers: endBlockers, | ||
| OverrideStoreKeys: overrides, | ||
| GasConfig: &runtimev2.GasConfig{ | ||
| ValidateTxGasLimit: 100_000, | ||
| QueryGasLimit: 100_000, | ||
| SimulationGasLimit: 100_000, | ||
| }, | ||
| } | ||
| if cfg.setInitGenesis { | ||
| runtimeConfig.InitGenesis = initGenesis | ||
| } | ||
|
|
||
| modules := []*appv1alpha1.ModuleConfig{{ | ||
| Name: "runtime", | ||
| Config: appconfig.WrapAny(runtimeConfig), | ||
| }} | ||
|
|
||
| for _, m := range cfg.ModuleConfigs { | ||
| modules = append(modules, m) | ||
| } | ||
|
|
||
| return appconfig.Compose(&appv1alpha1.Config{Modules: modules}) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to eliminate code duplication between NewAppConfig and NewAppV2Config
The functions NewAppConfig and NewAppV2Config share significant portions of code that are nearly identical, differing mainly in the types used (runtimev1alpha1 vs. runtimev2) and specific configurations like GasConfig. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider refactoring the shared logic into a common helper function or abstracting the differing parts.
| require.Equal(t, origAccNum, res2.GetAccountNumber()) | ||
| require.Equal(t, origSeq+1, res2.GetSequence()) | ||
|
|
||
| require.NoError(t, err) |
There was a problem hiding this comment.
Undefined variable err in TestSendNotEnoughBalance
At line 146, require.NoError(t, err) is called, but err is not defined in this scope. This will cause a compile-time error.
Consider removing this line or ensuring that err is properly defined before this point.
- require.NoError(t, err)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require.NoError(t, err) |
| ) | ||
|
|
||
| var ( | ||
| stablePrivateKey, _ = secec.NewPrivateKeyFromScalar(secp256k1_internal.NewScalarFromUint64(100)) |
There was a problem hiding this comment.
Handle the error returned by secec.NewPrivateKeyFromScalar
In line 40, the error returned by secec.NewPrivateKeyFromScalar is ignored. It's best practice in Go to handle errors to prevent unexpected issues.
Apply the following changes to handle the error:
var (
- stablePrivateKey, _ = secec.NewPrivateKeyFromScalar(secp256k1_internal.NewScalarFromUint64(100))
+ stablePrivateKey, err = secec.NewPrivateKeyFromScalar(secp256k1_internal.NewScalarFromUint64(100))
)
+require.NoError(t, err)Committable suggestion was skipped due to low confidence.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ctx, | ||
| &server.BlockRequest[stateMachineTx]{ | ||
| Height: 1, | ||
| Time: time.Now(), |
Check warning
Code scanning / CodeQL
Calling the system time Warning test
| ) server.TxResult { | ||
| t.Helper() | ||
|
|
||
| r := rand.New(rand.NewSource(time.Now().UnixNano())) |
Check warning
Code scanning / CodeQL
Calling the system time Warning test
|
|
||
| state, err := iCtx.state.GetWriter(s.actor) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning test
| for _, m := range cfg.ModuleConfigs { | ||
| modules = append(modules, m) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning test
| @@ -16,6 +16,13 @@ import ( | |||
| "github.com/cosmos/cosmos-sdk/codec/types" | |||
| ) | |||
|
|
|||
| var DefaultProviders = depinject.Provide( | |||
There was a problem hiding this comment.
Shal we use those in app_di (v1 & v2) to simplify things?
| @@ -59,6 +64,56 @@ type Store interface { | |||
| LastCommitID() (proof.CommitID, error) | |||
| } | |||
|
|
|||
| // StoreBuilder is a builder for a store/v2 RootStore satisfying the Store interface. | |||
| type StoreBuilder struct { | |||
There was a problem hiding this comment.
That's smart, I like the abstraction! Makes it more readble too.
| @@ -127,6 +126,15 @@ func NewSimApp[T transaction.Tx]( | |||
| ) | |||
| ) | |||
|
|
|||
| if sub := viper.Sub("store.options"); sub != nil { | |||
There was a problem hiding this comment.
can we add short comment on this
| &genesisTxCodec{txConfigOptions}, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed init genesiss: %w", err) |
| } | ||
|
|
||
| // Deliver delivers a block with the given transactions and returns the resulting state. | ||
| func (a *App) Deliver( |
There was a problem hiding this comment.
Should we add a helper DeliverAndCommit and remove the other two?
There was a problem hiding this comment.
So actually, the integration test framework was this:
cosmos-sdk/testutil/integration/router.go
Lines 46 to 48 in 3856e77
cosmos-sdk/tests/integration/auth/keeper/fixture_test.go
Lines 117 to 126 in 3856e77
cosmos-sdk/tests/integration/auth/keeper/msg_server_test.go
Lines 141 to 145 in 3856e77
There was a problem hiding this comment.
As discussed on Slack, RunMsg and Query will be added on this integration app.
We'll keep the setup logic from here because it is superior.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
tests/integration/v2/bank/determinisitic_test.go (2)
86-87: Initialize context within the fixture for consistencyCurrently, the context
f.ctxis initialized outside ofinitDeterministicFixture. For better encapsulation and to maintain consistency across tests, consider initializing the context within theinitDeterministicFixturefunction.Update the
initDeterministicFixturefunction to include context initialization:func initDeterministicFixture(t *testing.T) *deterministicFixture { t.Helper() // ... existing setup code ... require.NoError(t, err) require.NotNil(t, app) + ctx := app.StateLatestContext(t) + return &deterministicFixture{ app: app, bankKeeper: bankKeeper, + ctx: ctx, T: t, } }And remove the context initialization from
TestGRPCQueryBalance:func TestGRPCQueryBalance(t *testing.T) { f := initDeterministicFixture(t) - f.ctx = f.app.StateLatestContext(t) rapid.Check(t, func(rt *rapid.T) { // ... test code ... }) }
93-94: Optimize address encoding by reusing codec optionsIn the test, a new
CodecOptions{}instance is created each time to get the address codec. To optimize performance and reduce unnecessary allocations, consider defining the codec options once and reusing them.Define the codec options at the beginning of your test or within the fixture:
+addrCodec := codectestutil.CodecOptions{}.GetAddressCodec() func TestGRPCQueryBalance(t *testing.T) { f := initDeterministicFixture(t) rapid.Check(t, func(rt *rapid.T) { addr := testdata.AddressGenerator(rt).Draw(rt, "address") coin := getCoin(rt) fundAccount(f, addr, coin) - addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) + addrStr, err := addrCodec.BytesToString(addr) require.NoError(t, err) // ... remaining test code ... }) }Alternatively, incorporate the codec into the fixture for reuse across multiple tests.
testutil/testdata/grpc_query.go (1)
99-99: Add a GoDoc comment for 'DeterministicIterationsV2'Exported functions should have a GoDoc comment explaining their purpose and usage, starting with the function name, as per Go conventions and the Uber Go Style Guide.
Please add a comment for
DeterministicIterationsV2to improve code documentation and readability.Apply this diff to add the comment:
+// DeterministicIterationsV2 handles deterministic query requests for server/v2 and tests: +// 1. That the response is always the same when calling the +// grpcFn query iterCount times (defaults to 1000). +// 2. That the gas consumption of the query is consistent. When +// gasOverwrite is set to true, it also checks that this consumed +// gas value is equal to the hardcoded gasConsumed. func DeterministicIterationsV2[request, response proto.Message](tests/integration/v2/app.go (2)
288-288: Fix grammatical error inStateLatestContextcommentThe comment for the
StateLatestContextmethod contains a grammatical error. It currently reads: "StateLatestContext creates returns a new context...". This should be corrected for clarity.Apply this diff to fix the comment:
-// StateLatestContext creates returns a new context from context.Background() with the latest state. +// StateLatestContext returns a new context derived from context.Background() with the latest state.
358-359: Address the TODO comment regarding duplicate fetchesThere is a TODO comment questioning why the transaction is fetched twice. It's important to resolve TODOs to maintain code clarity and prevent potential issues.
Do you need assistance investigating the reason for fetching the transaction twice? I can help analyze the code to determine if this duplication is necessary or if it can be optimized. Let me know if you would like me to open a new GitHub issue for this.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- tests/integration/v2/app.go (1 hunks)
- tests/integration/v2/bank/app_test.go (1 hunks)
- tests/integration/v2/bank/determinisitic_test.go (1 hunks)
- testutil/testdata/grpc_query.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/v2/bank/app_test.go
🧰 Additional context used
📓 Path-based instructions (3)
tests/integration/v2/app.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/v2/bank/determinisitic_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"testutil/testdata/grpc_query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
| rapid.Check(t, func(rt *rapid.T) { | ||
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | ||
| coin := getCoin(rt) | ||
| fundAccount(f, addr, coin) | ||
|
|
||
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | ||
| require.NoError(t, err) | ||
|
|
||
| req := banktypes.NewQueryBalanceRequest(addrStr, coin.GetDenom()) | ||
|
|
||
| testdata.DeterministicIterationsV2(t, f.ctx, nil, req, f.QueryBalance, 0, true) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for bank module functionalities
While the TestGRPCQueryBalance function tests the balance query, to ensure comprehensive integration test coverage, consider porting additional tests from the x/bank module. This could include tests for:
- Transferring funds between accounts.
- Querying total supply of tokens.
- Handling edge cases like insufficient balances or invalid denominations.
Would you like assistance in identifying key tests to port or in writing additional tests to enhance coverage?
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QueryBalanceResponse), err |
There was a problem hiding this comment.
Prevent potential panic due to unchecked type assertion
In the QueryBalance method, the type assertion res.(*banktypes.QueryBalanceResponse) could cause a panic if res is not of the expected type. To enhance robustness, consider using a type assertion with an ok check to handle unexpected types gracefully.
Apply this diff to safely perform the type assertion:
func (f *deterministicFixture) QueryBalance(
ctx context.Context, req *banktypes.QueryBalanceRequest,
) (*banktypes.QueryBalanceResponse, error) {
res, err := f.app.Query(ctx, 0, req)
- return res.(*banktypes.QueryBalanceResponse), err
+ if err != nil {
+ return nil, err
+ }
+ balanceRes, ok := res.(*banktypes.QueryBalanceResponse)
+ if !ok {
+ return nil, fmt.Errorf("unexpected response type: %T", res)
+ }
+ return balanceRes, nil
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res, err := f.app.Query(ctx, 0, req) | |
| return res.(*banktypes.QueryBalanceResponse), err | |
| func (f *deterministicFixture) QueryBalance( | |
| ctx context.Context, req *banktypes.QueryBalanceRequest, | |
| ) (*banktypes.QueryBalanceResponse, error) { | |
| res, err := f.app.Query(ctx, 0, req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| balanceRes, ok := res.(*banktypes.QueryBalanceResponse) | |
| if !ok { | |
| return nil, fmt.Errorf("unexpected response type: %T", res) | |
| } | |
| return balanceRes, nil | |
| } |
| app, err := integration.NewApp(diConfig, startupConfig, &bankKeeper) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, app) |
There was a problem hiding this comment.
💡 Codebase verification
Ensure all instances of NewApp wrap errors with %w for consistent error handling.
- In
schema/testing/statesim/app.go, modify the error handling to usefmt.Errorf("module %s already initialized: %w", data.ModuleName, err)to wrap the original error.
🔗 Analysis chain
Verify error handling during app initialization
Ensure that any errors returned by integration.NewApp provide sufficient context for debugging. Proper error messages can greatly aid in diagnosing issues during test setup.
Run the following script to check the error handling in integration.NewApp:
This script searches for proper error wrapping within the NewApp function to ensure errors are informative.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for error handling in the integration.NewApp function implementation.
# Expected: The function should wrap errors with contextual information.
# Search for error wrapping patterns in the NewApp function.
rg --type go --max-depth 5 'func NewApp\(' -A 100 | rg 'fmt\.Errorf|errors\.Wrap|errors\.WithMessage'
Length of output: 800
| @@ -94,3 +95,32 @@ func DeterministicIterations[request, response proto.Message]( | |||
| assert.DeepEqual(t, res, prevRes) | |||
| } | |||
| } | |||
|
|
|||
| func DeterministicIterationsV2[request, response proto.Message]( | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming 'DeterministicIterationsV2' to a more descriptive name
The function name DeterministicIterationsV2 does not clearly convey the purpose or the difference from the original DeterministicIterations. Using version suffixes like V2 is discouraged in function names as per the Uber Go Style Guide.
Consider renaming the function to reflect its specific functionality or distinctions from the previous version, possibly highlighting that it's tailored for the server/v2 architecture.
testutil/testdata/grpc_query.go
Outdated
| } | ||
|
|
||
| for i := 0; i < iterCount; i++ { | ||
| before := consumed() |
There was a problem hiding this comment.
Avoid variable shadowing of 'before' variable
Re-declaring the before variable within the loop shadows the outer before variable declared earlier. According to the Uber Go Style Guide, shadowing is discouraged as it can lead to bugs and reduce code clarity.
Consider renaming the inner before variable or using the outer before variable to avoid shadowing.
Apply this diff to fix the variable shadowing:
for i := 0; i < iterCount; i++ {
- before := consumed()
+ loopBefore := consumed()
res, err := grpcFn(ctx, req)
- assert.Equal(t, consumed()-before, gasConsumed)
+ assert.Equal(t, consumed()-loopBefore, gasConsumed)
assert.NilError(t, err)
assert.DeepEqual(t, res, prevRes)
}Alternatively, you can reuse the before variable without redeclaring it:
for i := 0; i < iterCount; i++ {
- before := consumed()
+ before = consumed()
res, err := grpcFn(ctx, req)
assert.Equal(t, consumed()-before, gasConsumed)
assert.NilError(t, err)
assert.DeepEqual(t, res, prevRes)
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| before := consumed() | |
| loopBefore := consumed() |
| Abci: DefaultConsensusParams.Abci, | ||
| Synchrony: DefaultConsensusParams.Synchrony, | ||
| Feature: DefaultConsensusParams.Feature, | ||
| }, |
There was a problem hiding this comment.
Remove non-existent fields from DefaultConsensusParams usage
The DefaultConsensusParams struct does not have fields named Abci, Synchrony, or Feature. Attempting to access these fields will result in a compilation error.
Apply this diff to remove the invalid field references:
Authority: "consensus",
Block: DefaultConsensusParams.Block,
Evidence: DefaultConsensusParams.Evidence,
Validator: DefaultConsensusParams.Validator,
- Abci: DefaultConsensusParams.Abci,
- Synchrony: DefaultConsensusParams.Synchrony,
- Feature: DefaultConsensusParams.Feature,
},If these fields are necessary, ensure that DefaultConsensusParams is correctly defined with these fields or obtain them from the appropriate source.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Abci: DefaultConsensusParams.Abci, | |
| Synchrony: DefaultConsensusParams.Synchrony, | |
| Feature: DefaultConsensusParams.Feature, | |
| }, | |
| Authority: "consensus", | |
| Block: DefaultConsensusParams.Block, | |
| Evidence: DefaultConsensusParams.Evidence, | |
| Validator: DefaultConsensusParams.Validator, | |
| }, |
kocubinski
requested review from
facundomedica and
sontrinh16
as code owners
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (1)
server/v2/appmanager/appmanager.go (1)
138-150: Improved readability and error handling. Consider using a switch statement.The refactoring of the
Querymethod has improved both readability and error handling. The new structure is more concise and logical.Consider using a switch statement for better clarity:
var queryState corestore.ReaderMap var err error switch { case version != 0: queryState, err = a.db.StateAt(version) default: _, queryState, err = a.db.StateLatest() } if err != nil { return nil, err }This structure makes the logic even more explicit and is idiomatic Go.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (7)
- server/v2/appmanager/appmanager.go (2 hunks)
- server/v2/cometbft/abci.go (4 hunks)
- server/v2/cometbft/handlers/defaults.go (6 hunks)
- tests/integration/v2/app.go (1 hunks)
- tests/integration/v2/bank/determinisitic_test.go (1 hunks)
- tests/integration/v2/services.go (1 hunks)
- testutil/testdata/grpc_query.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/v2/app.go
- tests/integration/v2/services.go
🧰 Additional context used
📓 Path-based instructions (5)
server/v2/appmanager/appmanager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/handlers/defaults.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/v2/bank/determinisitic_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"testutil/testdata/grpc_query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (11)
testutil/testdata/grpc_query.go (3)
9-17: LGTM: New imports are correctly added and organized.The new imports are necessary for the
DeterministicIterationsV2function and follow the Uber Go Style Guide for grouping and ordering.
101-101: Consider renaming 'DeterministicIterationsV2' to a more descriptive nameThe function name
DeterministicIterationsV2still includes a version suffix, which is discouraged as per the Uber Go Style Guide. Consider renaming the function to reflect its specific functionality or distinctions from the previous version, possibly highlighting that it's tailored for the server/v2 architecture.
101-127: LGTM: Function logic is well-implementedThe
DeterministicIterationsV2function is well-structured and implements the required functionality correctly. It usesrequirefor better error handling and avoids the variable shadowing issue mentioned in a previous review. The use of generics and function parameters makes it flexible and reusable.server/v2/appmanager/appmanager.go (1)
160-162: Improved readability of method signature.The reformatting of the
QueryWithStatemethod signature enhances code readability without changing its functionality. This change is in line with good coding practices.server/v2/cometbft/abci.go (3)
192-194: Function signature formatting improved.The Query function signature has been reformatted to improve readability by splitting it across multiple lines. This change enhances code clarity without altering the function's behavior.
243-245: Function signature formatting improved.The InitChain function signature has been reformatted to improve readability by splitting it across multiple lines. This change enhances code clarity without altering the function's behavior.
561-563: Function signature formatting improved.The ExtendVote function signature has been reformatted to improve readability by splitting it across multiple lines. This change enhances code clarity without altering the function's behavior.
tests/integration/v2/bank/determinisitic_test.go (2)
1-48: LGTM: Imports and global variables are well-definedThe import statements cover all necessary packages for testing the bank module, and the global variables are appropriately defined for use in the test cases.
1-627: Overall assessment: Well-structured tests with room for enhancementThe integration tests for the bank module are comprehensive and well-structured, covering various scenarios using both property-based testing with rapid and specific test cases. The tests demonstrate a good understanding of the bank module's functionalities and aim to ensure its correctness.
However, there are several areas where the tests can be improved:
- Error handling in query methods of the
deterministicFixturestruct.- Additional assertions in various test functions to verify the correctness of returned data.
- More robust checks in property-based tests to cover edge cases.
Implementing the suggested improvements will significantly enhance the quality and reliability of these tests, providing stronger guarantees about the correctness of the bank module's implementation.
server/v2/cometbft/handlers/defaults.go (2)
43-43: Confirm that ignoring thegasreturn value fromapp.Queryis appropriateIn multiple locations (lines 43, 116, and 184-186), the
gasvalue returned fromapp.Queryis ignored using_. Please ensure that disregarding this value does not have unintended consequences and that it is acceptable in these contexts.Also applies to: 116-116, 184-186
20-22: Update all implementations ofAppManager[T].Queryto match the new method signatureThe method signature of
AppManager[T].Queryhas been updated to include an additionalgas gas.Gasreturn value. Ensure that all implementations of this interface are updated accordingly to prevent compilation errors.Run the following script to find implementations of
AppManager[T].Querythat may need updating:
| type deterministicFixture struct { | ||
| *testing.T | ||
| ctx context.Context | ||
| app *integration.App | ||
| bankKeeper bankkeeper.Keeper | ||
| } | ||
|
|
||
| func (f *deterministicFixture) QueryBalance( | ||
| ctx context.Context, req *banktypes.QueryBalanceRequest, | ||
| ) (*banktypes.QueryBalanceResponse, error) { | ||
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QueryBalanceResponse), err | ||
| } | ||
|
|
||
| func (f *deterministicFixture) QueryAllBalances( | ||
| ctx context.Context, req *banktypes.QueryAllBalancesRequest, | ||
| ) (*banktypes.QueryAllBalancesResponse, error) { | ||
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QueryAllBalancesResponse), err | ||
| } | ||
|
|
||
| func (f *deterministicFixture) QuerySpendableBalances( | ||
| ctx context.Context, req *banktypes.QuerySpendableBalancesRequest, | ||
| ) (*banktypes.QuerySpendableBalancesResponse, error) { | ||
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QuerySpendableBalancesResponse), err | ||
| } | ||
|
|
||
| func (f *deterministicFixture) QueryTotalSupply( | ||
| ctx context.Context, req *banktypes.QueryTotalSupplyRequest, | ||
| ) (*banktypes.QueryTotalSupplyResponse, error) { | ||
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QueryTotalSupplyResponse), err | ||
| } | ||
|
|
||
| func (f *deterministicFixture) QuerySupplyOf( | ||
| ctx context.Context, req *banktypes.QuerySupplyOfRequest, | ||
| ) (*banktypes.QuerySupplyOfResponse, error) { | ||
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QuerySupplyOfResponse), err | ||
| } | ||
|
|
||
| func (f *deterministicFixture) QueryParams( | ||
| ctx context.Context, req *banktypes.QueryParamsRequest, | ||
| ) (*banktypes.QueryParamsResponse, error) { | ||
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QueryParamsResponse), err | ||
| } | ||
|
|
||
| func (f *deterministicFixture) QueryDenomsMetadata( | ||
| ctx context.Context, req *banktypes.QueryDenomsMetadataRequest, | ||
| ) (*banktypes.QueryDenomsMetadataResponse, error) { | ||
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QueryDenomsMetadataResponse), err | ||
| } | ||
|
|
||
| func (f *deterministicFixture) QueryDenomMetadata( | ||
| ctx context.Context, req *banktypes.QueryDenomMetadataRequest, | ||
| ) (*banktypes.QueryDenomMetadataResponse, error) { | ||
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QueryDenomMetadataResponse), err | ||
| } | ||
|
|
||
| func (f *deterministicFixture) QuerySendEnabled( | ||
| ctx context.Context, req *banktypes.QuerySendEnabledRequest, | ||
| ) (*banktypes.QuerySendEnabledResponse, error) { | ||
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QuerySendEnabledResponse), err | ||
| } | ||
|
|
||
| func (f *deterministicFixture) QueryDenomOwners( | ||
| ctx context.Context, req *banktypes.QueryDenomOwnersRequest, | ||
| ) (*banktypes.QueryDenomOwnersResponse, error) { | ||
| res, err := f.app.Query(ctx, 0, req) | ||
| return res.(*banktypes.QueryDenomOwnersResponse), err | ||
| } |
There was a problem hiding this comment.
Enhance error handling in query methods
While the query methods are well-implemented, they could benefit from improved error handling. Currently, the methods are not checking if the type assertion is successful before returning the result.
Consider updating all query methods to include a check for the type assertion. Here's an example for the QueryBalance method:
func (f *deterministicFixture) QueryBalance(
ctx context.Context, req *banktypes.QueryBalanceRequest,
) (*banktypes.QueryBalanceResponse, error) {
res, err := f.app.Query(ctx, 0, req)
- return res.(*banktypes.QueryBalanceResponse), err
+ if err != nil {
+ return nil, err
+ }
+ balanceRes, ok := res.(*banktypes.QueryBalanceResponse)
+ if !ok {
+ return nil, fmt.Errorf("unexpected response type: %T", res)
+ }
+ return balanceRes, nil
}Apply similar changes to all other query methods in the deterministicFixture struct.
Committable suggestion was skipped due to low confidence.
| func fundAccount(f *deterministicFixture, addr sdk.AccAddress, coin ...sdk.Coin) { | ||
| err := banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, sdk.NewCoins(coin...)) | ||
| require.NoError(f.T, err) | ||
| } | ||
|
|
||
| func getCoin(rt *rapid.T) sdk.Coin { | ||
| return sdk.NewCoin( | ||
| rapid.StringMatching(denomRegex).Draw(rt, "denom"), | ||
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | ||
| ) | ||
| } | ||
|
|
||
| func initDeterministicFixture(t *testing.T) *deterministicFixture { | ||
| t.Helper() | ||
|
|
||
| ctrl := gomock.NewController(t) | ||
| acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl) | ||
| accNum := uint64(0) | ||
| acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) ( | ||
| uint64, error, | ||
| ) { | ||
| currentNum := accNum | ||
| accNum++ | ||
| return currentNum, nil | ||
| }) | ||
|
|
||
| startupConfig := integration.DefaultStartUpConfig(t) | ||
| startupConfig.GenesisBehavior = integration.Genesis_SKIP | ||
| diConfig := configurator.NewAppV2Config( | ||
| configurator.TxModule(), | ||
| configurator.AuthModule(), | ||
| configurator.BankModule(), | ||
| ) | ||
|
|
||
| var bankKeeper bankkeeper.Keeper | ||
| diConfig = depinject.Configs(diConfig, depinject.Supply(acctsModKeeper, log.NewNopLogger())) | ||
| app, err := integration.NewApp(diConfig, startupConfig, &bankKeeper) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, app) | ||
| return &deterministicFixture{app: app, bankKeeper: bankKeeper, T: t} | ||
| } | ||
|
|
||
| func assertNonZeroGas(t *testing.T, gasUsed gas.Gas) { | ||
| t.Helper() | ||
| require.NotZero(t, gasUsed) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in initDeterministicFixture
The initDeterministicFixture function is well-implemented, but it could benefit from additional error checking, particularly after creating the new app.
Consider adding more detailed error handling after the app creation:
app, err := integration.NewApp(diConfig, startupConfig, &bankKeeper)
require.NoError(t, err)
require.NotNil(t, app)
+if bankKeeper == nil {
+ t.Fatal("bankKeeper is nil after app creation")
+}
return &deterministicFixture{app: app, bankKeeper: bankKeeper, T: t}This change will provide more context if the bankKeeper is not properly initialized during the test setup.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func fundAccount(f *deterministicFixture, addr sdk.AccAddress, coin ...sdk.Coin) { | |
| err := banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, sdk.NewCoins(coin...)) | |
| require.NoError(f.T, err) | |
| } | |
| func getCoin(rt *rapid.T) sdk.Coin { | |
| return sdk.NewCoin( | |
| rapid.StringMatching(denomRegex).Draw(rt, "denom"), | |
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | |
| ) | |
| } | |
| func initDeterministicFixture(t *testing.T) *deterministicFixture { | |
| t.Helper() | |
| ctrl := gomock.NewController(t) | |
| acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl) | |
| accNum := uint64(0) | |
| acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) ( | |
| uint64, error, | |
| ) { | |
| currentNum := accNum | |
| accNum++ | |
| return currentNum, nil | |
| }) | |
| startupConfig := integration.DefaultStartUpConfig(t) | |
| startupConfig.GenesisBehavior = integration.Genesis_SKIP | |
| diConfig := configurator.NewAppV2Config( | |
| configurator.TxModule(), | |
| configurator.AuthModule(), | |
| configurator.BankModule(), | |
| ) | |
| var bankKeeper bankkeeper.Keeper | |
| diConfig = depinject.Configs(diConfig, depinject.Supply(acctsModKeeper, log.NewNopLogger())) | |
| app, err := integration.NewApp(diConfig, startupConfig, &bankKeeper) | |
| require.NoError(t, err) | |
| require.NotNil(t, app) | |
| return &deterministicFixture{app: app, bankKeeper: bankKeeper, T: t} | |
| } | |
| func assertNonZeroGas(t *testing.T, gasUsed gas.Gas) { | |
| t.Helper() | |
| require.NotZero(t, gasUsed) | |
| } | |
| func initDeterministicFixture(t *testing.T) *deterministicFixture { | |
| t.Helper() | |
| ctrl := gomock.NewController(t) | |
| acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl) | |
| accNum := uint64(0) | |
| acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) ( | |
| uint64, error, | |
| ) { | |
| currentNum := accNum | |
| accNum++ | |
| return currentNum, nil | |
| }) | |
| startupConfig := integration.DefaultStartUpConfig(t) | |
| startupConfig.GenesisBehavior = integration.Genesis_SKIP | |
| diConfig := configurator.NewAppV2Config( | |
| configurator.TxModule(), | |
| configurator.AuthModule(), | |
| configurator.BankModule(), | |
| ) | |
| var bankKeeper bankkeeper.Keeper | |
| diConfig = depinject.Configs(diConfig, depinject.Supply(acctsModKeeper, log.NewNopLogger())) | |
| app, err := integration.NewApp(diConfig, startupConfig, &bankKeeper) | |
| require.NoError(t, err) | |
| require.NotNil(t, app) | |
| if bankKeeper == nil { | |
| t.Fatal("bankKeeper is nil after app creation") | |
| } | |
| return &deterministicFixture{app: app, bankKeeper: bankKeeper, T: t} | |
| } | |
| func assertNonZeroGas(t *testing.T, gasUsed gas.Gas) { | |
| t.Helper() | |
| require.NotZero(t, gasUsed) | |
| } |
| func TestQueryBalance(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
| assertBalance := func(coin sdk.Coin) func(t *testing.T, res *banktypes.QueryBalanceResponse) { | ||
| return func(t *testing.T, res *banktypes.QueryBalanceResponse) { | ||
| require.Equal(t, coin.Denom, res.Balance.Denom) | ||
| require.Truef(t, coin.Amount.Equal(res.Balance.Amount), | ||
| "expected %s, got %s", coin.Amount, res.Balance.Amount) | ||
| } | ||
| } | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | ||
| coin := getCoin(rt) | ||
| fundAccount(f, addr, coin) | ||
|
|
||
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | ||
| require.NoError(t, err) | ||
|
|
||
| req := banktypes.NewQueryBalanceRequest(addrStr, coin.GetDenom()) | ||
|
|
||
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | ||
| f.QueryBalance, assertNonZeroGas, assertBalance(coin)) | ||
| }) | ||
|
|
||
| fundAccount(f, addr1, coin1) | ||
| addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) | ||
| require.NoError(t, err) | ||
| req := banktypes.NewQueryBalanceRequest(addr1Str, coin1.GetDenom()) | ||
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | ||
| f.QueryBalance, assertNonZeroGas, assertBalance(coin1)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance assertion in TestQueryBalance rapid check
The TestQueryBalance function is well-implemented, but the assertion in the rapid check could be more robust.
Consider adding an assertion to check if the balance is greater than zero:
assertBalance := func(coin sdk.Coin) func(t *testing.T, res *banktypes.QueryBalanceResponse) {
return func(t *testing.T, res *banktypes.QueryBalanceResponse) {
require.Equal(t, coin.Denom, res.Balance.Denom)
require.Truef(t, coin.Amount.Equal(res.Balance.Amount),
"expected %s, got %s", coin.Amount, res.Balance.Amount)
+ require.True(t, res.Balance.Amount.IsPositive(), "balance should be positive")
}
}This additional check ensures that the balance is always positive, which is an expected invariant for account balances.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestQueryBalance(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| assertBalance := func(coin sdk.Coin) func(t *testing.T, res *banktypes.QueryBalanceResponse) { | |
| return func(t *testing.T, res *banktypes.QueryBalanceResponse) { | |
| require.Equal(t, coin.Denom, res.Balance.Denom) | |
| require.Truef(t, coin.Amount.Equal(res.Balance.Amount), | |
| "expected %s, got %s", coin.Amount, res.Balance.Amount) | |
| } | |
| } | |
| rapid.Check(t, func(rt *rapid.T) { | |
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | |
| coin := getCoin(rt) | |
| fundAccount(f, addr, coin) | |
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | |
| require.NoError(t, err) | |
| req := banktypes.NewQueryBalanceRequest(addrStr, coin.GetDenom()) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QueryBalance, assertNonZeroGas, assertBalance(coin)) | |
| }) | |
| fundAccount(f, addr1, coin1) | |
| addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) | |
| require.NoError(t, err) | |
| req := banktypes.NewQueryBalanceRequest(addr1Str, coin1.GetDenom()) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QueryBalance, assertNonZeroGas, assertBalance(coin1)) | |
| } | |
| func TestQueryBalance(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| assertBalance := func(coin sdk.Coin) func(t *testing.T, res *banktypes.QueryBalanceResponse) { | |
| return func(t *testing.T, res *banktypes.QueryBalanceResponse) { | |
| require.Equal(t, coin.Denom, res.Balance.Denom) | |
| require.Truef(t, coin.Amount.Equal(res.Balance.Amount), | |
| "expected %s, got %s", coin.Amount, res.Balance.Amount) | |
| require.True(t, res.Balance.Amount.IsPositive(), "balance should be positive") | |
| } | |
| } | |
| rapid.Check(t, func(rt *rapid.T) { | |
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | |
| coin := getCoin(rt) | |
| fundAccount(f, addr, coin) | |
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | |
| require.NoError(t, err) | |
| req := banktypes.NewQueryBalanceRequest(addrStr, coin.GetDenom()) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QueryBalance, assertNonZeroGas, assertBalance(coin)) | |
| }) | |
| fundAccount(f, addr1, coin1) | |
| addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) | |
| require.NoError(t, err) | |
| req := banktypes.NewQueryBalanceRequest(addr1Str, coin1.GetDenom()) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QueryBalance, assertNonZeroGas, assertBalance(coin1)) | |
| } |
| func TestQueryParams(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| enabledStatus := banktypes.SendEnabled{ | ||
| Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"), | ||
| Enabled: rapid.Bool().Draw(rt, "status"), | ||
| } | ||
|
|
||
| params := banktypes.Params{ | ||
| SendEnabled: []*banktypes.SendEnabled{&enabledStatus}, | ||
| DefaultSendEnabled: rapid.Bool().Draw(rt, "send"), | ||
| } | ||
|
|
||
| err := f.bankKeeper.SetParams(f.ctx, params) | ||
| require.NoError(t, err) | ||
|
|
||
| req := &banktypes.QueryParamsRequest{} | ||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QueryParams, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| enabledStatus := banktypes.SendEnabled{ | ||
| Denom: "denom", | ||
| Enabled: true, | ||
| } | ||
|
|
||
| params := banktypes.Params{ | ||
| SendEnabled: []*banktypes.SendEnabled{&enabledStatus}, | ||
| DefaultSendEnabled: false, | ||
| } | ||
|
|
||
| err := f.bankKeeper.SetParams(f.ctx, params) | ||
| require.NoError(t, err) | ||
| req := &banktypes.QueryParamsRequest{} | ||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QueryParams, assertNonZeroGas, nil) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to TestQueryParams
The TestQueryParams function is well-structured, but it lacks specific assertions on the returned parameters.
Consider adding assertions to verify the correctness of the returned parameters:
testdata.DeterministicIterationsV2(
- t, f.ctx, req, gasMeterFactory, f.QueryParams, assertNonZeroGas, nil)
+ t, f.ctx, req, gasMeterFactory, f.QueryParams, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryParamsResponse) {
+ require.Equal(t, params.DefaultSendEnabled, res.Params.DefaultSendEnabled, "DefaultSendEnabled should match")
+ require.Equal(t, len(params.SendEnabled), len(res.Params.SendEnabled), "SendEnabled length should match")
+ for i, sendEnabled := range params.SendEnabled {
+ require.Equal(t, sendEnabled.Denom, res.Params.SendEnabled[i].Denom, "SendEnabled denom should match")
+ require.Equal(t, sendEnabled.Enabled, res.Params.SendEnabled[i].Enabled, "SendEnabled status should match")
+ }
+ })These assertions will ensure that the parameters returned by the query match the expected values set in the test.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestQueryParams(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| enabledStatus := banktypes.SendEnabled{ | |
| Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"), | |
| Enabled: rapid.Bool().Draw(rt, "status"), | |
| } | |
| params := banktypes.Params{ | |
| SendEnabled: []*banktypes.SendEnabled{&enabledStatus}, | |
| DefaultSendEnabled: rapid.Bool().Draw(rt, "send"), | |
| } | |
| err := f.bankKeeper.SetParams(f.ctx, params) | |
| require.NoError(t, err) | |
| req := &banktypes.QueryParamsRequest{} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryParams, assertNonZeroGas, nil) | |
| }) | |
| enabledStatus := banktypes.SendEnabled{ | |
| Denom: "denom", | |
| Enabled: true, | |
| } | |
| params := banktypes.Params{ | |
| SendEnabled: []*banktypes.SendEnabled{&enabledStatus}, | |
| DefaultSendEnabled: false, | |
| } | |
| err := f.bankKeeper.SetParams(f.ctx, params) | |
| require.NoError(t, err) | |
| req := &banktypes.QueryParamsRequest{} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryParams, assertNonZeroGas, nil) | |
| } | |
| func TestQueryParams(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| enabledStatus := banktypes.SendEnabled{ | |
| Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"), | |
| Enabled: rapid.Bool().Draw(rt, "status"), | |
| } | |
| params := banktypes.Params{ | |
| SendEnabled: []*banktypes.SendEnabled{&enabledStatus}, | |
| DefaultSendEnabled: rapid.Bool().Draw(rt, "send"), | |
| } | |
| err := f.bankKeeper.SetParams(f.ctx, params) | |
| require.NoError(t, err) | |
| req := &banktypes.QueryParamsRequest{} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryParams, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryParamsResponse) { | |
| require.Equal(t, params.DefaultSendEnabled, res.Params.DefaultSendEnabled, "DefaultSendEnabled should match") | |
| require.Equal(t, len(params.SendEnabled), len(res.Params.SendEnabled), "SendEnabled length should match") | |
| for i, sendEnabled := range params.SendEnabled { | |
| require.Equal(t, sendEnabled.Denom, res.Params.SendEnabled[i].Denom, "SendEnabled denom should match") | |
| require.Equal(t, sendEnabled.Enabled, res.Params.SendEnabled[i].Enabled, "SendEnabled status should match") | |
| } | |
| }) | |
| }) | |
| enabledStatus := banktypes.SendEnabled{ | |
| Denom: "denom", | |
| Enabled: true, | |
| } | |
| params := banktypes.Params{ | |
| SendEnabled: []*banktypes.SendEnabled{&enabledStatus}, | |
| DefaultSendEnabled: false, | |
| } | |
| err := f.bankKeeper.SetParams(f.ctx, params) | |
| require.NoError(t, err) | |
| req := &banktypes.QueryParamsRequest{} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryParams, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryParamsResponse) { | |
| require.Equal(t, params.DefaultSendEnabled, res.Params.DefaultSendEnabled, "DefaultSendEnabled should match") | |
| require.Equal(t, len(params.SendEnabled), len(res.Params.SendEnabled), "SendEnabled length should match") | |
| for i, sendEnabled := range params.SendEnabled { | |
| require.Equal(t, sendEnabled.Denom, res.Params.SendEnabled[i].Denom, "SendEnabled denom should match") | |
| require.Equal(t, sendEnabled.Enabled, res.Params.SendEnabled[i].Enabled, "SendEnabled status should match") | |
| } | |
| }) | |
| } |
| func TestQueryAllBalances(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
| addressCodec := codectestutil.CodecOptions{}.GetAddressCodec() | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | ||
| numCoins := rapid.IntRange(1, 10).Draw(rt, "num-count") | ||
| coins := make(sdk.Coins, 0, numCoins) | ||
|
|
||
| addrStr, err := addressCodec.BytesToString(addr) | ||
| require.NoError(t, err) | ||
|
|
||
| for i := 0; i < numCoins; i++ { | ||
| coin := getCoin(rt) | ||
| if exists, _ := coins.Find(coin.Denom); exists { | ||
| t.Skip("duplicate denom") | ||
| } | ||
| // NewCoins sorts the denoms | ||
| coins = sdk.NewCoins(append(coins, coin)...) | ||
| } | ||
|
|
||
| fundAccount(f, addr, coins...) | ||
|
|
||
| req := banktypes.NewQueryAllBalancesRequest( | ||
| addrStr, testdata.PaginationGenerator(rt, uint64(numCoins)).Draw(rt, "pagination"), false) | ||
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | ||
| f.QueryAllBalances, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| coins := sdk.NewCoins( | ||
| sdk.NewCoin("stake", math.NewInt(10)), | ||
| sdk.NewCoin("denom", math.NewInt(100)), | ||
| ) | ||
|
|
||
| fundAccount(f, addr1, coins...) | ||
| addr1Str, err := addressCodec.BytesToString(addr1) | ||
| require.NoError(t, err) | ||
|
|
||
| req := banktypes.NewQueryAllBalancesRequest(addr1Str, nil, false) | ||
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | ||
| f.QueryAllBalances, assertNonZeroGas, nil) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance assertions in TestQueryAllBalances
The TestQueryAllBalances function is well-implemented, but it could benefit from additional assertions to ensure the correctness of the returned balances.
Consider adding the following assertions within the rapid check:
testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory,
- f.QueryAllBalances, assertNonZeroGas, nil)
+ f.QueryAllBalances, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryAllBalancesResponse) {
+ require.Equal(t, len(coins), len(res.Balances), "number of returned balances should match")
+ for _, coin := range coins {
+ found := false
+ for _, balance := range res.Balances {
+ if balance.Denom == coin.Denom {
+ require.True(t, coin.Amount.Equal(balance.Amount), "amounts should match for denom %s", coin.Denom)
+ found = true
+ break
+ }
+ }
+ require.True(t, found, "coin %s should be present in the response", coin.Denom)
+ }
+ })These additional assertions will ensure that all funded coins are present in the response and that their amounts match the expected values.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestQueryAllBalances(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| addressCodec := codectestutil.CodecOptions{}.GetAddressCodec() | |
| rapid.Check(t, func(rt *rapid.T) { | |
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | |
| numCoins := rapid.IntRange(1, 10).Draw(rt, "num-count") | |
| coins := make(sdk.Coins, 0, numCoins) | |
| addrStr, err := addressCodec.BytesToString(addr) | |
| require.NoError(t, err) | |
| for i := 0; i < numCoins; i++ { | |
| coin := getCoin(rt) | |
| if exists, _ := coins.Find(coin.Denom); exists { | |
| t.Skip("duplicate denom") | |
| } | |
| // NewCoins sorts the denoms | |
| coins = sdk.NewCoins(append(coins, coin)...) | |
| } | |
| fundAccount(f, addr, coins...) | |
| req := banktypes.NewQueryAllBalancesRequest( | |
| addrStr, testdata.PaginationGenerator(rt, uint64(numCoins)).Draw(rt, "pagination"), false) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QueryAllBalances, assertNonZeroGas, nil) | |
| }) | |
| coins := sdk.NewCoins( | |
| sdk.NewCoin("stake", math.NewInt(10)), | |
| sdk.NewCoin("denom", math.NewInt(100)), | |
| ) | |
| fundAccount(f, addr1, coins...) | |
| addr1Str, err := addressCodec.BytesToString(addr1) | |
| require.NoError(t, err) | |
| req := banktypes.NewQueryAllBalancesRequest(addr1Str, nil, false) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QueryAllBalances, assertNonZeroGas, nil) | |
| } | |
| func TestQueryAllBalances(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| addressCodec := codectestutil.CodecOptions{}.GetAddressCodec() | |
| rapid.Check(t, func(rt *rapid.T) { | |
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | |
| numCoins := rapid.IntRange(1, 10).Draw(rt, "num-count") | |
| coins := make(sdk.Coins, 0, numCoins) | |
| addrStr, err := addressCodec.BytesToString(addr) | |
| require.NoError(t, err) | |
| for i := 0; i < numCoins; i++ { | |
| coin := getCoin(rt) | |
| if exists, _ := coins.Find(coin.Denom); exists { | |
| t.Skip("duplicate denom") | |
| } | |
| // NewCoins sorts the denoms | |
| coins = sdk.NewCoins(append(coins, coin)...) | |
| } | |
| fundAccount(f, addr, coins...) | |
| req := banktypes.NewQueryAllBalancesRequest( | |
| addrStr, testdata.PaginationGenerator(rt, uint64(numCoins)).Draw(rt, "pagination"), false) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QueryAllBalances, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryAllBalancesResponse) { | |
| require.Equal(t, len(coins), len(res.Balances), "number of returned balances should match") | |
| for _, coin := range coins { | |
| found := false | |
| for _, balance := range res.Balances { | |
| if balance.Denom == coin.Denom { | |
| require.True(t, coin.Amount.Equal(balance.Amount), "amounts should match for denom %s", coin.Denom) | |
| found = true | |
| break | |
| } | |
| } | |
| require.True(t, found, "coin %s should be present in the response", coin.Denom) | |
| } | |
| }) | |
| }) | |
| coins := sdk.NewCoins( | |
| sdk.NewCoin("stake", math.NewInt(10)), | |
| sdk.NewCoin("denom", math.NewInt(100)), | |
| ) | |
| fundAccount(f, addr1, coins...) | |
| addr1Str, err := addressCodec.BytesToString(addr1) | |
| require.NoError(t, err) | |
| req := banktypes.NewQueryAllBalancesRequest(addr1Str, nil, false) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QueryAllBalances, assertNonZeroGas, nil) | |
| } |
| func TestQuerySpendableBalances(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | ||
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | ||
| require.NoError(t, err) | ||
|
|
||
| // Denoms must be unique, otherwise sdk.NewCoins will panic. | ||
| denoms := rapid.SliceOfNDistinct(rapid.StringMatching(denomRegex), 1, 10, rapid.ID[string]).Draw(rt, "denoms") | ||
| coins := make(sdk.Coins, 0, len(denoms)) | ||
| for _, denom := range denoms { | ||
| coin := sdk.NewCoin( | ||
| denom, | ||
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | ||
| ) | ||
|
|
||
| // NewCoins sorts the denoms | ||
| coins = sdk.NewCoins(append(coins, coin)...) | ||
| } | ||
|
|
||
| err = banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, coins) | ||
| require.NoError(t, err) | ||
|
|
||
| req := banktypes.NewQuerySpendableBalancesRequest(addrStr, testdata.PaginationGenerator(rt, uint64(len(denoms))).Draw(rt, "pagination")) | ||
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | ||
| f.QuerySpendableBalances, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| coins := sdk.NewCoins( | ||
| sdk.NewCoin("stake", math.NewInt(10)), | ||
| sdk.NewCoin("denom", math.NewInt(100)), | ||
| ) | ||
|
|
||
| err := banktestutil.FundAccount(f.ctx, f.bankKeeper, addr1, coins) | ||
| require.NoError(t, err) | ||
|
|
||
| addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) | ||
| require.NoError(t, err) | ||
|
|
||
| req := banktypes.NewQuerySpendableBalancesRequest(addr1Str, nil) | ||
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | ||
| f.QuerySpendableBalances, assertNonZeroGas, nil) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to TestQuerySpendableBalances
The TestQuerySpendableBalances function is well-structured, but it lacks specific assertions on the returned spendable balances.
Consider adding assertions to verify the correctness of the spendable balances:
testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory,
- f.QuerySpendableBalances, assertNonZeroGas, nil)
+ f.QuerySpendableBalances, assertNonZeroGas, func(t *testing.T, res *banktypes.QuerySpendableBalancesResponse) {
+ require.Equal(t, len(coins), len(res.Balances), "number of spendable balances should match funded coins")
+ for _, coin := range coins {
+ found := false
+ for _, balance := range res.Balances {
+ if balance.Denom == coin.Denom {
+ require.True(t, coin.Amount.Equal(balance.Amount), "spendable amount should match funded amount for denom %s", coin.Denom)
+ found = true
+ break
+ }
+ }
+ require.True(t, found, "spendable balance for coin %s should be present in the response", coin.Denom)
+ }
+ })These assertions will ensure that the spendable balances returned by the query match the funded amounts for each coin.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestQuerySpendableBalances(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | |
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | |
| require.NoError(t, err) | |
| // Denoms must be unique, otherwise sdk.NewCoins will panic. | |
| denoms := rapid.SliceOfNDistinct(rapid.StringMatching(denomRegex), 1, 10, rapid.ID[string]).Draw(rt, "denoms") | |
| coins := make(sdk.Coins, 0, len(denoms)) | |
| for _, denom := range denoms { | |
| coin := sdk.NewCoin( | |
| denom, | |
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | |
| ) | |
| // NewCoins sorts the denoms | |
| coins = sdk.NewCoins(append(coins, coin)...) | |
| } | |
| err = banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, coins) | |
| require.NoError(t, err) | |
| req := banktypes.NewQuerySpendableBalancesRequest(addrStr, testdata.PaginationGenerator(rt, uint64(len(denoms))).Draw(rt, "pagination")) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QuerySpendableBalances, assertNonZeroGas, nil) | |
| }) | |
| coins := sdk.NewCoins( | |
| sdk.NewCoin("stake", math.NewInt(10)), | |
| sdk.NewCoin("denom", math.NewInt(100)), | |
| ) | |
| err := banktestutil.FundAccount(f.ctx, f.bankKeeper, addr1, coins) | |
| require.NoError(t, err) | |
| addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) | |
| require.NoError(t, err) | |
| req := banktypes.NewQuerySpendableBalancesRequest(addr1Str, nil) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QuerySpendableBalances, assertNonZeroGas, nil) | |
| } | |
| func TestQuerySpendableBalances(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | |
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | |
| require.NoError(t, err) | |
| // Denoms must be unique, otherwise sdk.NewCoins will panic. | |
| denoms := rapid.SliceOfNDistinct(rapid.StringMatching(denomRegex), 1, 10, rapid.ID[string]).Draw(rt, "denoms") | |
| coins := make(sdk.Coins, 0, len(denoms)) | |
| for _, denom := range denoms { | |
| coin := sdk.NewCoin( | |
| denom, | |
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | |
| ) | |
| // NewCoins sorts the denoms | |
| coins = sdk.NewCoins(append(coins, coin)...) | |
| } | |
| err = banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, coins) | |
| require.NoError(t, err) | |
| req := banktypes.NewQuerySpendableBalancesRequest(addrStr, testdata.PaginationGenerator(rt, uint64(len(denoms))).Draw(rt, "pagination")) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QuerySpendableBalances, assertNonZeroGas, nil) | |
| }) | |
| coins := sdk.NewCoins( | |
| sdk.NewCoin("stake", math.NewInt(10)), | |
| sdk.NewCoin("denom", math.NewInt(100)), | |
| ) | |
| err := banktestutil.FundAccount(f.ctx, f.bankKeeper, addr1, coins) | |
| require.NoError(t, err) | |
| addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) | |
| require.NoError(t, err) | |
| req := banktypes.NewQuerySpendableBalancesRequest(addr1Str, nil) | |
| testdata.DeterministicIterationsV2(t, f.ctx, req, gasMeterFactory, | |
| f.QuerySpendableBalances, assertNonZeroGas, func(t *testing.T, res *banktypes.QuerySpendableBalancesResponse) { | |
| require.Equal(t, len(coins), len(res.Balances), "number of spendable balances should match funded coins") | |
| for _, coin := range coins { | |
| found := false | |
| for _, balance := range res.Balances { | |
| if balance.Denom == coin.Denom { | |
| require.True(t, coin.Amount.Equal(balance.Amount), "spendable amount should match funded amount for denom %s", coin.Denom) | |
| found = true | |
| break | |
| } | |
| } | |
| require.True(t, found, "spendable balance for coin %s should be present in the response", coin.Denom) | |
| } | |
| }) | |
| } |
| func createAndReturnMetadatas(t *rapid.T, count int) []banktypes.Metadata { | ||
| denomsMetadata := make([]banktypes.Metadata, 0, count) | ||
| for i := 0; i < count; i++ { | ||
|
|
||
| denom := rapid.StringMatching(denomRegex).Draw(t, "denom") | ||
|
|
||
| aliases := rapid.SliceOf(rapid.String()).Draw(t, "aliases") | ||
| // In the GRPC server code, empty arrays are returned as nil | ||
| if len(aliases) == 0 { | ||
| aliases = nil | ||
| } | ||
|
|
||
| metadata := banktypes.Metadata{ | ||
| Description: rapid.StringN(1, 100, 100).Draw(t, "desc"), | ||
| DenomUnits: []*banktypes.DenomUnit{ | ||
| { | ||
| Denom: denom, | ||
| Exponent: rapid.Uint32().Draw(t, "exponent"), | ||
| Aliases: aliases, | ||
| }, | ||
| }, | ||
| Base: denom, | ||
| Display: denom, | ||
| Name: rapid.String().Draw(t, "name"), | ||
| Symbol: rapid.String().Draw(t, "symbol"), | ||
| URI: rapid.String().Draw(t, "uri"), | ||
| URIHash: rapid.String().Draw(t, "uri-hash"), | ||
| } | ||
|
|
||
| denomsMetadata = append(denomsMetadata, metadata) | ||
| } | ||
|
|
||
| return denomsMetadata | ||
| } | ||
|
|
||
| func TestDenomsMetadata(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| count := rapid.IntRange(1, 3).Draw(rt, "count") | ||
| denomsMetadata := createAndReturnMetadatas(rt, count) | ||
| require.True(t, len(denomsMetadata) == count) | ||
|
|
||
| for i := 0; i < count; i++ { | ||
| f.bankKeeper.SetDenomMetaData(f.ctx, denomsMetadata[i]) | ||
| } | ||
|
|
||
| req := &banktypes.QueryDenomsMetadataRequest{ | ||
| Pagination: testdata.PaginationGenerator(rt, uint64(count)).Draw(rt, "pagination"), | ||
| } | ||
|
|
||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QueryDenomsMetadata, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| f = initDeterministicFixture(t) // reset | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory = integration.GasMeterFactory(f.ctx) | ||
|
|
||
| f.bankKeeper.SetDenomMetaData(f.ctx, metadataAtom) | ||
|
|
||
| req := &banktypes.QueryDenomsMetadataRequest{} | ||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QueryDenomsMetadata, assertNonZeroGas, nil) | ||
| } | ||
|
|
||
| func TestDenomMetadata(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| denomMetadata := createAndReturnMetadatas(rt, 1) | ||
| require.True(t, len(denomMetadata) == 1) | ||
| f.bankKeeper.SetDenomMetaData(f.ctx, denomMetadata[0]) | ||
|
|
||
| req := &banktypes.QueryDenomMetadataRequest{ | ||
| Denom: denomMetadata[0].Base, | ||
| } | ||
|
|
||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QueryDenomMetadata, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| f.bankKeeper.SetDenomMetaData(f.ctx, metadataAtom) | ||
|
|
||
| req := &banktypes.QueryDenomMetadataRequest{ | ||
| Denom: metadataAtom.Base, | ||
| } | ||
|
|
||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QueryDenomMetadata, assertNonZeroGas, nil) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance metadata-related test assertions
The TestDenomsMetadata and TestDenomMetadata functions are well-implemented, but they could benefit from more specific assertions to ensure the correctness of the returned metadata.
For TestDenomsMetadata, consider adding the following assertion:
testdata.DeterministicIterationsV2(
- t, f.ctx, req, gasMeterFactory, f.QueryDenomsMetadata, assertNonZeroGas, nil)
+ t, f.ctx, req, gasMeterFactory, f.QueryDenomsMetadata, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryDenomsMetadataResponse) {
+ require.Equal(t, len(denomsMetadata), len(res.Metadatas), "number of metadata entries should match")
+ for _, expected := range denomsMetadata {
+ found := false
+ for _, actual := range res.Metadatas {
+ if expected.Base == actual.Base {
+ require.Equal(t, expected, actual, "metadata for denom %s should match", expected.Base)
+ found = true
+ break
+ }
+ }
+ require.True(t, found, "metadata for denom %s should be present in the response", expected.Base)
+ }
+ })For TestDenomMetadata, add this assertion:
testdata.DeterministicIterationsV2(
- t, f.ctx, req, gasMeterFactory, f.QueryDenomMetadata, assertNonZeroGas, nil)
+ t, f.ctx, req, gasMeterFactory, f.QueryDenomMetadata, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryDenomMetadataResponse) {
+ require.Equal(t, denomMetadata[0], res.Metadata, "returned metadata should match the set metadata")
+ })These additions will ensure that the returned metadata values match the expected metadata set in the tests.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func createAndReturnMetadatas(t *rapid.T, count int) []banktypes.Metadata { | |
| denomsMetadata := make([]banktypes.Metadata, 0, count) | |
| for i := 0; i < count; i++ { | |
| denom := rapid.StringMatching(denomRegex).Draw(t, "denom") | |
| aliases := rapid.SliceOf(rapid.String()).Draw(t, "aliases") | |
| // In the GRPC server code, empty arrays are returned as nil | |
| if len(aliases) == 0 { | |
| aliases = nil | |
| } | |
| metadata := banktypes.Metadata{ | |
| Description: rapid.StringN(1, 100, 100).Draw(t, "desc"), | |
| DenomUnits: []*banktypes.DenomUnit{ | |
| { | |
| Denom: denom, | |
| Exponent: rapid.Uint32().Draw(t, "exponent"), | |
| Aliases: aliases, | |
| }, | |
| }, | |
| Base: denom, | |
| Display: denom, | |
| Name: rapid.String().Draw(t, "name"), | |
| Symbol: rapid.String().Draw(t, "symbol"), | |
| URI: rapid.String().Draw(t, "uri"), | |
| URIHash: rapid.String().Draw(t, "uri-hash"), | |
| } | |
| denomsMetadata = append(denomsMetadata, metadata) | |
| } | |
| return denomsMetadata | |
| } | |
| func TestDenomsMetadata(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| count := rapid.IntRange(1, 3).Draw(rt, "count") | |
| denomsMetadata := createAndReturnMetadatas(rt, count) | |
| require.True(t, len(denomsMetadata) == count) | |
| for i := 0; i < count; i++ { | |
| f.bankKeeper.SetDenomMetaData(f.ctx, denomsMetadata[i]) | |
| } | |
| req := &banktypes.QueryDenomsMetadataRequest{ | |
| Pagination: testdata.PaginationGenerator(rt, uint64(count)).Draw(rt, "pagination"), | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomsMetadata, assertNonZeroGas, nil) | |
| }) | |
| f = initDeterministicFixture(t) // reset | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory = integration.GasMeterFactory(f.ctx) | |
| f.bankKeeper.SetDenomMetaData(f.ctx, metadataAtom) | |
| req := &banktypes.QueryDenomsMetadataRequest{} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomsMetadata, assertNonZeroGas, nil) | |
| } | |
| func TestDenomMetadata(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| denomMetadata := createAndReturnMetadatas(rt, 1) | |
| require.True(t, len(denomMetadata) == 1) | |
| f.bankKeeper.SetDenomMetaData(f.ctx, denomMetadata[0]) | |
| req := &banktypes.QueryDenomMetadataRequest{ | |
| Denom: denomMetadata[0].Base, | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomMetadata, assertNonZeroGas, nil) | |
| }) | |
| f.bankKeeper.SetDenomMetaData(f.ctx, metadataAtom) | |
| req := &banktypes.QueryDenomMetadataRequest{ | |
| Denom: metadataAtom.Base, | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomMetadata, assertNonZeroGas, nil) | |
| } | |
| func TestDenomsMetadata(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| count := rapid.IntRange(1, 3).Draw(rt, "count") | |
| denomsMetadata := createAndReturnMetadatas(rt, count) | |
| require.True(t, len(denomsMetadata) == count) | |
| for i := 0; i < count; i++ { | |
| f.bankKeeper.SetDenomMetaData(f.ctx, denomsMetadata[i]) | |
| } | |
| req := &banktypes.QueryDenomsMetadataRequest{ | |
| Pagination: testdata.PaginationGenerator(rt, uint64(count)).Draw(rt, "pagination"), | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomsMetadata, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryDenomsMetadataResponse) { | |
| require.Equal(t, len(denomsMetadata), len(res.Metadatas), "number of metadata entries should match") | |
| for _, expected := range denomsMetadata { | |
| found := false | |
| for _, actual := range res.Metadatas { | |
| if expected.Base == actual.Base { | |
| require.Equal(t, expected, actual, "metadata for denom %s should match", expected.Base) | |
| found = true | |
| break | |
| } | |
| } | |
| require.True(t, found, "metadata for denom %s should be present in the response", expected.Base) | |
| } | |
| }) | |
| }) | |
| f = initDeterministicFixture(t) // reset | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory = integration.GasMeterFactory(f.ctx) | |
| f.bankKeeper.SetDenomMetaData(f.ctx, metadataAtom) | |
| req := &banktypes.QueryDenomsMetadataRequest{} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomsMetadata, assertNonZeroGas, nil) | |
| } | |
| func TestDenomMetadata(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| denomMetadata := createAndReturnMetadatas(rt, 1) | |
| require.True(t, len(denomMetadata) == 1) | |
| f.bankKeeper.SetDenomMetaData(f.ctx, denomMetadata[0]) | |
| req := &banktypes.QueryDenomMetadataRequest{ | |
| Denom: denomMetadata[0].Base, | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomMetadata, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryDenomMetadataResponse) { | |
| require.Equal(t, denomMetadata[0], res.Metadata, "returned metadata should match the set metadata") | |
| }) | |
| }) | |
| f.bankKeeper.SetDenomMetaData(f.ctx, metadataAtom) | |
| req := &banktypes.QueryDenomMetadataRequest{ | |
| Denom: metadataAtom.Base, | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomMetadata, assertNonZeroGas, nil) | |
| } |
| func TestQueryTotalSupply(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
|
|
||
| res, err := f.QueryTotalSupply(f.ctx, &banktypes.QueryTotalSupplyRequest{}) | ||
| require.NoError(t, err) | ||
| initialSupply := res.GetSupply() | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| numCoins := rapid.IntRange(1, 3).Draw(rt, "num-count") | ||
| coins := make(sdk.Coins, 0, numCoins) | ||
|
|
||
| for i := 0; i < numCoins; i++ { | ||
| coin := sdk.NewCoin( | ||
| rapid.StringMatching(denomRegex).Draw(rt, "denom"), | ||
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | ||
| ) | ||
|
|
||
| coins = coins.Add(coin) | ||
| } | ||
|
|
||
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins)) | ||
|
|
||
| initialSupply = initialSupply.Add(coins...) | ||
|
|
||
| req := &banktypes.QueryTotalSupplyRequest{ | ||
| Pagination: testdata.PaginationGenerator(rt, uint64(len(initialSupply))).Draw(rt, "pagination"), | ||
| } | ||
|
|
||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QueryTotalSupply, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| f = initDeterministicFixture(t) // reset | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory = integration.GasMeterFactory(f.ctx) | ||
|
|
||
| coins := sdk.NewCoins( | ||
| sdk.NewCoin("foo", math.NewInt(10)), | ||
| sdk.NewCoin("bar", math.NewInt(100)), | ||
| ) | ||
|
|
||
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins)) | ||
|
|
||
| req := &banktypes.QueryTotalSupplyRequest{} | ||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QueryTotalSupply, assertNonZeroGas, nil) | ||
| } | ||
|
|
||
| func TestQueryTotalSupplyOf(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| coin := sdk.NewCoin( | ||
| rapid.StringMatching(denomRegex).Draw(rt, "denom"), | ||
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | ||
| ) | ||
|
|
||
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, sdk.NewCoins(coin))) | ||
|
|
||
| req := &banktypes.QuerySupplyOfRequest{Denom: coin.GetDenom()} | ||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QuerySupplyOf, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| coin := sdk.NewCoin("bar", math.NewInt(100)) | ||
|
|
||
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, sdk.NewCoins(coin))) | ||
| req := &banktypes.QuerySupplyOfRequest{Denom: coin.GetDenom()} | ||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QuerySupplyOf, assertNonZeroGas, nil) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance assertions in supply-related tests
The TestQueryTotalSupply and TestQueryTotalSupplyOf functions are well-implemented, but they could benefit from more specific assertions to ensure the correctness of the returned supply.
For TestQueryTotalSupply, consider adding the following assertion:
testdata.DeterministicIterationsV2(
- t, f.ctx, req, gasMeterFactory, f.QueryTotalSupply, assertNonZeroGas, nil)
+ t, f.ctx, req, gasMeterFactory, f.QueryTotalSupply, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryTotalSupplyResponse) {
+ require.Equal(t, initialSupply.String(), res.Supply.String(), "total supply should match the expected value")
+ })For TestQueryTotalSupplyOf, add this assertion:
testdata.DeterministicIterationsV2(
- t, f.ctx, req, gasMeterFactory, f.QuerySupplyOf, assertNonZeroGas, nil)
+ t, f.ctx, req, gasMeterFactory, f.QuerySupplyOf, assertNonZeroGas, func(t *testing.T, res *banktypes.QuerySupplyOfResponse) {
+ require.Equal(t, coin.Amount, res.Amount.Amount, "supply of %s should match the minted amount", coin.Denom)
+ })These additions will ensure that the returned supply values match the expected amounts based on the minted coins.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestQueryTotalSupply(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| res, err := f.QueryTotalSupply(f.ctx, &banktypes.QueryTotalSupplyRequest{}) | |
| require.NoError(t, err) | |
| initialSupply := res.GetSupply() | |
| rapid.Check(t, func(rt *rapid.T) { | |
| numCoins := rapid.IntRange(1, 3).Draw(rt, "num-count") | |
| coins := make(sdk.Coins, 0, numCoins) | |
| for i := 0; i < numCoins; i++ { | |
| coin := sdk.NewCoin( | |
| rapid.StringMatching(denomRegex).Draw(rt, "denom"), | |
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | |
| ) | |
| coins = coins.Add(coin) | |
| } | |
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins)) | |
| initialSupply = initialSupply.Add(coins...) | |
| req := &banktypes.QueryTotalSupplyRequest{ | |
| Pagination: testdata.PaginationGenerator(rt, uint64(len(initialSupply))).Draw(rt, "pagination"), | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryTotalSupply, assertNonZeroGas, nil) | |
| }) | |
| f = initDeterministicFixture(t) // reset | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory = integration.GasMeterFactory(f.ctx) | |
| coins := sdk.NewCoins( | |
| sdk.NewCoin("foo", math.NewInt(10)), | |
| sdk.NewCoin("bar", math.NewInt(100)), | |
| ) | |
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins)) | |
| req := &banktypes.QueryTotalSupplyRequest{} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryTotalSupply, assertNonZeroGas, nil) | |
| } | |
| func TestQueryTotalSupplyOf(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| coin := sdk.NewCoin( | |
| rapid.StringMatching(denomRegex).Draw(rt, "denom"), | |
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | |
| ) | |
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, sdk.NewCoins(coin))) | |
| req := &banktypes.QuerySupplyOfRequest{Denom: coin.GetDenom()} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QuerySupplyOf, assertNonZeroGas, nil) | |
| }) | |
| coin := sdk.NewCoin("bar", math.NewInt(100)) | |
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, sdk.NewCoins(coin))) | |
| req := &banktypes.QuerySupplyOfRequest{Denom: coin.GetDenom()} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QuerySupplyOf, assertNonZeroGas, nil) | |
| } | |
| func TestQueryTotalSupply(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| res, err := f.QueryTotalSupply(f.ctx, &banktypes.QueryTotalSupplyRequest{}) | |
| require.NoError(t, err) | |
| initialSupply := res.GetSupply() | |
| rapid.Check(t, func(rt *rapid.T) { | |
| numCoins := rapid.IntRange(1, 3).Draw(rt, "num-count") | |
| coins := make(sdk.Coins, 0, numCoins) | |
| for i := 0; i < numCoins; i++ { | |
| coin := sdk.NewCoin( | |
| rapid.StringMatching(denomRegex).Draw(rt, "denom"), | |
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | |
| ) | |
| coins = coins.Add(coin) | |
| } | |
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins)) | |
| initialSupply = initialSupply.Add(coins...) | |
| req := &banktypes.QueryTotalSupplyRequest{ | |
| Pagination: testdata.PaginationGenerator(rt, uint64(len(initialSupply))).Draw(rt, "pagination"), | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryTotalSupply, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryTotalSupplyResponse) { | |
| require.Equal(t, initialSupply.String(), res.Supply.String(), "total supply should match the expected value") | |
| }) | |
| }) | |
| f = initDeterministicFixture(t) // reset | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory = integration.GasMeterFactory(f.ctx) | |
| coins := sdk.NewCoins( | |
| sdk.NewCoin("foo", math.NewInt(10)), | |
| sdk.NewCoin("bar", math.NewInt(100)), | |
| ) | |
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins)) | |
| req := &banktypes.QueryTotalSupplyRequest{} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryTotalSupply, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryTotalSupplyResponse) { | |
| require.Equal(t, initialSupply.String(), res.Supply.String(), "total supply should match the expected value") | |
| }) | |
| } | |
| func TestQueryTotalSupplyOf(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| coin := sdk.NewCoin( | |
| rapid.StringMatching(denomRegex).Draw(rt, "denom"), | |
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | |
| ) | |
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, sdk.NewCoins(coin))) | |
| req := &banktypes.QuerySupplyOfRequest{Denom: coin.GetDenom()} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QuerySupplyOf, assertNonZeroGas, func(t *testing.T, res *banktypes.QuerySupplyOfResponse) { | |
| require.Equal(t, coin.Amount, res.Amount.Amount, "supply of %s should match the minted amount", coin.Denom) | |
| }) | |
| }) | |
| coin := sdk.NewCoin("bar", math.NewInt(100)) | |
| require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, sdk.NewCoins(coin))) | |
| req := &banktypes.QuerySupplyOfRequest{Denom: coin.GetDenom()} | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QuerySupplyOf, assertNonZeroGas, func(t *testing.T, res *banktypes.QuerySupplyOfResponse) { | |
| require.Equal(t, coin.Amount, res.Amount.Amount, "supply of %s should match the minted amount", coin.Denom) | |
| }) | |
| } |
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| count := rapid.IntRange(0, 10).Draw(rt, "count") | ||
| denoms := make([]string, 0, count) | ||
|
|
||
| for i := 0; i < count; i++ { | ||
| coin := banktypes.SendEnabled{ | ||
| Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"), | ||
| Enabled: rapid.Bool().Draw(rt, "enabled-status"), | ||
| } | ||
|
|
||
| f.bankKeeper.SetSendEnabled(f.ctx, coin.Denom, coin.Enabled) | ||
| denoms = append(denoms, coin.Denom) | ||
| } | ||
|
|
||
| allDenoms = append(allDenoms, denoms...) | ||
|
|
||
| req := &banktypes.QuerySendEnabledRequest{ | ||
| Denoms: denoms, | ||
| // Pagination is only taken into account when `denoms` is an empty array | ||
| Pagination: testdata.PaginationGenerator(rt, uint64(len(allDenoms))).Draw(rt, "pagination"), | ||
| } | ||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QuerySendEnabled, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| coin1 := banktypes.SendEnabled{ | ||
| Denom: "falsecoin", | ||
| Enabled: false, | ||
| } | ||
| coin2 := banktypes.SendEnabled{ | ||
| Denom: "truecoin", | ||
| Enabled: true, | ||
| } | ||
|
|
||
| f.bankKeeper.SetSendEnabled(f.ctx, coin1.Denom, false) | ||
| f.bankKeeper.SetSendEnabled(f.ctx, coin2.Denom, true) | ||
|
|
||
| req := &banktypes.QuerySendEnabledRequest{ | ||
| Denoms: []string{coin1.GetDenom(), coin2.GetDenom()}, | ||
| } | ||
|
|
||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QuerySendEnabled, assertNonZeroGas, nil) | ||
| } | ||
|
|
||
| func TestDenomOwners(t *testing.T) { | ||
| t.Parallel() | ||
| f := initDeterministicFixture(t) | ||
| f.ctx = f.app.StateLatestContext(t) | ||
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
|
|
||
| rapid.Check(t, func(rt *rapid.T) { | ||
| denom := rapid.StringMatching(denomRegex).Draw(rt, "denom") | ||
| numAddr := rapid.IntRange(1, 10).Draw(rt, "number-address") | ||
| for i := 0; i < numAddr; i++ { | ||
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | ||
|
|
||
| coin := sdk.NewCoin( | ||
| denom, | ||
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | ||
| ) | ||
|
|
||
| err := banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, sdk.NewCoins(coin)) | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| req := &banktypes.QueryDenomOwnersRequest{ | ||
| Denom: denom, | ||
| Pagination: testdata.PaginationGenerator(rt, uint64(numAddr)).Draw(rt, "pagination"), | ||
| } | ||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QueryDenomOwners, assertNonZeroGas, nil) | ||
| }) | ||
|
|
||
| denomOwners := []*banktypes.DenomOwner{ | ||
| { | ||
| Address: "cosmos1qg65a9q6k2sqq7l3ycp428sqqpmqcucgzze299", | ||
| Balance: coin1, | ||
| }, | ||
| { | ||
| Address: "cosmos1qglnsqgpq48l7qqzgs8qdshr6fh3gqq9ej3qut", | ||
| Balance: coin1, | ||
| }, | ||
| } | ||
|
|
||
| for i := 0; i < len(denomOwners); i++ { | ||
| addr, err := sdk.AccAddressFromBech32(denomOwners[i].Address) | ||
| require.NoError(t, err) | ||
|
|
||
| err = banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, sdk.NewCoins(coin1)) | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| req := &banktypes.QueryDenomOwnersRequest{ | ||
| Denom: coin1.GetDenom(), | ||
| } | ||
| testdata.DeterministicIterationsV2( | ||
| t, f.ctx, req, gasMeterFactory, f.QueryDenomOwners, assertNonZeroGas, nil) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance assertions in SendEnabled and DenomOwners tests
The TestSendEnabled and TestDenomOwners functions are well-implemented, but they could benefit from more specific assertions to ensure the correctness of the returned data.
For TestSendEnabled, consider adding the following assertion:
testdata.DeterministicIterationsV2(
- t, f.ctx, req, gasMeterFactory, f.QuerySendEnabled, assertNonZeroGas, nil)
+ t, f.ctx, req, gasMeterFactory, f.QuerySendEnabled, assertNonZeroGas, func(t *testing.T, res *banktypes.QuerySendEnabledResponse) {
+ require.Equal(t, len(req.Denoms), len(res.SendEnabled), "number of SendEnabled entries should match")
+ for _, denom := range req.Denoms {
+ found := false
+ for _, sendEnabled := range res.SendEnabled {
+ if sendEnabled.Denom == denom {
+ enabled, _ := f.bankKeeper.IsSendEnabledDenom(f.ctx, denom)
+ require.Equal(t, enabled, sendEnabled.Enabled, "SendEnabled status for denom %s should match", denom)
+ found = true
+ break
+ }
+ }
+ require.True(t, found, "SendEnabled entry for denom %s should be present in the response", denom)
+ }
+ })For TestDenomOwners, add this assertion:
testdata.DeterministicIterationsV2(
- t, f.ctx, req, gasMeterFactory, f.QueryDenomOwners, assertNonZeroGas, nil)
+ t, f.ctx, req, gasMeterFactory, f.QueryDenomOwners, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryDenomOwnersResponse) {
+ require.Equal(t, len(denomOwners), len(res.DenomOwners), "number of DenomOwners should match")
+ for _, expected := range denomOwners {
+ found := false
+ for _, actual := range res.DenomOwners {
+ if expected.Address == actual.Address {
+ require.Equal(t, expected.Balance.Denom, actual.Balance.Denom, "denom should match for address %s", expected.Address)
+ require.True(t, expected.Balance.Amount.Equal(actual.Balance.Amount), "amount should match for address %s", expected.Address)
+ found = true
+ break
+ }
+ }
+ require.True(t, found, "DenomOwner for address %s should be present in the response", expected.Address)
+ }
+ })These additions will ensure that the returned SendEnabled statuses and DenomOwners data match the expected values set in the tests.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestSendEnabled(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| allDenoms := []string{} | |
| rapid.Check(t, func(rt *rapid.T) { | |
| count := rapid.IntRange(0, 10).Draw(rt, "count") | |
| denoms := make([]string, 0, count) | |
| for i := 0; i < count; i++ { | |
| coin := banktypes.SendEnabled{ | |
| Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"), | |
| Enabled: rapid.Bool().Draw(rt, "enabled-status"), | |
| } | |
| f.bankKeeper.SetSendEnabled(f.ctx, coin.Denom, coin.Enabled) | |
| denoms = append(denoms, coin.Denom) | |
| } | |
| allDenoms = append(allDenoms, denoms...) | |
| req := &banktypes.QuerySendEnabledRequest{ | |
| Denoms: denoms, | |
| // Pagination is only taken into account when `denoms` is an empty array | |
| Pagination: testdata.PaginationGenerator(rt, uint64(len(allDenoms))).Draw(rt, "pagination"), | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QuerySendEnabled, assertNonZeroGas, nil) | |
| }) | |
| coin1 := banktypes.SendEnabled{ | |
| Denom: "falsecoin", | |
| Enabled: false, | |
| } | |
| coin2 := banktypes.SendEnabled{ | |
| Denom: "truecoin", | |
| Enabled: true, | |
| } | |
| f.bankKeeper.SetSendEnabled(f.ctx, coin1.Denom, false) | |
| f.bankKeeper.SetSendEnabled(f.ctx, coin2.Denom, true) | |
| req := &banktypes.QuerySendEnabledRequest{ | |
| Denoms: []string{coin1.GetDenom(), coin2.GetDenom()}, | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QuerySendEnabled, assertNonZeroGas, nil) | |
| } | |
| func TestDenomOwners(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| denom := rapid.StringMatching(denomRegex).Draw(rt, "denom") | |
| numAddr := rapid.IntRange(1, 10).Draw(rt, "number-address") | |
| for i := 0; i < numAddr; i++ { | |
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | |
| coin := sdk.NewCoin( | |
| denom, | |
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | |
| ) | |
| err := banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, sdk.NewCoins(coin)) | |
| require.NoError(t, err) | |
| } | |
| req := &banktypes.QueryDenomOwnersRequest{ | |
| Denom: denom, | |
| Pagination: testdata.PaginationGenerator(rt, uint64(numAddr)).Draw(rt, "pagination"), | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomOwners, assertNonZeroGas, nil) | |
| }) | |
| denomOwners := []*banktypes.DenomOwner{ | |
| { | |
| Address: "cosmos1qg65a9q6k2sqq7l3ycp428sqqpmqcucgzze299", | |
| Balance: coin1, | |
| }, | |
| { | |
| Address: "cosmos1qglnsqgpq48l7qqzgs8qdshr6fh3gqq9ej3qut", | |
| Balance: coin1, | |
| }, | |
| } | |
| for i := 0; i < len(denomOwners); i++ { | |
| addr, err := sdk.AccAddressFromBech32(denomOwners[i].Address) | |
| require.NoError(t, err) | |
| err = banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, sdk.NewCoins(coin1)) | |
| require.NoError(t, err) | |
| } | |
| req := &banktypes.QueryDenomOwnersRequest{ | |
| Denom: coin1.GetDenom(), | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomOwners, assertNonZeroGas, nil) | |
| } | |
| func TestSendEnabled(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| allDenoms := []string{} | |
| rapid.Check(t, func(rt *rapid.T) { | |
| count := rapid.IntRange(0, 10).Draw(rt, "count") | |
| denoms := make([]string, 0, count) | |
| for i := 0; i < count; i++ { | |
| coin := banktypes.SendEnabled{ | |
| Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"), | |
| Enabled: rapid.Bool().Draw(rt, "enabled-status"), | |
| } | |
| f.bankKeeper.SetSendEnabled(f.ctx, coin.Denom, coin.Enabled) | |
| denoms = append(denoms, coin.Denom) | |
| } | |
| allDenoms = append(allDenoms, denoms...) | |
| req := &banktypes.QuerySendEnabledRequest{ | |
| Denoms: denoms, | |
| // Pagination is only taken into account when `denoms` is an empty array | |
| Pagination: testdata.PaginationGenerator(rt, uint64(len(allDenoms))).Draw(rt, "pagination"), | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QuerySendEnabled, assertNonZeroGas, func(t *testing.T, res *banktypes.QuerySendEnabledResponse) { | |
| require.Equal(t, len(req.Denoms), len(res.SendEnabled), "number of SendEnabled entries should match") | |
| for _, denom := range req.Denoms { | |
| found := false | |
| for _, sendEnabled := range res.SendEnabled { | |
| if sendEnabled.Denom == denom { | |
| enabled, _ := f.bankKeeper.IsSendEnabledDenom(f.ctx, denom) | |
| require.Equal(t, enabled, sendEnabled.Enabled, "SendEnabled status for denom %s should match", denom) | |
| found = true | |
| break | |
| } | |
| } | |
| require.True(t, found, "SendEnabled entry for denom %s should be present in the response", denom) | |
| } | |
| }) | |
| }) | |
| coin1 := banktypes.SendEnabled{ | |
| Denom: "falsecoin", | |
| Enabled: false, | |
| } | |
| coin2 := banktypes.SendEnabled{ | |
| Denom: "truecoin", | |
| Enabled: true, | |
| } | |
| f.bankKeeper.SetSendEnabled(f.ctx, coin1.Denom, false) | |
| f.bankKeeper.SetSendEnabled(f.ctx, coin2.Denom, true) | |
| req := &banktypes.QuerySendEnabledRequest{ | |
| Denoms: []string{coin1.GetDenom(), coin2.GetDenom()}, | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QuerySendEnabled, assertNonZeroGas, func(t *testing.T, res *banktypes.QuerySendEnabledResponse) { | |
| require.Equal(t, len(req.Denoms), len(res.SendEnabled), "number of SendEnabled entries should match") | |
| for _, denom := range req.Denoms { | |
| found := false | |
| for _, sendEnabled := range res.SendEnabled { | |
| if sendEnabled.Denom == denom { | |
| enabled, _ := f.bankKeeper.IsSendEnabledDenom(f.ctx, denom) | |
| require.Equal(t, enabled, sendEnabled.Enabled, "SendEnabled status for denom %s should match", denom) | |
| found = true | |
| break | |
| } | |
| } | |
| require.True(t, found, "SendEnabled entry for denom %s should be present in the response", denom) | |
| } | |
| }) | |
| } | |
| func TestDenomOwners(t *testing.T) { | |
| t.Parallel() | |
| f := initDeterministicFixture(t) | |
| f.ctx = f.app.StateLatestContext(t) | |
| gasMeterFactory := integration.GasMeterFactory(f.ctx) | |
| rapid.Check(t, func(rt *rapid.T) { | |
| denom := rapid.StringMatching(denomRegex).Draw(rt, "denom") | |
| numAddr := rapid.IntRange(1, 10).Draw(rt, "number-address") | |
| for i := 0; i < numAddr; i++ { | |
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | |
| coin := sdk.NewCoin( | |
| denom, | |
| math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")), | |
| ) | |
| err := banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, sdk.NewCoins(coin)) | |
| require.NoError(t, err) | |
| } | |
| req := &banktypes.QueryDenomOwnersRequest{ | |
| Denom: denom, | |
| Pagination: testdata.PaginationGenerator(rt, uint64(numAddr)).Draw(rt, "pagination"), | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomOwners, assertNonZeroGas, nil) | |
| }) | |
| denomOwners := []*banktypes.DenomOwner{ | |
| { | |
| Address: "cosmos1qg65a9q6k2sqq7l3ycp428sqqpmqcucgzze299", | |
| Balance: coin1, | |
| }, | |
| { | |
| Address: "cosmos1qglnsqgpq48l7qqzgs8qdshr6fh3gqq9ej3qut", | |
| Balance: coin1, | |
| }, | |
| } | |
| for i := 0; i < len(denomOwners); i++ { | |
| addr, err := sdk.AccAddressFromBech32(denomOwners[i].Address) | |
| require.NoError(t, err) | |
| err = banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, sdk.NewCoins(coin1)) | |
| require.NoError(t, err) | |
| } | |
| req := &banktypes.QueryDenomOwnersRequest{ | |
| Denom: coin1.GetDenom(), | |
| } | |
| testdata.DeterministicIterationsV2( | |
| t, f.ctx, req, gasMeterFactory, f.QueryDenomOwners, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryDenomOwnersResponse) { | |
| require.Equal(t, len(denomOwners), len(res.DenomOwners), "number of DenomOwners should match") | |
| for _, expected := range denomOwners { | |
| found := false | |
| for _, actual := range res.DenomOwners { | |
| if expected.Address == actual.Address { | |
| require.Equal(t, expected.Balance.Denom, actual.Balance.Denom, "denom should match for address %s", expected.Address) | |
| require.True(t, expected.Balance.Amount.Equal(actual.Balance.Amount), "amount should match for address %s", expected.Address) | |
| found = true | |
| break | |
| } | |
| } | |
| require.True(t, found, "DenomOwner for address %s should be present in the response", expected.Address) | |
| } | |
| }) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
tests/integration/v2/bank/determinisitic_test.go (3)
347-354: Check for error when setting bank parametersIn
TestQueryParams, after callingf.bankKeeper.SetParams, it is good practice to check for errors even if the current implementation does not return one. This ensures robustness against future changes where an error might be returned.Apply this diff to check for errors:
params := banktypes.Params{ SendEnabled: []*banktypes.SendEnabled{&enabledStatus}, DefaultSendEnabled: false, } -err := f.bankKeeper.SetParams(f.ctx, params) +require.NoError(t, f.bankKeeper.SetParams(f.ctx, params))
218-219: Check for error when funding accountIn
TestQuerySpendableBalances, after callingbanktestutil.FundAccount, it's good practice to check for errors even if it's unlikely to occur. This ensures robustness and easier debugging.Ensure that the error is not overwritten and is properly checked:
err = banktestutil.FundAccount(f.ctx, f.bankKeeper, addr, coins) -require.NoError(t, err) +require.NoError(t, err, "failed to fund account")
360-389: Limit the length of random strings to avoid excessively large inputsIn the
createAndReturnMetadatasfunction, the random strings generated for fields likeDescription,Name,Symbol,URI, andURIHashcan potentially be very long. This might lead to excessively large inputs that can slow down tests or cause unexpected failures.Consider limiting the length of the random strings generated:
metadata := banktypes.Metadata{ Description: rapid.StringN(1, 100, 100).Draw(t, "desc"), DenomUnits: []*banktypes.DenomUnit{ { Denom: denom, Exponent: rapid.Uint32().Draw(t, "exponent"), Aliases: aliases, }, }, Base: denom, Display: denom, - Name: rapid.String().Draw(t, "name"), - Symbol: rapid.String().Draw(t, "symbol"), - URI: rapid.String().Draw(t, "uri"), - URIHash: rapid.String().Draw(t, "uri-hash"), + Name: rapid.StringN(1, 50, 50).Draw(t, "name"), + Symbol: rapid.StringN(1, 10, 10).Draw(t, "symbol"), + URI: rapid.StringN(1, 100, 100).Draw(t, "uri"), + URIHash: rapid.StringN(1, 64, 64).Draw(t, "uri-hash"), }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- tests/integration/v2/bank/determinisitic_test.go (1 hunks)
- testutil/testdata/grpc_query.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- testutil/testdata/grpc_query.go
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/bank/determinisitic_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (3)
tests/integration/v2/bank/determinisitic_test.go (3)
185-191:⚠️ Potential issueUndefined variable
addr1inTestQueryAllBalancesThe variable
addr1is used but not defined in theTestQueryAllBalancesfunction. This will lead to a compilation error.Define
addr1before using it. For example, you can declareaddr1as:+ addr1 := testdata.AddressGenerator(t).Draw(t, "address") fundAccount(f, addr1, coins...) addr1Str, err := addressCodec.BytesToString(addr1) require.NoError(t, err) req := banktypes.NewQueryAllBalancesRequest(addr1Str, nil, false)Likely invalid or redundant comment.
141-145:⚠️ Potential issueUndefined variable
addr1inTestQueryBalanceThe variable
addr1is used but not defined in theTestQueryBalancefunction. This will lead to a compilation error.Define
addr1before using it. For example, you can declareaddr1as:+ addr1 := testdata.AddressGenerator(t).Draw(t, "address") fundAccount(f, addr1, coin1) addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) require.NoError(t, err) req := banktypes.NewQueryBalanceRequest(addr1Str, coin1.GetDenom())Likely invalid or redundant comment.
230-238:⚠️ Potential issueUndefined variable
addr1inTestQuerySpendableBalancesThe variable
addr1is used but not defined in theTestQuerySpendableBalancesfunction. This will lead to a compilation error.Define
addr1before using it. You can declareaddr1as:+ addr1 := testdata.AddressGenerator(t).Draw(t, "address") err := banktestutil.FundAccount(f.ctx, f.bankKeeper, addr1, coins) require.NoError(t, err) addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) require.NoError(t, err) req := banktypes.NewQuerySpendableBalancesRequest(addr1Str, nil)Likely invalid or redundant comment.
| require.NotNil(t, app) | ||
| return &deterministicFixture{app: app, bankKeeper: bankKeeper, T: t} |
There was a problem hiding this comment.
Check for nil bankKeeper after app initialization
After initializing the app in initDeterministicFixture, it's important to check that bankKeeper is not nil to avoid potential nil pointer dereferences later.
Apply this diff to add the check:
require.NoError(t, err)
require.NotNil(t, app)
+require.NotNil(t, bankKeeper, "bankKeeper is nil after app initialization")
return &deterministicFixture{app: app, bankKeeper: bankKeeper, T: t}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require.NotNil(t, app) | |
| return &deterministicFixture{app: app, bankKeeper: bankKeeper, T: t} | |
| require.NotNil(t, app) | |
| require.NotNil(t, bankKeeper, "bankKeeper is nil after app initialization") | |
| return &deterministicFixture{app: app, bankKeeper: bankKeeper, T: t} |
| rapid.Check(t, func(rt *rapid.T) { | ||
| addr := testdata.AddressGenerator(rt).Draw(rt, "address") | ||
| coin := getCoin(rt) | ||
| fundAccount(f, addr, coin) | ||
|
|
||
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | ||
| require.NoError(t, err) | ||
|
|
||
| req := banktypes.NewQueryBalanceRequest(addrStr, coin.GetDenom()) | ||
|
|
||
| testdata.DeterministicIterationsV2(t, req, gasMeterFactory, queryFn, assertNonZeroGas, assertBalance(coin)) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Handle potential duplicate denominations in Rapid Check
In TestQueryBalance, when generating random coins using Rapid, there's a possibility of generating duplicate denominations, which might cause the test to skip frequently or produce unintended behavior.
Ensure that the denominations generated are unique or adjust the test logic to handle duplicates appropriately.
Consider updating the coin generation to ensure uniqueness:
rapid.Check(t, func(rt *rapid.T) {
addr := testdata.AddressGenerator(rt).Draw(rt, "address")
- coin := getCoin(rt)
+ denom := rapid.StringMatching(denomRegex).Draw(rt, "denom")
+ amount := math.NewInt(rapid.Int64Min(1).Draw(rt, "amount"))
+ coin := sdk.NewCoin(denom, amount)
fundAccount(f, addr, coin)
...Alternatively, if duplicates are acceptable, adjust the test to handle them without skipping.
Committable suggestion was skipped due to low confidence.
| func queryFnFactory[RequestT, ResponseT proto.Message]( | ||
| f *deterministicFixture, | ||
| ) func(RequestT) (ResponseT, error) { | ||
| return func(req RequestT) (ResponseT, error) { | ||
| res, err := f.app.Query(f.ctx, 0, req) | ||
| return res.(ResponseT), err | ||
| } |
There was a problem hiding this comment.
Handle potential panic due to unchecked type assertion
In the queryFnFactory function, the type assertion return res.(ResponseT), err on line 63 could cause a panic if res is not of the expected type ResponseT. To enhance robustness, consider using a type assertion with an ok check to handle unexpected types gracefully.
Apply this diff to safely perform the type assertion:
func queryFnFactory[RequestT, ResponseT proto.Message](
f *deterministicFixture,
) func(RequestT) (ResponseT, error) {
return func(req RequestT) (ResponseT, error) {
res, err := f.app.Query(f.ctx, 0, req)
+ if err != nil {
+ return nil, err
+ }
+ castedRes, ok := res.(ResponseT)
+ if !ok {
+ return nil, fmt.Errorf("unexpected response type: %T", res)
+ }
- return res.(ResponseT), err
+ return castedRes, nil
}
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func queryFnFactory[RequestT, ResponseT proto.Message]( | |
| f *deterministicFixture, | |
| ) func(RequestT) (ResponseT, error) { | |
| return func(req RequestT) (ResponseT, error) { | |
| res, err := f.app.Query(f.ctx, 0, req) | |
| return res.(ResponseT), err | |
| } | |
| func queryFnFactory[RequestT, ResponseT proto.Message]( | |
| f *deterministicFixture, | |
| ) func(RequestT) (ResponseT, error) { | |
| return func(req RequestT) (ResponseT, error) { | |
| res, err := f.app.Query(f.ctx, 0, req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| castedRes, ok := res.(ResponseT) | |
| if !ok { | |
| return nil, fmt.Errorf("unexpected response type: %T", res) | |
| } | |
| return castedRes, nil | |
| } | |
| } |
| f.bankKeeper.SetSendEnabled(f.ctx, coin1.Denom, false) | ||
| f.bankKeeper.SetSendEnabled(f.ctx, coin2.Denom, true) | ||
|
|
||
| req := &banktypes.QuerySendEnabledRequest{ | ||
| Denoms: []string{coin1.GetDenom(), coin2.GetDenom()}, | ||
| } | ||
|
|
||
| testdata.DeterministicIterationsV2(t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
| } |
There was a problem hiding this comment.
Avoid appending to allDenoms without concurrency control
In TestSendEnabled, the global slice allDenoms is appended to within a concurrent test (rapid.Check). This can lead to race conditions when tests are run in parallel.
Consider making allDenoms a local variable within the rapid.Check function to avoid potential race conditions.
Apply this diff:
allDenoms := []string{}
rapid.Check(t, func(rt *rapid.T) {
+ allDenoms := []string{}
count := rapid.IntRange(0, 10).Draw(rt, "count")
denoms := make([]string, 0, count)
for i := 0; i < count; i++ {
coin := banktypes.SendEnabled{
Denom: rapid.StringMatching(denomRegex).Draw(rt, "denom"),
Enabled: rapid.Bool().Draw(rt, "enabled-status"),
}
f.bankKeeper.SetSendEnabled(f.ctx, coin.Denom, coin.Enabled)
denoms = append(denoms, coin.Denom)
}
- allDenoms = append(allDenoms, denoms...)Alternatively, if allDenoms is needed outside the function, consider using thread-safe mechanisms like sync.Mutex or running the test sequentially.
Committable suggestion was skipped due to low confidence.
|
|
||
| initialSupply = initialSupply.Add(coins...) |
There was a problem hiding this comment.
Reset bank keeper's state after minting coins
In TestQueryTotalSupply, after minting coins in the rapid.Check, the minted coins are added to initialSupply, but the bank keeper's state is not reset between iterations. This could lead to accumulated state affecting subsequent test runs.
Consider resetting the bank keeper's state or re-initializing the fixture after each iteration to ensure test isolation.
Apply this change:
...
rapid.Check(t, func(rt *rapid.T) {
...
require.NoError(t, f.bankKeeper.MintCoins(f.ctx, minttypes.ModuleName, coins))
initialSupply = initialSupply.Add(coins...)
+ // Reset the bank keeper's state to ensure isolation between test runs
+ f = initDeterministicFixture(t)
+ f.ctx = f.app.StateLatestContext(t)
+ gasMeterFactory = integration.GasMeterFactory(f.ctx)
+ queryFn = queryFnFactory[*banktypes.QueryTotalSupplyRequest, *banktypes.QueryTotalSupplyResponse](f)
req := &banktypes.QueryTotalSupplyRequest{
Pagination: testdata.PaginationGenerator(rt, uint64(len(initialSupply))).Draw(rt, "pagination"),
}
...Committable suggestion was skipped due to low confidence.
Description
This PR creates integration tests for x/bank in server/v2 architecture by doing the following:
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
DefaultProvidersfor improved dependency injection.StoreBuilderfor enhanced store management.Bug Fixes
Tests