-
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
feat: middleware support for ICS20 #533
feat: middleware support for ICS20 #533
Conversation
b6873a1
to
f060f30
Compare
f060f30
to
ae0629b
Compare
This pull request introduces 1 alert when merging ae0629b into 823ef67 - view on LGTM.com new alerts:
|
@@ -19,8 +22,14 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types. | |||
if err != nil { | |||
return nil, err | |||
} | |||
|
|||
channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(msg.SourcePort, msg.SourceChannel)) |
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 I am not totally sure how I should handle the channel cap here. I moved it to the msg_server level. Is there anything else that needs to be done?
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 for the delayed reply! This looks good to me, although I'm not sure it matters if ics20 continues to claim the channel capability in the channel handshake callbacks.
@AdityaSripal Do we want to allow applications using ics20 as middleware to claim the capability?
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.
@thomas-nguy Is it desirable for your use case for the connected application to be capable of sending packets? Otherwise I'm thinking maybe we should leave all the capability management as it was. Essentially only allowing the connected application to get information about the ICS20 transfer, but not act upon it
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.
Not necessary, but I did claim here to preserve the original behavior.
Before it was claimed at "SendTransfer" level, but this line needs to be removed since the capability is now passed by parameter
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.
So in this case, we should have some sort of conditional logic that only claims capability if im.app is nil. This applies to both port and channnel capabilities.
Do you refer to this section @AdityaSripal ?
The capability here is claim by the msg_server which is outside the middleware layer. Not sure to understand which check you want to do
0aaede8
to
1edd2fa
Compare
1edd2fa
to
9b4ac36
Compare
This pull request introduces 1 alert when merging 9b4ac36 into 823ef67 - view on LGTM.com new alerts:
|
what's the status of this PR? @thomas-nguy |
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.
LGTM, @AdityaSripal can you reply to the comment above?
@thomas-nguy can you add a changelog entry?
This pull request introduces 1 alert when merging a019faf into 4523ef5 - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## main #533 +/- ##
==========================================
- Coverage 79.56% 79.40% -0.16%
==========================================
Files 145 145
Lines 10625 10640 +15
==========================================
- Hits 8454 8449 -5
- Misses 1755 1773 +18
- Partials 416 418 +2
|
To help push this along, I'm going to pull out some of the changes into a separate pr, primarily allowing transfer to hook up to a middleware like ics29. This change is very small and doesn't change any ics20 functionality (simply adding the ics4Wrapper interface) I like the direction of this pr (excluding OnRecv from supported callbacks). I'd like to resolve the channel capability discussion with @AdityaSripal once he returns to work. I'm leaning towards only allowing applications connecting to ics20 from below to act as logging for now. I think logic that requires returning of acknowledgements (such as triggers ie sending an IBC packet) require more thoughts as we may need to change the ics20 acknowledgement format to wrap the returned acknowledgements |
return nil | ||
} | ||
// call underlying app's OnChanOpenAck callback with the counterparty app version. | ||
return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion) |
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 the reasoning for now adding callbacks for OnChanOpenTry and OnChanOpenConfirm?
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.
not much reason beside adding more flexibility, might be useful for the wired app? if not it should return nil anyways?
@@ -19,8 +22,14 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types. | |||
if err != nil { | |||
return nil, err | |||
} | |||
|
|||
channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(msg.SourcePort, msg.SourceChannel)) |
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.
@thomas-nguy Is it desirable for your use case for the connected application to be capable of sending packets? Otherwise I'm thinking maybe we should leave all the capability management as it was. Essentially only allowing the connected application to get information about the ICS20 transfer, but not act upon it
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.
For middleware architecture, the underlying base app must be the one to claim capability.
So in this case, we should have some sort of conditional logic that only claims capability if im.app
is nil. This applies to both port and channnel capabilities.
Can we have some usecases presented to see how this will be used in real life so we can see if that is workable in those cases? cc: @fedekunze
Also, please wrap all callbacks to allow maximum flexibility to underlying apps
Our main use case is to enable IBC hooks that can execute trigger different functionality (eg: claiming airdrop tokens) |
@thomas-nguy sorry that this is just being brought up now, but have you thought about making your hooks be the middleware application? I had suggested to @fedekunze to do this for claiming airdrop tokens and @AdityaSripal pointed out that this is also possible for post processing hooks. The middleware would simply call the ICS20 application callbacks and then would perform any custom logic as desired. Acting as a pass through module (not modifying acknowledgements and errors) This would render this pr unnecessary. I think logically it makes sense that ICS20 is a base application that would never need to call additional logic. Any custom logic should simply wrap ICS20 (either performing the logic before or after calling ICS20). Does this make sense? |
@colin-axner thank you for your feedback. No issue, I can close this PR if it become unnecessary Dumb question though, would it be possible to do post processing logic after a "SendTransfer" without this line change? I would requires to override or wrap the channel keeper to add additional logic but this does not seem clean |
Not a dumb question. It is required. The ICS4Wrapper is the interface for middleware applications when we are doing logic which originates from the application and ends up at core IBC. In IBC there are two flows: app -> mw -> core utilizes ICS4Wrapper to achieve this (SendPacket/WriteAcknowledgement) When you hook up your middleware to ICS20, when the line for SendTransfer uses I think it makes sense to close the issue The change from calling |
Thanks for the explanation, yes it is clear. If the change from calling I am closing this PR |
yup #675 thanks again for starting this discussion |
Add cost and api cost options
* WIP Provider interface * Add definitions to the txProvider and begin migration of cosmos specific logic to cosmos provider * get connection based msgs behind TxProvider * finish implementing TxProvider interface * WIP: implementing QueryProvider * finish implementing QueryProvider * Define and implement the KeyProvider interface * Merge PR cosmos#530: Config refactor to handle provider abstraction * WIP: refactor config for provider abstraction * bump gopkg.in/yaml from v2 to v3 * refactor config for provider abstraction * better var naming & doc tweaks * Merge PR cosmos#533: Refactor core relayer code to use provider interface * refactor cli based key management * refactor path cli based commands * testing new config * WIP: refactor chain cli based commands * delete dev and testnet cmds * Merge PR cosmos#534: State Based Relaying, Reenable tests, Add Osmosis to tests * Migrate https://github.com/strangelove-ventures/relayer/pull/35 to this PR * Merge PR cosmos#535: Working tests and Strategy removal * Add osmosis to supported chains for test env * Remove more stuff and add make test-short * Add build osmosis container make file * WIP: push everything behind provider interface * WIP: push everything behind provider interface pt 2 * WIP: swap out old function calls for new provider calls * IT'S ALIVE! (bug fix time) * Most tests working! 🎉🚀💯 * update filename for fetch tests Co-authored-by: Jack Zampolin <[email protected]> * Fix test race conditions * Add back makefile test targets Co-authored-by: jtieri <[email protected]>
Description
Based on interchains-account implementation (https://github.com/cosmos/ibc-go/pull/417/files)
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