Skip to content
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

Merged
merged 84 commits into from
Nov 6, 2024

Conversation

kocubinski
Copy link
Member

@kocubinski kocubinski commented Sep 25, 2024

Description

This PR creates integration tests for x/bank in server/v2 architecture by doing the following:

  • Create a server/v2 integration test fixture
  • Port x/bank integration tests to use that fixture

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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Introduced a global variable for improved dependency injection.
    • Added functionality for managing the genesis state in blockchain integration.
    • Implemented a comprehensive testing framework for the Cosmos SDK application.
  • Bug Fixes

    • Enhanced error handling and control flow in the app manager's query method.
  • Tests

    • Added a suite of integration tests for the bank module, covering various transaction scenarios and query functionalities.
    • Established a robust testing framework for testing Cosmos SDK applications.
  • Chores

    • Updated dependencies to improve module structure and functionality.

Copy link
Member

@julienrbrt julienrbrt left a 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)

@kocubinski
Copy link
Member Author

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?

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 constants Genesis_COMMIT, Genesis_NOCOMMIT, and Genesis_SKIP could benefit from more descriptive names to improve readability and maintainability. Consider prefixing them with GenesisMode_ 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 fixture

In 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 from SetParams instead of just requiring no error

In TestQueryParams, the error from f.bankKeeper.SetParams is checked using require.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

📥 Commits

Reviewing files that changed from the base of the PR and between 862b351 and 83333fa.

⛔ 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 in DefaultProviders
  • 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: ⚠️ Potential issue

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: ⚠️ Potential issue

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: ⚠️ Potential issue

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: ⚠️ Potential issue

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: ⚠️ Potential issue

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:

Comment on lines +118 to +275
}

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
}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024

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 an err 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@octavio12345300, understood.

Comment on lines +176 to +178
if exists, _ := coins.Find(coin.Denom); exists {
t.Skip("duplicate denom")
}
Copy link
Contributor

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.

Comment on lines +143 to +145
addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr)
require.NoError(t, err)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Comment on lines +471 to +487
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...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Comment on lines +262 to +276
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...)
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__

Copy link
Contributor

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!

Copy link

@octavio12345300 octavio12345300 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gracias

@julienrbrt julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Nov 4, 2024
@@ -440,3 +441,71 @@ func NewAppConfig(opts ...ModuleOption) depinject.Config {

return appconfig.Compose(&appv1alpha1.Config{Modules: modules})
}

func NewAppV2Config(opts ...ModuleOption) depinject.Config {
Copy link
Member

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

Copy link
Member

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

Copy link
Member

@julienrbrt julienrbrt left a 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

@julienrbrt
Copy link
Member

@sontrinh16
Copy link
Member

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

@sontrinh16
Copy link
Member

@kocubinski can i push in this PR, i can help finish this PR

@julienrbrt
Copy link
Member

@kocubinski can i push in this PR, i can help finish this PR

Feel free to open a PR targeting this branch

@kocubinski
Copy link
Member Author

Could you check this: https://github.com/cosmos/cosmos-sdk/actions/runs/11632561253/job/32395923816?pr=21912

It looks like runtime.Store has the commit method removed since I made this PR, refactoring to see if I make it work.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 moving DefaultConsensusParams 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

📥 Commits

Reviewing files that changed from the base of the PR and between 83333fa and 7c32d2f.

⛔ 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:

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 queryFnFactory

The queryFnFactory function is well-implemented, but it could benefit from improved error handling. Consider returning the error immediately if f.app.Query returns an error, and use a type assertion with an ok 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 queries

The 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 TestQueryTotalSupplyOf

The 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 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, 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 assertions

The 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 assertions

The 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 test

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7c32d2f and 7e5f838.

📒 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.

Comment on lines +250 to +299
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)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Comment on lines +158 to +201
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)
}
Copy link
Contributor

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.

Comment on lines +467 to +516
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)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

@julienrbrt julienrbrt added this pull request to the merge queue Nov 6, 2024
Merged via the queue into main with commit b5993d6 Nov 6, 2024
75 of 76 checks passed
@julienrbrt julienrbrt deleted the kocu/v2-integration-tests branch November 6, 2024 19:02
mergify bot pushed a commit that referenced this pull request Nov 6, 2024
(cherry picked from commit b5993d6)

# Conflicts:
#	server/v2/appmanager/appmanager.go
#	server/v2/stf/core_header_service.go
#	tests/go.mod
julienrbrt added a commit that referenced this pull request Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release C:server/v2 appmanager C:server/v2 stf C:server/v2 Issues related to server/v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants