Use connection id instead of channel id in IsMiddlewareEnabled #2260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…se-the-connection-id-instead-of-the-channel-id
chatton
commented
Sep 9, 2022
| @@ -23,7 +23,7 @@ type IBCMiddleware struct { | |||
| keeper keeper.Keeper | |||
| } | |||
|
|
|||
| // IBCMiddleware creates a new IBCMiddleware given the associated keeper and underlying application | |||
| // NewIBCMiddleware creates a new IBCMiddleware given the associated keeper and underlying application | |||
There was a problem hiding this comment.
unrelated docstring fix
| @@ -61,6 +62,15 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger { | |||
| return ctx.Logger().With("module", fmt.Sprintf("x/%s-%s", host.ModuleName, icatypes.ModuleName)) | |||
| } | |||
|
|
|||
| // GetConnectionID returns the connection id for the given port and channelIDs. | |||
| func (k Keeper) GetConnectionID(ctx sdk.Context, portID, channelID string) (string, error) { | |||
There was a problem hiding this comment.
I chose to add this function rather than export the channelKeeper, as so far it's the only time we've needed.
…lso use connection ID
…ection-id-instead-of-the-channel-id' of https://github.com/cosmos/ibc-go into cian/issue#2235-ismiddlewareenabled-should-use-the-connection-id-instead-of-the-channel-id
chatton
commented
Sep 9, 2022
Comment on lines
+157
to
+159
| portID := keySplit[1] | ||
| connectionID := keySplit[2] | ||
| channelID := string(iterator.Value()) |
There was a problem hiding this comment.
extracted these out to make it more clear which values are being passed into IsMiddlewareEnabled
chatton
requested review from
seantking,
colin-axner,
AdityaSripal and
damiannolan
as code owners
…se-the-connection-id-instead-of-the-channel-id
…se-the-connection-id-instead-of-the-channel-id
Codecov Report
@@ Coverage Diff @@
## main #2260 +/- ##
==========================================
- Coverage 79.51% 79.43% -0.08%
==========================================
Files 175 175
Lines 12077 12103 +26
==========================================
+ Hits 9603 9614 +11
- Misses 2049 2059 +10
- Partials 425 430 +5
|
…se-the-connection-id-instead-of-the-channel-id
…se-the-connection-id-instead-of-the-channel-id
9 tasks
…se-the-connection-id-instead-of-the-channel-id
…se-the-connection-id-instead-of-the-channel-id
…se-the-connection-id-instead-of-the-channel-id
…se-the-connection-id-instead-of-the-channel-id
colin-axner
approved these changes
Sep 20, 2022
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| k.SetMiddlewareEnabled(ctx, portID, channelID) | ||
| k.SetMiddlewareEnabled(ctx, portID, connectionID) |
There was a problem hiding this comment.
Is there an issue that moves this setting (it needs to be moved to be called before registerInterchainAccount, since it is referenced in OnChanOpenInit
There was a problem hiding this comment.
Looks like its covered in #2283
…ection-id-instead-of-the-channel-id' of https://github.com/cosmos/ibc-go into cian/issue#2235-ismiddlewareenabled-should-use-the-connection-id-instead-of-the-channel-id
chatton
deleted the
cian/issue#2235-ismiddlewareenabled-should-use-the-connection-id-instead-of-the-channel-id
branch
mergify bot
pushed a commit
that referenced
this pull request
Sep 20, 2022
(cherry picked from commit 6b7d67f) # Conflicts: # modules/apps/27-interchain-accounts/controller/keeper/keeper.go
damiannolan
pushed a commit
that referenced
this pull request
Sep 26, 2022
…ort #2260) (#2319) * Use connection id instead of channel id in IsMiddlewareEnabled (#2260) (cherry picked from commit 6b7d67f) # Conflicts: # modules/apps/27-interchain-accounts/controller/keeper/keeper.go * chore: handle merge conflicts Co-authored-by: Cian Hatton <[email protected]> Co-authored-by: Cian Hatton <[email protected]> Co-authored-by: colin axnér <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR updates
IsMiddlewareEnabled,SetMiddlewareEnabledandDeleteMiddlewareEnabledto use portID and connectionID as opposed to portID and channelID.closes: #2235
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.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes