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

Jump to bottom

chore: Keep a single encoding config in the application #308

Merged
merged 5 commits into from
Feb 21, 2023
Merged

chore: Keep a single encoding config in the application #308

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9140ef2
keep single encoding config in the application
gitferry Feb 17, 2023
9aadd53
remove test encoding config
gitferry Feb 20, 2023
9fe1651
Merge branch 'dev' into checkpointing-keep-single-encoding-config
gitferry Feb 20, 2023
733f113
address comment
gitferry Feb 21, 2023
3827a96
remove config files
gitferry Feb 21, 2023
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
11 changes: 7 additions & 4 deletions app/test_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,12 @@ import (
"cosmossdk.io/math"
tmconfig "github.com/tendermint/tendermint/config"

tmjson "github.com/tendermint/tendermint/libs/json"

"github.com/babylonchain/babylon/app/params"
appparams "github.com/babylonchain/babylon/app/params"
"github.com/babylonchain/babylon/testutil/datagen"
tmjson "github.com/tendermint/tendermint/libs/json"

txformat "github.com/babylonchain/babylon/btctxformatter"
bbn "github.com/babylonchain/babylon/types"
bam "github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/client"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
Expand All @@ -43,6 +42,9 @@ import (
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
tmtypes "github.com/tendermint/tendermint/types"
dbm "github.com/tendermint/tm-db"

txformat "github.com/babylonchain/babylon/btctxformatter"
bbn "github.com/babylonchain/babylon/types"
)

// DefaultConsensusParams defines the default Tendermint consensus params used in
Expand Down Expand Up @@ -242,7 +244,8 @@ func SetupPrivSigner() (*PrivSigner, error) {
if err != nil {
return nil, err
}
privSigner, _ := InitPrivSigner(client.Context{}, ".", kr)
encodingCfg := appparams.MakeTestEncodingConfig()
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this function deprecated? See here

Copy link
Member

Choose a reason for hiding this comment

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

Since the encoding config never changes except when adding a new module, perhaps we need to create a unified function that returns the encoding config, or even make it a singleton. I wrote such a function in rpc-client. Perhaps this should instead be done in Babylon repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vitsalis It is indeed deprecated but it seems that it is the only way to get the global encoding config. It should be only called once during the lifecycle of the validator and this PR is to make sure of that.

@SebastianElvis I think appparams.MakeTestEncodingConfig() sets global ModuleBasics and returns the encoding config. So, maybe we can just call appparams.MakeTestEncodingConfig() in rpc-client?

Copy link
Member

Choose a reason for hiding this comment

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

So, maybe we can just call appparams.MakeTestEncodingConfig() in rpc-client?

The rpc-client introduces this new function exactly because MakeTestEncodingConfig is deprecated 🤣 so how about we formalise appparams.MakeTestEncodingConfig() a bit to make something like appparams.MakeEncodingConfig()?

Copy link
Contributor

Choose a reason for hiding this comment

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

from what I see in Juno and Osmosis, each have similar setup to ours. They just do do not call their function MakeTestEncodingConifg but MakeEncodingConfig and those functions are not deprecated. In general, I wonder why this function (MakeTestEncodingConfig in proto.go) is deprecated ? from file history it seems it was marked as such from the start.

My general sentiment here would be to check what others are doing (osmosis, juno, gaia?) and do the same, maybe we could change the naming also i.e drop the Test from name and remove deprecation mark if we do not know why it is there.

From what i understand the only real requirement here is that MakeEncodingConfig should be called only once per application initialisation.

privSigner, _ := InitPrivSigner(client.Context{}, ".", kr, encodingCfg)
privSigner.WrappedPV.Clean(nodeCfg.PrivValidatorKeyFile(), nodeCfg.PrivValidatorStateFile())
return privSigner, nil
}
Expand Down
4 changes: 2 additions & 2 deletions app/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
tmconfig "github.com/tendermint/tendermint/config"
tmos "github.com/tendermint/tendermint/libs/os"

appparams "github.com/babylonchain/babylon/app/params"
"github.com/babylonchain/babylon/privval"
)

Expand Down Expand Up @@ -43,7 +44,7 @@ type PrivSigner struct {
ClientCtx client.Context
}

func InitPrivSigner(clientCtx client.Context, nodeDir string, kr keyring.Keyring) (*PrivSigner, error) {
func InitPrivSigner(clientCtx client.Context, nodeDir string, kr keyring.Keyring, encodingCfg appparams.EncodingConfig) (*PrivSigner, error) {
// setup private validator
nodeCfg := tmconfig.DefaultConfig()
pvKeyFile := filepath.Join(nodeDir, nodeCfg.PrivValidatorKeyFile())
Expand All @@ -60,7 +61,6 @@ func InitPrivSigner(clientCtx client.Context, nodeDir string, kr keyring.Keyring

// TODO this should probably not create separate config, but rahter accept it
// as argument
encodingCfg := MakeTestEncodingConfig()
clientCtx = clientCtx.
WithInterfaceRegistry(encodingCfg.InterfaceRegistry).
WithCodec(encodingCfg.Marshaler).
Expand Down
4 changes: 2 additions & 2 deletions cmd/babylond/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ func (a appCreator) newApp(logger log.Logger, db dbm.DB, traceStore io.Writer, a
if err != nil {
panic(err)
}
privSigner, err := app.InitPrivSigner(clientCtx, homeDir, clientCtx.Keyring)
privSigner, err := app.InitPrivSigner(clientCtx, homeDir, clientCtx.Keyring, a.encCfg)
if err != nil {
panic(err)
}
Expand Down Expand Up @@ -337,7 +337,7 @@ func (a appCreator) appExport(
panic(err)
}

privSigner, err := app.InitPrivSigner(clientCtx, homePath, kr)
privSigner, err := app.InitPrivSigner(clientCtx, homePath, kr, a.encCfg)
if err != nil {
panic(err)
}
Expand Down