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

feat(app): Upgrading to v50 #1736

Open
wants to merge 60 commits into
base: main
Choose a base branch
from
Open

feat(app): Upgrading to v50 #1736

wants to merge 60 commits into from

Conversation

mtsitrin
Copy link
Contributor

@mtsitrin mtsitrin commented Feb 18, 2025

This PR bumps the dymension repo to use cosmos-sdk v50.x

the main changes besides the straightforward syntax changes:

  • deprecated the legacytx Msg interface and the deprecated global amino variables
  • removed deprecated case "/ethermint.types.v1.ExtensionOptionsWeb3Tx" in ante handler

Few technical debts left:

Comment on lines +369 to +376
for _, m := range app.mm.Modules {
if moduleWithName, ok := m.(module.HasName); ok {
moduleName := moduleWithName.Name()
if appModule, ok := moduleWithName.(appmodule.AppModule); ok {
modules[moduleName] = appModule
}
}
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
a.AccountKeeper,
a.BankKeeper,
govModuleAddress,
appCodec, runtime.NewKVStoreService(a.keys[stakingtypes.StoreKey]), a.AccountKeeper, a.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), authcodec.NewBech32Codec(appparams.Bech32PrefixValAddr), authcodec.NewBech32Codec(appparams.Bech32PrefixConsAddr),

Check warning

Code scanning / CodeQL

Directly using the bech32 constants Warning

Directly using the bech32 constants instead of the configuration values
a.AccountKeeper,
a.BankKeeper,
govModuleAddress,
appCodec, runtime.NewKVStoreService(a.keys[stakingtypes.StoreKey]), a.AccountKeeper, a.BankKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), authcodec.NewBech32Codec(appparams.Bech32PrefixValAddr), authcodec.NewBech32Codec(appparams.Bech32PrefixConsAddr),

Check warning

Code scanning / CodeQL

Directly using the bech32 constants Warning

Directly using the bech32 constants instead of the configuration values
@@ -18,26 +21,37 @@
BaseDenomUnit = 18

AccountAddressPrefix = "dym"
Name = "dymension"
)
Bech32MainPrefix = AccountAddressPrefix

Check warning

Code scanning / CodeQL

Directly using the bech32 constants Warning

Directly using the bech32 constants instead of the configuration values
// Bech32PrefixConsAddr defines the Bech32 prefix of a consensus node address
Bech32PrefixConsAddr = Bech32MainPrefix + PrefixValidator + PrefixConsensus
// Bech32PrefixConsPub defines the Bech32 prefix of a consensus node public key
Bech32PrefixConsPub = Bech32MainPrefix + PrefixValidator + PrefixConsensus + PrefixPublic

Check warning

Code scanning / CodeQL

Directly using the bech32 constants Warning

Directly using the bech32 constants instead of the configuration values
// Bech32PrefixConsAddr defines the Bech32 prefix of a consensus node address
Bech32PrefixConsAddr = Bech32MainPrefix + PrefixValidator + PrefixConsensus
// Bech32PrefixConsPub defines the Bech32 prefix of a consensus node public key
Bech32PrefixConsPub = Bech32MainPrefix + PrefixValidator + PrefixConsensus + PrefixPublic

Check warning

Code scanning / CodeQL

Directly using the bech32 constants Warning

Directly using the bech32 constants instead of the configuration values
@mtsitrin mtsitrin marked this pull request as ready for review February 19, 2025 10:10
@mtsitrin mtsitrin requested a review from a team as a code owner February 19, 2025 10:10
Copy link
Contributor

@keruch keruch left a comment

Choose a reason for hiding this comment

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

you can also do make proto-format to reformat all the proto files. and then make proto-gen once again.

* add new update client tx wrapper

* maybe were done, expecting import loops

* rebuild proto

* gonna go back

* Revert "gonna go back"

This reverts commit 8721196.

* move, lets try

* need an actual wrapper message

* need our own message

* regen

* test passes

* fmt

* add a better comment
zale144
zale144 previously approved these changes Feb 27, 2025
Copy link
Contributor

@zale144 zale144 left a comment

Choose a reason for hiding this comment

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

wow I didn't know we had so many files in the repo.
nice work, left a few non-critical comments

panic("failed to load state streaming services: " + err.Error())
// register streaming services
if err := bApp.RegisterStreamingServices(appOpts, app.keys); err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

panic("failed to load register streaming services: " + err.Error())

autocliv1.RegisterQueryServer(app.GRPCQueryRouter(), runtimeservices.NewAutoCLIQueryService(app.mm.Modules))

reflectionSvc, err := runtimeservices.NewReflectionService()
// At startup, after all modules have been registered, check that all prot
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// At startup, after all modules have been registered, check that all prot
// At startup, after all modules have been registered, check that all proto


reflectionSvc, err := runtimeservices.NewReflectionService()
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

add error message

app.mm.RegisterServices(app.configurator)
err := app.mm.RegisterServices(app.configurator)
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

add error message

// The genesis state of the blockchain is represented here as a map of raw json
// messages key'd by an identifier string.
// GenesisState of the blockchain is represented here as a map of raw json
// messages key'd by a identifier string.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// messages key'd by a identifier string.
// messages key'd by an identifier string.

MintCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) error
SendCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error
SpendableCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins
// MintCoins(ctx context.Context, moduleName string, amt sdk.Coins) error
Copy link
Contributor

Choose a reason for hiding this comment

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

if not used, better remove

Comment on lines +20 to +26
// func init() {
// v1beta1.RegisterProposalType(ProposalTypeMigrateChainIdsProposal)
// govcdc.ModuleCdc.Amino.RegisterConcrete(&MigrateChainIdsProposal{}, "dymns/"+ProposalTypeMigrateChainIdsProposal, nil)

v1beta1.RegisterProposalType(ProposalTypeUpdateAliasesProposal)
govcdc.ModuleCdc.Amino.RegisterConcrete(&UpdateAliasesProposal{}, "dymns/"+ProposalTypeUpdateAliasesProposal, nil)
}
// v1beta1.RegisterProposalType(ProposalTypeUpdateAliasesProposal)
// govcdc.ModuleCdc.Amino.RegisterConcrete(&UpdateAliasesProposal{}, "dymns/"+ProposalTypeUpdateAliasesProposal, nil)
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

if not used, better remove

_ module.HasGenesis = AppModule{}
_ module.HasInvariants = AppModule{}
)
// _ module.HasGenesis = AppModule{}
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed?

Comment on lines +31 to +32
// _ module.AppModuleSimulation = AppModule{}
// _ module.HasGenesis = AppModule{}
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants