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

app.go and module manager clean up #1

Merged
merged 3 commits into from
Jan 13, 2025
Merged

Conversation

kocubinski
Copy link
Collaborator

The module manager work with configurator and upgrades was a little tricky.

  • Upgrade gov keeper
  • ABCI methods in app.go
  • Add RegisterServices to Configurator, satisfying 0.52 interface. Include acceptedMessages logic.
  • Update module manager to sync with 0.52 implementation, copy over code where appropriate.

app/module/manager.go Outdated Show resolved Hide resolved
- leave some TODOs
@@ -897,57 +921,6 @@ func initParamsKeeper(appCodec codec.BinaryCodec, legacyAmino *codec.LegacyAmino
return paramsKeeper
}

func (app *App) InitializeAppVersion(ctx sdk.Context) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems redundant in 0.52 since AppVersion in BaseApp passes directly through to the consensus keeper.

}
}

// OfferSnapshot is a wrapper around the baseapp's OfferSnapshot method. It is
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems redundant in 0.52 since there's no way to obtain the AppVersion from an abci.RequestOfferSnapshot. Not totally clear why it is needed to lazy mount stores, or offer snapshots for an arbitrary consensus version (maybe for syncing?) but if it is we'll need a workaround. Maybe a custom request type is needed?

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

🚀

app/check_tx.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Maybe in a follow-up this should be a sdk.CheckTxHandler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense to me

Copy link
Member

Choose a reason for hiding this comment

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

any reason we cannot use the sdk manager?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the same thought, but this manager is doing something fundamentally different with keeping around sets of versioned modules. I haven't audited the code paths to enough to determine if there could be a map[uint64]sdk.ModuleManger, but maybe worth investigation into a refactoring clean up

@kocubinski kocubinski merged commit 0c1bb0f into sdk-v0.52.x Jan 13, 2025
12 of 26 checks passed
@kocubinski kocubinski deleted the kocu/module-manager branch January 13, 2025 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants