-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add sentinel localhost connection end to genesis state #3040
Add sentinel localhost connection end to genesis state #3040
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and clean!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it should be possible to register multiple migrations.
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/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.