Add sentinel localhost connection end to genesis state #3040
Conversation
|
We discussed making this migration happen automatically, and also removing the boolean field and always creating this connection end in state in |
There was a problem hiding this comment.
Thank you, @chatton. I have left some ideas; curious to read your thoughts about them.
| @@ -19,6 +19,7 @@ func InitGenesis(ctx sdk.Context, k keeper.Keeper, gs types.GenesisState) { | |||
| } | |||
| k.SetNextConnectionSequence(ctx, gs.NextConnectionSequence) | |||
| k.SetParams(ctx, gs.Params) | |||
| k.CreateLocalhostConnectionEnd(ctx) | |||
There was a problem hiding this comment.
Maybe this could be done in two steps if we reuse the existing SetConnection function. Something like this:
localhostConnection = k.GenerateSentinelLocalhostConnection(...)
k.SetConnection(ctx, localhostConnection)There was a problem hiding this comment.
I think this is a good idea! A dedicated function is maybe not required.
| } | ||
|
|
||
| // CreateLocalhostConnectionEnd writes a sentinel localhost ConnectionEnd to state. | ||
| func (k Keeper) CreateLocalhostConnectionEnd(ctx sdk.Context) { |
There was a problem hiding this comment.
Do we really need this function? Cannot we use the existing SetConnection and pass as argument the sentinel localhost connection end created by GetSentinelLocalhostConnectionEnd?
| @@ -208,6 +208,25 @@ func (k Keeper) GetAllConnections(ctx sdk.Context) (connections []types.Identifi | |||
| return connections | |||
| } | |||
|
|
|||
| // GetSentinelLocalhostConnectionEnd returns the sentinel localhost connection end. | |||
| func (k Keeper) GetSentinelLocalhostConnectionEnd() types.ConnectionEnd { | |||
There was a problem hiding this comment.
Maybe also a bit of a nit, but should we rename this to GenerateSentinelLocalhostConnection or CreateSentinelLocalhostConnection because we usually use the verb Get for methods that retrieve data from the store. I would also remove the suffix End to keep it consistent with the naming in the rest of the keeper.
There was a problem hiding this comment.
Yes this sounds like a good change!
| // GetSentinelLocalhostConnectionEnd returns the sentinel localhost connection end. | ||
| func (k Keeper) GetSentinelLocalhostConnectionEnd() types.ConnectionEnd { | ||
| counterparty := types.NewCounterparty(exported.Localhost, types.LocalhostID, commitmenttypes.NewMerklePrefix(k.GetCommitmentPrefix().Bytes())) | ||
| return types.ConnectionEnd{ |
There was a problem hiding this comment.
Should we use the constructor function NewConnectionEnd?
|
|
||
| // MigrateLocalhostConnectionEnd creates the sentinel localhost connection end to enable | ||
| // localhost ibc functionality. | ||
| func MigrateLocalhostConnectionEnd(ctx sdk.Context, connectionKeeper ConnectionKeeper) { |
There was a problem hiding this comment.
Sorry, super nit:
| func MigrateLocalhostConnectionEnd(ctx sdk.Context, connectionKeeper ConnectionKeeper) { | |
| func MigrateLocalhostConnection(ctx sdk.Context, connectionKeeper ConnectionKeeper) { |
| @@ -124,8 +124,8 @@ func (am AppModule) RegisterServices(cfg module.Configurator) { | |||
| channeltypes.RegisterMsgServer(cfg.MsgServer(), am.keeper) | |||
| types.RegisterQueryService(cfg.QueryServer(), am.keeper) | |||
|
|
|||
| m := clientkeeper.NewMigrator(am.keeper.ClientKeeper) | |||
There was a problem hiding this comment.
Maybe this is due to my lack of knowledge of how migrations work, but if we replace Migrate2to3 with Migration3to4 and a chain upgrades from v6.0.0 to v7.1.0, wouldn't it miss the Migrate2to3 migration?
There was a problem hiding this comment.
right, my understanding was that chains needed to update to each version one by one in order to run all the migrations. I feel like it should be possible, to just register all migrations at once (as they should be idempotent) however I'm not sure if this is the case. Maybe @AdityaSripal has an answer to this?
There was a problem hiding this comment.
Yeah, I am also not sure if the chains really need to upgrade to each version of ibc-go or if they can jump versions as long as they run the migrations of the versions in between.
There was a problem hiding this comment.
if this is the case then we would preferably just add it as a migration instead of replace
There was a problem hiding this comment.
Yeah, it should be possible to register multiple migrations.
chatton
deleted the
cian/issue#3031-add-sentinel-localhost-connectionend-to-genesis-state
branch
Description
closes: #3031
closes: #3032
Commit Message / Changelog Entry
N/A
entry will be added as part of feature branch
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.