-
Notifications
You must be signed in to change notification settings - Fork 625
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
Draft: support middleware in ICS20 #351
Conversation
7fcd78f
to
5dfaf88
Compare
This pull request introduces 3 alerts when merging 29caccd2048b2d29456afcb8c26056b5be1e0b6b into 358bfa5 - view on LGTM.com new alerts:
|
29caccd
to
5dabfe5
Compare
This pull request introduces 2 alerts when merging 5dabfe5 into 358bfa5 - view on LGTM.com new alerts:
|
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.
Thanks for opening this PR!
Is it necessary to add/modify tests together with these changes?
channelCap *capabilitytypes.Capability, | ||
packet exported.PacketI, | ||
) error { | ||
//todo |
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.
What's this todo
?
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 it is probably slightly unclear of how to implement this function. But the general idea is we need to Authenticate the capability passed in as being the app capability, then we need to get our own capability and pass it into the SendPacket function of the middleware above us
I can draft up code later
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 have some ideas here.
modules/apps/transfer/module.go
Outdated
@@ -37,7 +38,8 @@ var ( | |||
) | |||
|
|||
// AppModuleBasic is the IBC Transfer AppModuleBasic | |||
type AppModuleBasic struct{} | |||
type AppModuleBasic struct { | |||
} |
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.
This change is not needed, right?
modules/apps/transfer/module.go
Outdated
@@ -102,6 +106,11 @@ func NewAppModule(k keeper.Keeper) AppModule { | |||
} | |||
} | |||
|
|||
// SetMiddleware set ICS30 middleware |
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.
Typo: it should be sets
.
@@ -29,8 +29,14 @@ const ( | |||
|
|||
// DenomPrefix is the prefix used for internal SDK coin representation. | |||
DenomPrefix = "ibc" | |||
|
|||
KeyAppCapability = "app_capabilities" |
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 in order to be consistent with other modules this should be KeyAppCapabilityPrefix = "appCapabilities"
. Maybe @colin-axner can confirm whether what I say is correct or not.
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 agree, but it looks like these changes come from @AdityaSripal pr
) | ||
|
||
func AppCapabilityName(channelID, portID string) string { |
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.
And for consistency I think this function should be called AppCapabilityPath
.
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.
We can leave it as AppCapabilityName
(it isn't being used as a storage path, but as a capability name). This code can be added into 05-port though via the main branch (I can do this)
|
||
import "strings" | ||
|
||
// SplitChannelVersion middleware version will split the channel version string |
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 believe it should read as SplitChannelVersion splits ...
.
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, but it was taken from @AdityaSripal's pr
This change can go directly into the main branch, before any of these pr's. I can open a pr
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.
Excellent! Thanks for taking up this initiative! I'll be adding the middleware to interchain accounts as well soon. That should help fill in any remaining gaps and I'll also figure out how to test for these changes
@@ -102,6 +106,11 @@ func NewAppModule(k keeper.Keeper) AppModule { | |||
} | |||
} | |||
|
|||
// SetMiddleware set ICS30 middleware | |||
func (am AppModule) SetMiddleware(app porttypes.IBCModule) { |
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.
The intent here is to avoid breaking API of NewAppModule
? Nice call 👍
modules/apps/transfer/module.go
Outdated
return am.app.OnChanCloseInit(ctx, portID, channelID) | ||
} | ||
|
||
return nil |
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.
this function shouldn't change. ICS 20 should never allow OnChanCloseInit
to succeed
|
||
import "strings" | ||
|
||
// SplitChannelVersion middleware version will split the channel version string |
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, but it was taken from @AdityaSripal's pr
This change can go directly into the main branch, before any of these pr's. I can open a pr
modules/apps/transfer/module.go
Outdated
@@ -4,6 +4,7 @@ import ( | |||
"context" | |||
"encoding/json" | |||
"fmt" | |||
capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/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.
nit: this should go in the middle section of imports
modules/apps/transfer/module.go
Outdated
transferVersion, appVersion := channeltypes.SplitChannelVersion(version) | ||
|
||
if transferVersion == "" { | ||
// middleware not supported |
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.
// middleware not supported | |
// This may occur if: | |
// - transfer is acting as the base application in the middleware stack | |
// - the connected base application has chosen not to append its own version |
modules/apps/transfer/module.go
Outdated
appCap, ok = am.scopedKeeper.GetCapability(ctx, types.AppCapabilityName(channelID, portID)) | ||
if !ok { | ||
return sdkerrors.Wrap(capabilitytypes.ErrCapabilityNotFound, | ||
"could not find app capability on OnChanOpenTry even after OnChanOpenInit called on this chain first (crossing hellos)") |
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 would have an error message like "could not find app capability for channel %s and port %s", channelID, portID"
or something like that. @crodriguezvega any suggestion on format we should follow?
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.
@colin-axner what you propose looks good!
@@ -29,8 +29,14 @@ const ( | |||
|
|||
// DenomPrefix is the prefix used for internal SDK coin representation. | |||
DenomPrefix = "ibc" | |||
|
|||
KeyAppCapability = "app_capabilities" |
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 agree, but it looks like these changes come from @AdityaSripal pr
) | ||
|
||
func AppCapabilityName(channelID, portID string) string { |
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.
We can leave it as AppCapabilityName
(it isn't being used as a storage path, but as a capability name). This code can be added into 05-port though via the main branch (I can do this)
channelCap *capabilitytypes.Capability, | ||
packet exported.PacketI, | ||
) error { | ||
//todo |
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 it is probably slightly unclear of how to implement this function. But the general idea is we need to Authenticate the capability passed in as being the app capability, then we need to get our own capability and pass it into the SendPacket function of the middleware above us
I can draft up code later
This pull request introduces 2 alerts when merging 1df0d88 into 9d5da17 - view on LGTM.com new alerts:
|
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.
Looks good, although I'd like to see additional documentation on how to incorporate the middleware into the module
if cpTransferVersion != transferVersion { | ||
return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected counterparty version: %s, got: %s", types.Version, cpTransferVersion) | ||
} |
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'd move this above the ValidateTransferChannelParams
since it doesn't require stateful validation
scopedKeeper capabilitykeeper.ScopedKeeper | ||
app porttypes.IBCModule |
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.
can you add comments for each of these fields and explain what they do in the context of the AppModule
?
Unclear how I could implement my idea for hub packet forwarding with this proposed middleware setup. Also would love feedback on the following PR: |
Outside the scope of this pr as it isn't the responsibility of an external contributor. I've opened #359 (core maintainers will address this issue) |
@thomas-nguy is there anything we can do to help you move forward with this PR? @colin-axner has finished a PR where he implemented middleware for another feature and we have now middleware documentation available as well. I hope these two sources can offer some guidance about how to develop middleware. If you need any assistance, please let us know! |
@crodriguezvega thanks for the reminder, I will have a look this week and try to complete a PR |
i'm restarting the implementation from a clean main branch (#533) |
* add markdown linter * fix existing markdowns
Description
closes: #347
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.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes