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: upgrade to cosmos v50 #1833

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

feat: upgrade to cosmos v50 #1833

wants to merge 61 commits into from

Conversation

Yaroms
Copy link
Collaborator

@Yaroms Yaroms commented Dec 12, 2024

Description

Closes: #XXXX


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

  • read the contribution guide
  • 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 main branch
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • 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.

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

Copy link

github-actions bot commented Dec 15, 2024

Test Results

2 391 tests  +5   2 390 ✅ +4   14m 43s ⏱️ - 10m 25s
  121 suites  - 1       0 💤 ±0 
    7 files   ±0       0 ❌ ±0   1 🔥 +1 

For more details on these errors, see this check.

Results for commit 18a9c09. ± Comparison against base commit 0284c25.

This pull request removes 1 and adds 6 tests. Note that renamed tests count towards both.
github.com/lavanet/lava/v4/x/pairing/keeper ‑ TestGetPairing/epochTimesChanged
github.com/lavanet/lava/v4/utils/lavaslices ‑ TestSortStable
github.com/lavanet/lava/v4/utils/lavaslices ‑ TestSortStable/already_sorted
github.com/lavanet/lava/v4/utils/lavaslices ‑ TestSortStable/duplicates
github.com/lavanet/lava/v4/utils/lavaslices ‑ TestSortStable/empty_slice
github.com/lavanet/lava/v4/utils/lavaslices ‑ TestSortStable/negative_numbers
github.com/lavanet/lava/v4/utils/lavaslices ‑ TestSortStable/reverse_order

♻️ This comment has been updated with latest results.

@@ -29,7 +28,7 @@ func MakeEncodingConfig() params.EncodingConfig {
encodingConfig := makeEncodingConfig()
std.RegisterLegacyAminoCodec(encodingConfig.Amino)
std.RegisterInterfaces(encodingConfig.InterfaceRegistry)
ModuleBasics.RegisterLegacyAminoCodec(encodingConfig.Amino)
// ModuleBasics.RegisterLegacyAminoCodec(encodingConfig.Amino)
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove comment

if err != nil {
return servertypes.ExportedApp{}, err
}

validators, err := staking.WriteValidators(ctx, app.StakingKeeper)
Copy link
Collaborator

Choose a reason for hiding this comment

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

check error and return in err != nil instead of returning the error

if len(splittedAttrs) > 1 {
var err error
index, err = strconv.Atoi(splittedAttrs[1])
if len(splittedAttrs) == 2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

put a comment specifying that you check for 2 because in cosmos 50 you have msg_index

@@ -71,7 +71,7 @@ func eventsLookup(ctx context.Context, clientCtx client.Context, blocks, fromBlo
utils.LavaFormatError("invalid blockResults status", err)
return
}
for _, event := range blockResults.BeginBlockEvents {
for _, event := range blockResults.FinalizeBlockEvents {
Copy link
Collaborator

Choose a reason for hiding this comment

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

check mode=BeginBlock (see EndBlockEvents)

@@ -101,7 +101,7 @@ func (et *EventTracker) getLatestVersionEvents(latestBlock int64) (updated bool,
if et.latestUpdatedBlock != latestBlock {
return false, utils.LavaFormatWarning("event results are different than expected", nil, utils.Attribute{Key: "requested latestBlock", Value: latestBlock}, utils.Attribute{Key: "current latestBlock", Value: et.latestUpdatedBlock})
}
for _, event := range et.blockResults.EndBlockEvents {
for _, event := range et.blockResults.FinalizeBlockEvents {
Copy link
Collaborator

Choose a reason for hiding this comment

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

check mode=EndBlock (see EndBlockEvents)


return nil
return k.distributionKeeper.SetFeePool(ctx, feePool)
Copy link
Collaborator

Choose a reason for hiding this comment

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

handle the error before returning and print a informative message (like "failed funding community pool")

@@ -131,7 +134,13 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {
migrator := keeper.NewMigrator(am.keeper)

// register v3 -> v4 migration
if err := cfg.RegisterMigration(types.ModuleName, 3, migrator.Migrate3to4); err != nil {
// if err := cfg.RegisterMigration(types.ModuleName, 3, migrator.Migrate3to4); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove comment

upgradeclient.LegacyCancelProposalHandler,
ibcclientclient.UpdateClientProposalHandler,
ibcclientclient.UpgradeProposalHandler,
// UNFORKING v2 TODO: Verify it is okay to remove these
Copy link
Collaborator

Choose a reason for hiding this comment

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

do the todo

AddRoute(upgradetypes.RouterKey, upgrade.NewSoftwareUpgradeProposalHandler(&app.UpgradeKeeper)).
// AddRoute(ibcexported.RouterKey, ibcclient.NewClientProposalHandler(app.IBCKeeper.ClientKeeper))
AddRoute(ibcclienttypes.RouterKey, ibcclient.NewClientProposalHandler(app.IBCKeeper.ClientKeeper))
// TODO check all these proposals still work YAROM
Copy link
Collaborator

Choose a reason for hiding this comment

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

do the todo

@@ -1079,17 +1132,21 @@ func GetMaccPerms() map[string][]string {
func initParamsKeeper(appCodec codec.BinaryCodec, legacyAmino *codec.LegacyAmino, key, tkey storetypes.StoreKey) paramskeeper.Keeper {
paramsKeeper := paramskeeper.NewKeeper(appCodec, legacyAmino, key, tkey)

// legacy param set to for ibc migrator
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you explain?

authcmd.QueryTxsByEventsCmd(),
authcmd.QueryTxCmd(),
)

app.ModuleBasics.AddQueryCommands(cmd)
moduleBasics.AddQueryCommands(cmd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -306,7 +346,7 @@ func txCommand() *cobra.Command {
authcmd.GetDecodeCommand(),
)

app.ModuleBasics.AddTxCommands(cmd)
moduleBasics.AddTxCommands(cmd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@oren-lava
Copy link
Collaborator

Should we consider these?

  1. grpc-web port: https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#grpc-web-1
  2. all GetTxCmd() and GetQueryCmd() can be removed (https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#all-1)
  3. all AddTxCommands() and AddQueryCommands() can be removed (https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#autocli)

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