-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(integration): port x/bank tests to server/v2 app #21912
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we still add the APIs to execute a message/messages without a tx, basically being directly routed to the handlers? That is as well what the integration framework could do (the other one)
Is there an example usage you can point to that the API should fulfill? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
tests/integration/v2/app.go (1)
46-51
: Consider using a more descriptive constant name.The constant
DefaultGenTxGas
is clear and descriptive. However, the constantsGenesis_COMMIT
,Genesis_NOCOMMIT
, andGenesis_SKIP
could benefit from more descriptive names to improve readability and maintainability. Consider prefixing them withGenesisMode_
or a similar prefix to indicate their purpose more clearly.-const ( - Genesis_COMMIT = iota - Genesis_NOCOMMIT - Genesis_SKIP -) +const ( + GenesisMode_COMMIT = iota + GenesisMode_NOCOMMIT + GenesisMode_SKIP +)tests/integration/v2/bank/determinisitic_test.go (2)
285-288
: Avoid redundant re-initialization of fixtureIn
TestQueryTotalSupply
, after the rapid check, the fixture is re-initialized. Since the test is already isolated per iteration, re-initializing here might be unnecessary and could mask issues that arise from state changes during tests.Consider removing the re-initialization unless it's specifically required:
-f = initDeterministicFixture(t) // reset -f.ctx = f.app.StateLatestContext(t) -gasMeterFactory = integration.GasMeterFactory(f.ctx) -queryFn = queryFnFactory[*banktypes.QueryTotalSupplyRequest, *banktypes.QueryTotalSupplyResponse](f)
345-346
: Handle error fromSetParams
instead of just requiring no errorIn
TestQueryParams
, the error fromf.bankKeeper.SetParams
is checked usingrequire.NoError
, which will halt the test on failure. Consider handling or logging the error to provide more context if the test fails.Alternatively, ensure that the error provides sufficient context:
err := f.bankKeeper.SetParams(f.ctx, params) -require.NoError(t, err) +require.NoErrorf(t, err, "failed to set bank params: %v", err)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/go.sum
is excluded by!**/*.sum
📒 Files selected for processing (5)
simapp/v2/app_di.go
(1 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/bank/determinisitic_test.go
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/go.mod
🧰 Additional context used
📓 Path-based instructions (4)
simapp/v2/app_di.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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/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 (25)
simapp/v2/app_di.go (1)
51-51
: Add a comment explaining DefaultProviders.
The addition of codec.DefaultProviders
improves code organization by centralizing codec-related dependency injection. However, a brief comment would help explain its purpose and the providers it includes.
Add a comment above this line:
+// DefaultProviders includes standard codec providers (InterfaceRegistry, Amino, ProtoCodec, AddressCodec)
codec.DefaultProviders,
Let's verify that this change doesn't break existing codec usage:
✅ Verification successful
Let me verify the usage of codec providers by checking where DefaultProviders
is used to ensure there are no conflicts.
The addition of codec.DefaultProviders
is safe and follows the intended usage pattern
The verification shows that:
DefaultProviders
is used consistently in both test and app configurations- Individual codec providers (
ProvideInterfaceRegistry
,ProvideLegacyAmino
, etc.) are properly bundled inDefaultProviders
- No conflicts exist between
DefaultProviders
and individual provider usage, as direct provider usage occurs in test utilities and the codec package itself
The suggestion to add a comment remains valid for better code documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of individual codec providers that might conflict
rg -A 2 "ProvideInterfaceRegistry|ProvideLegacyAmino|ProvideProtoCodec|ProvideAddressCodec" --type go
Length of output: 2686
Script:
#!/bin/bash
# Search for DefaultProviders usage
rg -A 2 "DefaultProviders" --type go
Length of output: 473
tests/integration/v2/app.go (13)
1-44
: LGTM!
The package declaration and imports are correctly defined. The package name integration
is appropriate for an integration testing package. The imported packages are relevant and necessary for the functionality provided in this file.
53-53
: LGTM!
The type alias stateMachineTx
is appropriately defined and improves code readability by providing a more meaningful name for the transaction.Tx
type.
55-76
: LGTM!
The DefaultConsensusParams
variable is correctly defined with appropriate values for each consensus parameter. The comments provide clear explanations of the purpose and usage of these default parameters.
78-90
: LGTM!
The StartupConfig
struct is well-defined with clear field names and comments explaining the purpose of each field. The struct encapsulates the necessary configuration options for initializing a new test application.
92-116
: LGTM!
The DefaultStartUpConfig
function is implemented correctly. It generates a default StartupConfig
with appropriate values for testing purposes. The function uses helper functions to create a random validator set, generates a genesis account with a default balance, and sets up a temporary home directory for the application.
277-283
: LGTM!
The App
struct is correctly defined as a wrapper around runtime.App
with additional testing utilities. The struct fields are appropriately named and typed, providing access to the underlying application, store, transaction config, and last block height.
302-310
: LGTM!
The StateLatestContext
method is implemented correctly. It retrieves the latest state from the store, creates a writable branch, and returns a new context with the state value set. This method is useful for obtaining a context with the latest state for testing purposes.
312-320
: LGTM!
The Commit
method is implemented correctly. It retrieves the state changes from the given writable state, creates a changeset, and commits the changes to the store. The method returns the new state hash or an error if the commit fails.
322-397
: Verify the function signature change in the codebase.
The SignCheckDeliver
method is quite complex and involves several steps for signing and delivering transactions. While the implementation looks correct, it's important to verify that the function signature change doesn't break any existing code that relies on this method.
Run the following script to verify the function usage:
#!/bin/bash
# Description: Verify all calls to `SignCheckDeliver` match the new signature.
# Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg -A 5 $'SignCheckDeliver'
This script searches for all occurrences of the SignCheckDeliver
method and prints the surrounding lines. Review the output to ensure that all calls to this method have been updated to match the new signature, especially regarding the txErrString
parameter.
399-406
: LGTM!
The CheckBalance
method is implemented correctly. It retrieves the balances of the given address using the provided bank keeper and asserts that the balances match the expected coins. This method is useful for verifying account balances in integration tests.
240-243
:
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,
},
251-251
:
Avoid using time.Now()
directly in tests for determinism.
Using time.Now()
can introduce non-determinism in tests. Consider providing a fixed time or allowing the time to be injected, ensuring that test runs are consistent.
Apply this diff to use a fixed time:
- Time: time.Now(),
+ Time: fixedTime,
Define fixedTime
in your test setup:
fixedTime := time.Date(2024, time.January, 1, 0, 0, 0, 0, time.UTC)
285-300
: 🛠️ Refactor suggestion
Consider adding a helper DeliverAndCommit
method.
The Deliver
method is implemented correctly and provides a convenient way to deliver a block with the given transactions. However, it doesn't automatically commit the resulting state, which is a common pattern in integration tests.
Consider adding a new helper method DeliverAndCommit
that combines the functionality of Deliver
and Commit
:
func (a *App) DeliverAndCommit(
t *testing.T, ctx context.Context, txs []stateMachineTx,
) (*server.BlockResponse, []byte) {
t.Helper()
resp, state := a.Deliver(t, ctx, txs)
hash, err := a.Commit(state)
require.NoError(t, err)
return resp, hash
}
This method would simplify the usage in integration tests by automatically committing the state after delivering the block.
tests/integration/v2/bank/app_test.go (7)
106-143
: LGTM!
The TestSendNotEnoughBalance
test case is well-structured and covers the scenario of sending coins with insufficient balance. It verifies the expected behavior and checks the account balances and sequences after the transaction.
145-237
: LGTM!
The TestMsgMultiSendWithAccounts
test case thoroughly covers various scenarios for the MsgMultiSend
message type. It includes test cases for valid transactions, wrong account numbers, wrong account sequences, and multiple inputs. The test cases are well-defined and cover the expected behavior and error conditions.
239-291
: LGTM!
The TestMsgMultiSendMultipleOut
test case covers the scenario of sending coins to multiple output addresses using the MsgMultiSend
message type. It sets up the necessary accounts, funds them, and verifies the expected balances after the transaction. The test case is well-structured and covers the desired functionality.
293-351
: LGTM!
The TestMsgMultiSendDependent
test case covers the scenario of sending coins between dependent accounts using the MsgMultiSend
message type. It sets up the necessary accounts with different account numbers, funds them, and verifies the expected balances after the transactions. The test case is well-structured and covers the desired functionality.
353-430
: LGTM!
The TestMsgSetSendEnabled
test case covers the functionality of the MsgSetSendEnabled
message type. It includes test cases for wrong authority, right authority with wrong signer, and submitting a valid governance proposal. The test cases are well-defined and cover the expected behavior and error conditions.
432-469
: LGTM!
The TestSendToNonExistingAccount
test case covers the scenario of sending coins to a non-existing account. It verifies that the account is not created when sending coins to it, but the balance is updated. It also checks that the account is created when sending coins back from the non-existing account. The test case is well-structured and covers the desired functionality.
40-40
:
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)
Likely invalid or redundant comment.
tests/integration/v2/bank/determinisitic_test.go (4)
523-536
: Ensure addresses are valid and unique in TestDenomOwners
In TestDenomOwners
, random addresses are generated, but there could be a risk of duplicates or invalid addresses, which might affect the test outcome.
Add a check to ensure that addresses are unique and valid. Run the following script to verify uniqueness:
#!/bin/bash
# Description: Check for duplicate addresses in TestDenomOwners
# Expected: No duplicate addresses in the generated list
awk '/addr := testdata.AddressGenerator/ {print $0}' tests/integration/v2/bank/deterministic_test.go | sort | uniq -d
215-216
:
Avoid potential panic due to duplicate denominations
In TestQuerySpendableBalances
, using rapid.StringMatching(denomRegex)
directly might generate duplicate denominations, leading to a panic when creating sdk.NewCoins
. Ensure that the generated denominations are unique.
Use rapid.SliceOfNDistinct
to guarantee unique denominations:
- denoms := rapid.SliceOf(rapid.StringMatching(denomRegex)).Filter(func(denoms []string) bool {
- return len(denoms) >= 1 && len(denoms) <= 10
- }).Draw(rt, "denoms")
+ denoms := rapid.SliceOfNDistinct(
+ rapid.StringMatching(denomRegex),
+ 1,
+ 10,
+ rapid.ID[string],
+ ).Draw(rt, "denoms")
Likely invalid or redundant comment.
63-71
:
Improve type assertion to prevent potential panic
In the queryFnFactory
function, the type assertion could cause a panic if res
is not of the expected type ResponseT
. To enhance robustness and prevent potential runtime errors, consider introducing an ok
check for the type assertion.
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) {
- var emptyResponse ResponseT
res, err := f.app.Query(f.ctx, 0, req)
if err != nil {
- return emptyResponse, err
+ var zero ResponseT
+ return zero, err
}
- castedRes, ok := res.(ResponseT)
+ castedRes, ok := res.(ResponseT)
if !ok {
- return emptyResponse, fmt.Errorf("unexpected response type: %T", res)
+ return zero, fmt.Errorf("unexpected response type: %T", res)
}
return castedRes, nil
}
}
Likely invalid or redundant comment.
380-395
: Validate metadata fields to ensure proper test coverage
In createAndReturnMetadatas
, random strings are generated for metadata fields without validation. This could lead to invalid metadata, especially if empty strings or invalid formats are generated.
Ensure that the generated metadata fields meet any required validation criteria. You can add validation logic or constraints to the random generation. Run the following script to check for empty or invalid fields:
} | ||
|
||
store := storeBuilder.Get() | ||
if store == nil { | ||
return nil, fmt.Errorf("failed to build store: %w", err) | ||
} | ||
err = store.SetInitialVersion(1) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to set initial version: %w", err) | ||
} | ||
|
||
integrationApp := &App{App: app, Store: store, txConfig: txConfig, lastHeight: 1} | ||
if startupConfig.GenesisBehavior == Genesis_SKIP { | ||
return integrationApp, nil | ||
} | ||
|
||
// create validator set | ||
valSet, err := startupConfig.ValidatorSet() | ||
if err != nil { | ||
return nil, errors.New("failed to create validator set") | ||
} | ||
|
||
var ( | ||
balances []banktypes.Balance | ||
genAccounts []authtypes.GenesisAccount | ||
) | ||
for _, ga := range startupConfig.GenesisAccounts { | ||
genAccounts = append(genAccounts, ga.GenesisAccount) | ||
balances = append( | ||
balances, | ||
banktypes.Balance{ | ||
Address: ga.GenesisAccount.GetAddress().String(), | ||
Coins: ga.Coins, | ||
}, | ||
) | ||
} | ||
|
||
genesisJSON, err := genesisStateWithValSet( | ||
cdc, | ||
app.DefaultGenesis(), | ||
valSet, | ||
genAccounts, | ||
balances...) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to create genesis state: %w", err) | ||
} | ||
|
||
// init chain must be called to stop deliverState from being nil | ||
genesisJSONBytes, err := cmtjson.MarshalIndent(genesisJSON, "", " ") | ||
if err != nil { | ||
return nil, fmt.Errorf( | ||
"failed to marshal default genesis state: %w", | ||
err, | ||
) | ||
} | ||
|
||
ctx := context.WithValue( | ||
context.Background(), | ||
corecontext.CometParamsInitInfoKey, | ||
&consensustypes.MsgUpdateParams{ | ||
Authority: "consensus", | ||
Block: DefaultConsensusParams.Block, | ||
Evidence: DefaultConsensusParams.Evidence, | ||
Validator: DefaultConsensusParams.Validator, | ||
Abci: DefaultConsensusParams.Abci, | ||
Synchrony: DefaultConsensusParams.Synchrony, | ||
Feature: DefaultConsensusParams.Feature, | ||
}, | ||
) | ||
|
||
emptyHash := sha256.Sum256(nil) | ||
_, genesisState, err := app.InitGenesis( | ||
ctx, | ||
&server.BlockRequest[stateMachineTx]{ | ||
Height: 1, | ||
Time: time.Now(), | ||
Hash: emptyHash[:], | ||
ChainId: "test-chain", | ||
AppHash: emptyHash[:], | ||
IsGenesis: true, | ||
}, | ||
genesisJSONBytes, | ||
&genesisTxCodec{txConfigOptions}, | ||
) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed init genesis: %w", err) | ||
} | ||
|
||
if startupConfig.GenesisBehavior == Genesis_NOCOMMIT { | ||
integrationApp.lastHeight = 0 | ||
return integrationApp, nil | ||
} | ||
|
||
_, err = integrationApp.Commit(genesisState) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to commit initial version: %w", err) | ||
} | ||
|
||
return integrationApp, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Error handling is properly implemented with one minor inconsistency
The error handling in the NewApp
function is generally well-implemented with proper error wrapping and propagation, but there is one inconsistency:
- At line 147, the error is returned without wrapping:
return nil, errors.New("failed to create validator set")
while there's anerr
variable available that should be wrapped.
All other error cases properly:
- Use descriptive error messages
- Wrap the underlying error using
%w
- Return both nil and the error
- Follow consistent error message formatting
🔗 Analysis chain
Verify error handling and propagation.
The NewApp
function is quite complex and involves multiple steps for initializing a new application. While the error handling seems to be in place, it's important to verify that all potential errors are properly handled and propagated to the caller.
To verify the error handling, you can use the following script:
This script checks if all errors are handled and returned using fmt.Errorf
, and if all errors are propagated to the caller using %w
in the error format string. If any of these checks fail, it indicates that there might be some errors that are not properly handled or propagated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all errors are handled and returned
if ! rg -q -e 'return nil, fmt.Errorf'; then
echo "Not all errors are handled and returned"
exit 1
fi
# Check if all errors are propagated to the caller
if ! rg -q -e 'return nil, fmt.Errorf("failed to.*: %w", err)'; then
echo "Not all errors are propagated to the caller"
exit 1
fi
echo "Error handling and propagation looks good"
Length of output: 201
Script:
#!/bin/bash
# Let's check all error handling patterns in the NewApp function
rg -A 1 "return nil, " "tests/integration/v2/app.go"
# Also check for any direct error returns without wrapping
rg -A 1 "return nil, err" "tests/integration/v2/app.go"
# Check for any other error handling patterns we might have missed
rg -A 1 "if err [!:]" "tests/integration/v2/app.go"
Length of output: 1763
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@octavio12345300, understood.
if exists, _ := coins.Find(coin.Denom); exists { | ||
t.Skip("duplicate denom") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid skipping tests due to duplicate denominations
In TestQueryAllBalances
, the test skips when a duplicate denomination is found. Skipping tests within deterministic iterations can reduce test coverage and may hide potential issues. Instead of skipping, consider regenerating the coin or ensuring denominations are unique from the start.
Refactor the coin generation to ensure uniqueness without needing to skip:
for i := 0; i < numCoins; i++ {
- coin := getCoin(rt)
- if exists, _ := coins.Find(coin.Denom); exists {
- t.Skip("duplicate denom")
- }
+ denom := rapid.StringMatching(denomRegex).Filter(func(d string) bool {
+ _, exists := coins.Find(d)
+ return !exists
+ }).Draw(rt, "denom")
+ coin := sdk.NewCoin(denom, math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")))
// NewCoins sorts the denoms
coins = sdk.NewCoins(append(coins, coin)...)
}
Committable suggestion skipped: line range outside the PR's diff.
addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | ||
require.NoError(t, err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential error from address conversion
In TestQueryBalance
, the error from BytesToString(addr)
is being checked, but the test continues even if there's an error. This could lead to unexpected behavior if addrStr
is not valid. Ensure that the test exits or handles the error appropriately upon failure.
Modify the error handling to fail the test if addrStr
is not obtained:
addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr)
if err != nil {
- require.NoError(t, err)
+ t.Fatalf("failed to convert address bytes to string: %v", err)
}
Committable suggestion skipped: line range outside the PR's diff.
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...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent race conditions by avoiding shared mutable state
In TestSendEnabled
, the allDenoms
slice is shared across parallel tests, which can lead to race conditions when appending to it concurrently.
Make allDenoms
a local variable within the test function to prevent concurrent access:
func TestSendEnabled(t *testing.T) {
t.Parallel()
f := initDeterministicFixture(t)
f.ctx = f.app.StateLatestContext(t)
gasMeterFactory := integration.GasMeterFactory(f.ctx)
queryFn := queryFnFactory[*banktypes.QuerySendEnabledRequest, *banktypes.QuerySendEnabledResponse](f)
- 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...)
Committable suggestion skipped: line range outside the PR's diff.
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...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reset state between iterations to prevent state leakage
In TestQueryTotalSupply
, the initialSupply
is accumulated across iterations, which can cause tests to depend on state from previous runs. This may lead to flaky tests or incorrect assertions.
Reset initialSupply
at the beginning of each iteration:
rapid.Check(t, func(rt *rapid.T) {
+ res, err := queryFn(&banktypes.QueryTotalSupplyRequest{})
+ require.NoError(t, err)
+ initialSupply := res.GetSupply()
+
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...)
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@octavio12345300, the new unit tests in deterministic_test.go
provide comprehensive coverage of the bank module's query functionalities. The use of deterministic iterations and rapid testing ensures robustness across various scenarios. Great job on enhancing the test coverage!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gracias
@@ -440,3 +441,71 @@ func NewAppConfig(opts ...ModuleOption) depinject.Config { | |||
|
|||
return appconfig.Compose(&appv1alpha1.Config{Modules: modules}) | |||
} | |||
|
|||
func NewAppV2Config(opts ...ModuleOption) depinject.Config { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put a go.mod in that tests/integration/v2 package and move that there too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit like we do for system tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
@sontrinh16 will probably do a follow with what is missing
Could you check this: https://github.com/cosmos/cosmos-sdk/actions/runs/11632561253/job/32395923816?pr=21912 |
should the store interface in runtime v2 has commit func ? or we could just use store v2 store interface |
@kocubinski can i push in this PR, i can help finish this PR |
Feel free to open a PR targeting this branch |
It looks like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/integration/v2/app.go (1)
54-77
: Consider movingDefaultConsensusParams
to a separate configuration file.While having
DefaultConsensusParams
defined here is acceptable, consider moving it to a separate configuration file (e.g.,config.go
) to keep the main application file focused on the core logic.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/go.sum
is excluded by!**/*.sum
📒 Files selected for processing (2)
tests/go.mod
(5 hunks)tests/integration/v2/app.go
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
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"
🔇 Additional comments (10)
tests/integration/v2/app.go (10)
1-52
: LGTM!
The package declaration, imports, and constants look good. The constants are well-defined and follow the naming conventions.
79-91
: LGTM!
The StartupConfig
struct is well-defined with clear comments explaining each field. The field names are descriptive and follow the naming conventions.
93-117
: LGTM!
The DefaultStartUpConfig
function is implemented correctly. It generates a default configuration with a random validator set, genesis accounts, and a temporary home directory for testing.
278-284
: LGTM!
The App
struct is a wrapper around runtime.App
that provides additional testing utilities. The struct fields are well-defined and follow the naming conventions.
286-301
: LGTM!
The Deliver
method is implemented correctly. It delivers a block with the given transactions, updates the application state, and returns the block response and the resulting state.
303-311
: LGTM!
The StateLatestContext
method is implemented correctly. It creates a new context with the latest application state for testing purposes.
313-321
: LGTM!
The Commit
method is implemented correctly. It commits the given state changes and returns the new state hash.
400-407
: LGTM!
The CheckBalance
method is implemented correctly. It checks the balance of the given address against the expected balance using the bank keeper.
119-276
: Verify error handling and propagation.
The NewApp
function is quite complex and involves multiple steps for initializing a new application. While the error handling seems to be in place, it's important to verify that all potential errors are properly handled and propagated to the caller.
Run the following script to verify the error handling:
✅ Verification successful
Error handling is properly implemented with clear error wrapping
The error handling in the NewApp
function is well-implemented:
- All error cases return both nil and a wrapped error
- Error messages are descriptive and follow a consistent "failed to X" pattern
- Errors are properly wrapped using
fmt.Errorf
with%w
verb for error chain preservation - One case uses
errors.New
which is appropriate as it's a new error without wrapping
The only exception is the marshal default genesis state
error which spans multiple lines but still follows the same pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all errors are handled and returned
if ! rg -q -e 'return nil, fmt.Errorf'; then
echo "Not all errors are handled and returned"
exit 1
fi
# Check if all errors are propagated to the caller
if ! rg -q -e 'return nil, fmt.Errorf("failed to.*: %w", err)'; then
echo "Not all errors are propagated to the caller"
exit 1
fi
echo "Error handling and propagation looks good"
Length of output: 201
Script:
#!/bin/bash
# Let's examine all error handling patterns in the NewApp function
rg -A 2 "if err.*{" "tests/integration/v2/app.go"
# Check for direct error returns without wrapping
rg "return.*err[^o]" "tests/integration/v2/app.go"
# Check for all error handling patterns
rg "return nil, (fmt\.Errorf|errors\.|err)" "tests/integration/v2/app.go"
Length of output: 1473
323-398
: Verify the correctness of the signing process.
The SignCheckDeliver
method is responsible for signing and delivering a set of messages. It's crucial to ensure that the signing process is implemented correctly and follows the expected behavior.
Run the following script to verify the signing process:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
tests/integration/v2/bank/determinisitic_test.go (7)
59-74
: Enhance error handling in queryFnFactoryThe
queryFnFactory
function is well-implemented, but it could benefit from improved error handling. Consider returning the error immediately iff.app.Query
returns an error, and use a type assertion with anok
check to handle unexpected response types gracefully.Apply this diff to enhance the error handling:
func queryFnFactory[RequestT, ResponseT proto.Message]( f *deterministicFixture, ) func(RequestT) (ResponseT, error) { return func(req RequestT) (ResponseT, error) { var emptyResponse ResponseT res, err := f.app.Query(f.ctx, 0, req) if err != nil { + return emptyResponse, err } castedRes, ok := res.(ResponseT) if !ok { return emptyResponse, fmt.Errorf("unexpected response type: %T", res) } return castedRes, nil } }
123-156
: Enhance test coverage for balance queriesThe
TestQueryBalance
function provides a good starting point for testing balance queries. To further improve test coverage, consider the following suggestions:
- Test querying balances for multiple accounts with different denominations.
- Test querying balances for non-existent accounts or denominations.
- Verify that the returned balance matches the expected balance after performing transactions (e.g., transfers, minting, burning).
These additional test cases will help ensure the robustness of the balance querying functionality.
301-325
: Add assertions to TestQueryTotalSupplyOfThe
TestQueryTotalSupplyOf
function is well-structured, but it lacks specific assertions on the returned supply.Consider adding an assertion to verify the correctness of the returned supply:
testdata.DeterministicIterationsV2( - t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) + t, req, gasMeterFactory, queryFn, 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) + })This assertion will ensure that the supply returned by the query matches the minted amount for the specific denomination.
327-366
: Add assertions to TestQueryParamsThe
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, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) + t, req, gasMeterFactory, queryFn, 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.
403-437
: Enhance metadata-related test assertionsThe
TestDenomsMetadata
function is well-implemented, but it could benefit from more specific assertions to ensure the correctness of the returned metadata.Consider adding the following assertion:
testdata.DeterministicIterationsV2( - t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) + t, req, gasMeterFactory, queryFn, 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) + } + })This addition will ensure that the returned metadata values match the expected metadata set in the test.
439-465
: Enhance metadata-related test assertionsThe
TestDenomMetadata
function is well-implemented, but it could benefit from more specific assertions to ensure the correctness of the returned metadata.Consider adding the following assertion:
testdata.DeterministicIterationsV2( - t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) + t, req, gasMeterFactory, queryFn, assertNonZeroGas, func(t *testing.T, res *banktypes.QueryDenomMetadataResponse) { + require.Equal(t, denomMetadata[0], res.Metadata, "returned metadata should match the set metadata") + })This addition will ensure that the returned metadata value matches the expected metadata set in the test.
518-570
: Enhance assertions in DenomOwners testThe
TestDenomOwners
function is well-implemented, but it could benefit from more specific assertions to ensure the correctness of the returned data.Consider adding the following assertion:
testdata.DeterministicIterationsV2( - t, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) + t, req, gasMeterFactory, queryFn, 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) + } + })This addition will ensure that the returned DenomOwners data matches the expected values set in the test.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
tests/integration/v2/app.go
(1 hunks)tests/integration/v2/bank/determinisitic_test.go
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/v2/app.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 (2)
tests/integration/v2/bank/determinisitic_test.go (2)
88-116
: LGTM!
The initDeterministicFixture
function is well-structured and sets up the necessary components for the deterministic fixture. It initializes the mock account keeper, configures the app, and returns the fixture with the required fields.
203-248
: LGTM!
The TestQuerySpendableBalances
function is well-implemented and tests the querying of spendable balances. It generates random test cases using rapid testing and also includes a fixed test case. The use of FundAccount
and DeterministicIterationsV2
ensures thorough testing of the functionality.
func TestQueryTotalSupply(t *testing.T) { | ||
t.Parallel() | ||
f := initDeterministicFixture(t) | ||
f.ctx = f.app.StateLatestContext(t) | ||
gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
queryFn := queryFnFactory[*banktypes.QueryTotalSupplyRequest, *banktypes.QueryTotalSupplyResponse](f) | ||
|
||
res, err := queryFn(&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, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
}) | ||
|
||
f = initDeterministicFixture(t) // reset | ||
f.ctx = f.app.StateLatestContext(t) | ||
gasMeterFactory = integration.GasMeterFactory(f.ctx) | ||
queryFn = queryFnFactory[*banktypes.QueryTotalSupplyRequest, *banktypes.QueryTotalSupplyResponse](f) | ||
|
||
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, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reset state between iterations to prevent state leakage
In the TestQueryTotalSupply
function, the initialSupply
is accumulated across iterations, which can cause tests to depend on state from previous runs. This may lead to flaky tests or incorrect assertions.
Reset initialSupply
at the beginning of each iteration:
rapid.Check(t, func(rt *rapid.T) {
+ res, err := queryFn(&banktypes.QueryTotalSupplyRequest{})
+ require.NoError(t, err)
+ initialSupply := res.GetSupply()
+
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...)
Committable suggestion skipped: line range outside the PR's diff.
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() | ||
queryFn := queryFnFactory[*banktypes.QueryAllBalancesRequest, *banktypes.QueryAllBalancesResponse](f) | ||
|
||
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, req, gasMeterFactory, queryFn, 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, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid skipping tests due to duplicate denominations
In the TestQueryAllBalances
function, the test skips when a duplicate denomination is found. Skipping tests within deterministic iterations can reduce test coverage and may hide potential issues. Instead of skipping, consider regenerating the coin or ensuring denominations are unique from the start.
Refactor the coin generation to ensure uniqueness without needing to skip:
for i := 0; i < numCoins; i++ {
- coin := getCoin(rt)
- if exists, _ := coins.Find(coin.Denom); exists {
- t.Skip("duplicate denom")
- }
+ denom := rapid.StringMatching(denomRegex).Filter(func(d string) bool {
+ _, exists := coins.Find(d)
+ return !exists
+ }).Draw(rt, "denom")
+ coin := sdk.NewCoin(denom, math.NewInt(rapid.Int64Min(1).Draw(rt, "amount")))
// NewCoins sorts the denoms
coins = sdk.NewCoins(append(coins, coin)...)
}
Committable suggestion skipped: line range outside the PR's diff.
func TestSendEnabled(t *testing.T) { | ||
t.Parallel() | ||
f := initDeterministicFixture(t) | ||
f.ctx = f.app.StateLatestContext(t) | ||
gasMeterFactory := integration.GasMeterFactory(f.ctx) | ||
queryFn := queryFnFactory[*banktypes.QuerySendEnabledRequest, *banktypes.QuerySendEnabledResponse](f) | ||
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, req, gasMeterFactory, queryFn, 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, req, gasMeterFactory, queryFn, assertNonZeroGas, nil) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent race conditions by avoiding shared mutable state
In the TestSendEnabled
function, the allDenoms
slice is shared across parallel tests, which can lead to race conditions when appending to it concurrently.
Make allDenoms
a local variable within the test function to prevent concurrent access:
func TestSendEnabled(t *testing.T) {
t.Parallel()
f := initDeterministicFixture(t)
f.ctx = f.app.StateLatestContext(t)
gasMeterFactory := integration.GasMeterFactory(f.ctx)
queryFn := queryFnFactory[*banktypes.QuerySendEnabledRequest, *banktypes.QuerySendEnabledResponse](f)
- 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...)
Committable suggestion skipped: line range outside the PR's diff.
(cherry picked from commit b5993d6) # Conflicts: # server/v2/appmanager/appmanager.go # server/v2/stf/core_header_service.go # tests/go.mod
… (#22457) Co-authored-by: Matt Kocubinski <[email protected]> Co-authored-by: Julien Robert <[email protected]>
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.md
Reviewers 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
Bug Fixes
Tests
Chores