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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(runtime/v2): rm singleton storebuilder scope #22204

Closed
wants to merge 2 commits into from
Closed

refactor(runtime/v2): rm singleton storebuilder scope #22204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
390d97f
refactor(runtime/v2): rm singleton storebuilder scope
kocubinski Oct 9, 2024
712d8c3
Merge branch 'main' into kocu/rm-singleton-db
kocubinski Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions runtime/v2/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,35 +3,13 @@ package runtime
import (
"errors"
"fmt"
"sync"

"cosmossdk.io/core/store"
"cosmossdk.io/server/v2/stf"
storev2 "cosmossdk.io/store/v2"
"cosmossdk.io/store/v2/proof"
"cosmossdk.io/store/v2/root"
)

var (
storeBuilderSingleton root.Builder
storeBuilderSingletonOnce sync.Once
)

// ProvideSingletonScopedStoreBuilder returns a store builder that is a singleton
// in the scope of the process lifetime.
func ProvideSingletonScopedStoreBuilder() root.Builder {
storeBuilderSingletonOnce.Do(func() {
storeBuilderSingleton = root.NewBuilder()
})
return storeBuilderSingleton
}

// ResetSingletonScopedStoreBuilder resets the singleton store builder. Applications
// should not ever need to call this, but it may be useful in tests.
func ResetSingletonScopedStoreBuilder() {
storeBuilderSingletonOnce = sync.Once{}
}

// NewKVStoreService creates a new KVStoreService.
// This wrapper is kept for backwards compatibility.
// When migrating from runtime to runtime/v2, use runtimev2.NewKVStoreService(storeKey.Name()) instead of runtime.NewKVStoreService(storeKey).
Expand Down
10 changes: 5 additions & 5 deletions simapp/v2/app_di.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Here storeBuilder is injected and supplied. We should remove the inject one.

Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ func AppConfig() depinject.Config {
codec.ProvideAddressCodec,
codec.ProvideProtoCodec,
codec.ProvideLegacyAmino,
runtime.ProvideSingletonScopedStoreBuilder,
),
depinject.Invoke(
std.RegisterInterfaces,
Expand All @@ -79,19 +78,20 @@ func AppConfig() depinject.Config {
func NewSimApp[T transaction.Tx](
logger log.Logger,
viper *viper.Viper,
storeBuilder root.Builder,
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Action Required: NewSimApp calls missing storeBuilder parameter.

Several instances of NewSimApp function calls are missing the new storeBuilder parameter, which may lead to inconsistent store configurations across the application. Please update the following files to include the storeBuilder parameter:

  • tests/integration/store/rootmulti/rollback_test.go
  • tests/e2e/genutil/export_test.go
  • simapp/sim_test.go
  • simapp/test_helpers.go
  • simapp/simd/cmd/commands.go
  • simapp/app_di.go
  • simapp/app.go
🔗 Analysis chain

LGTM: Addition of storeBuilder parameter enhances flexibility.

The addition of the storeBuilder parameter to the NewSimApp function signature aligns with the PR objective of removing the singleton storebuilder scope. This change allows for more flexibility in how the store is built and passed to the application.

To ensure this change is consistently applied, please run the following script to check for any occurrences of NewSimApp that might need updating:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all occurrences of NewSimApp function calls in the codebase
# Expect: All occurrences should have the new storeBuilder parameter

rg --type go -A 5 'NewSimApp\s*\('

Length of output: 7126

) *SimApp[T] {
var (
app = &SimApp[T]{}
appBuilder *runtime.AppBuilder[T]
storeBuilder root.Builder
err error
app = &SimApp[T]{}
appBuilder *runtime.AppBuilder[T]
err error

// merge the AppConfig and other configuration in one config
appConfig = depinject.Configs(
AppConfig(),
depinject.Supply(
logger,
viper,
storeBuilder,

// ADVANCED CONFIGURATION

Expand Down
5 changes: 2 additions & 3 deletions simapp/v2/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ import (
"cosmossdk.io/core/transaction"
"cosmossdk.io/log"
sdkmath "cosmossdk.io/math"
"cosmossdk.io/runtime/v2"
serverv2 "cosmossdk.io/server/v2"
serverv2store "cosmossdk.io/server/v2/store"
"cosmossdk.io/store/v2/db"
"cosmossdk.io/store/v2/root"
banktypes "cosmossdk.io/x/bank/types"

"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
Expand All @@ -40,8 +40,7 @@ func NewTestApp(t *testing.T) (*SimApp[transaction.Tx], context.Context) {
vp.Set(serverv2store.FlagAppDBBackend, string(db.DBTypeGoLevelDB))
vp.Set(serverv2.FlagHome, t.TempDir())

runtime.ResetSingletonScopedStoreBuilder()
app := NewSimApp[transaction.Tx](logger, vp)
app := NewSimApp[transaction.Tx](logger, vp, root.NewBuilder())
genesis := app.ModuleManager().DefaultGenesis()

privVal := mock.NewPV()
Expand Down
16 changes: 8 additions & 8 deletions simapp/v2/simdv2/cmd/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,6 @@ import (
v2 "github.com/cosmos/cosmos-sdk/x/genutil/v2/cli"
)

func newApp[T transaction.Tx](logger log.Logger, viper *viper.Viper) serverv2.AppI[T] {
viper.Set(serverv2.FlagHome, simapp.DefaultNodeHome)
return serverv2.AppI[T](simapp.NewSimApp[T](logger, viper))
}

func initRootCmd[T transaction.Tx](
rootCmd *cobra.Command,
txConfig client.TxConfig,
Expand Down Expand Up @@ -71,10 +66,15 @@ func initRootCmd[T transaction.Tx](
offchain.OffChain(),
)

appCreator := func(logger log.Logger, viper *viper.Viper) serverv2.AppI[T] {
viper.Set(serverv2.FlagHome, simapp.DefaultNodeHome)
return serverv2.AppI[T](simapp.NewSimApp[T](logger, viper, storeBuilder))
}

// wire server commands
if err = serverv2.AddCommands(
rootCmd,
newApp,
appCreator,
logger,
initServerConfig(),
cometbft.New(
Expand Down Expand Up @@ -176,13 +176,13 @@ func appExport[T transaction.Tx](

var simApp *simapp.SimApp[T]
if height != -1 {
simApp = simapp.NewSimApp[T](logger, viper)
simApp = simapp.NewSimApp[T](logger, viper, root.NewBuilder())
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved

if err := simApp.LoadHeight(uint64(height)); err != nil {
return genutilv2.ExportedApp{}, err
}
} else {
simApp = simapp.NewSimApp[T](logger, viper)
simApp = simapp.NewSimApp[T](logger, viper, root.NewBuilder())
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
}

return simApp.ExportAppStateAndValidators(jailAllowedAddrs)
Expand Down
4 changes: 2 additions & 2 deletions simapp/v2/simdv2/cmd/root_di.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ func NewRootCmd[T transaction.Tx]() *cobra.Command {
autoCliOpts autocli.AppOptions
moduleManager *runtime.MM[T]
clientCtx client.Context
storeBuilder root.Builder
storeBuilder = root.NewBuilder()
)

if err := depinject.Inject(
depinject.Configs(
simapp.AppConfig(),
depinject.Provide(ProvideClientContext),
depinject.Supply(log.NewNopLogger()),
depinject.Supply(log.NewNopLogger(), storeBuilder),
),
&storeBuilder,
&autoCliOpts,
Expand Down
Loading