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

merged 5 commits into from
Feb 21, 2023

Conversation

gitferry
Copy link
Contributor

This PR addresses #174 to keep a single encoding config in the application other than creating a new one in InitPrivSigner.

vitsalis
vitsalis reviewed Feb 17, 2023
View reviewed changes
app/test_helpers.go Outdated
@@ -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.

@gitferry
Copy link
Contributor Author

gitferry commented Feb 20, 2023 •
edited
Loading

@vitsalis @SebastianElvis @KonradStaniec Thanks you guys for your input. I renamed MakeTestEncodingConfig into makeEncodingConfig and added a getter to globally get the singleton of the encodingConfig. Making makeEncodingConfig private because it ensures that the global variable encodingConfig is initiated once during the lifecycle of the application. In that sense, I don't think Osmosis is doing it right because MakeEncodingConfig is public and called everywhere...

@gitferry gitferry force-pushed the checkpointing-keep-single-encoding-config branch from c4fab7d to 9b2ef01 Compare February 20, 2023 14:27
@gitferry gitferry force-pushed the checkpointing-keep-single-encoding-config branch from 9b2ef01 to 9aadd53 Compare February 20, 2023 14:29
SebastianElvis
SebastianElvis reviewed Feb 20, 2023
View reviewed changes
Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Looks like many MakeEncodingConfig invocations can be replaced by GetEncodingConfig. Maybe I miss something. Otherwise lgtm!

app/app_test.go Outdated
@@ -11,7 +11,7 @@ import (
)

func TestBabylonBlockedAddrs(t *testing.T) {
encCfg := MakeTestEncodingConfig()
encCfg := MakeEncodingConfig()
Copy link
Member

Choose a reason for hiding this comment

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

Here should we use GetEncodingConfig or MakeEncodingConfig? Looks like the former one is usable since the package has already finished initialising encodingConfig when reaching this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment. I just realized that we should only use GetEncodingConfig because the global variable encodingConfig is initiated when the app package is loaded. So I made madeEncodingConfig private and only use GetEncodingConfig externally.

app/app_test.go Outdated
@@ -48,7 +48,7 @@ func TestGetMaccPerms(t *testing.T) {
}

func TestUpgradeStateOnGenesis(t *testing.T) {
encCfg := MakeTestEncodingConfig()
encCfg := MakeEncodingConfig()
Copy link
Member

Choose a reason for hiding this comment

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

ditto

app/test_helpers.go Outdated
@@ -77,7 +80,7 @@ type SetupOptions struct {

func setup(withGenesis bool, invCheckPeriod uint) (*BabylonApp, GenesisState) {
db := dbm.NewMemDB()
encCdc := MakeTestEncodingConfig()
encCdc := MakeEncodingConfig()
Copy link
Member

Choose a reason for hiding this comment

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

same here, looks like GetEncodingConfig is usable?

@gitferry gitferry force-pushed the checkpointing-keep-single-encoding-config branch from 4f3a1e8 to 2b3d37e Compare February 21, 2023 04:07
@gitferry gitferry force-pushed the checkpointing-keep-single-encoding-config branch from 2b3d37e to 733f113 Compare February 21, 2023 04:12
SebastianElvis
SebastianElvis approved these changes Feb 21, 2023
View reviewed changes
Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

Thanks, now we have a single source of encoding config 💯

@gitferry gitferry merged commit 570860e into dev Feb 21, 2023
@gitferry gitferry deleted the checkpointing-keep-single-encoding-config branch February 21, 2023 07:21
@gitferry gitferry mentioned this pull request Feb 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitsalis vitsalis vitsalis left review comments

@SebastianElvis SebastianElvis SebastianElvis approved these changes

@KonradStaniec KonradStaniec KonradStaniec approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gitferry @vitsalis @SebastianElvis @KonradStaniec