-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[ADR: 015] IBC Packet Receiver #5230
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.
Hmm, I am not sure if I like introducing this complexity into the core architecture as opposed to just having modules call CacheContext
themselves (perhaps with a wrapper function to make it easier).
How do you plan to deal with acknowledgements with this routing system? That is a relevant criterion, I think.
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.
Agree with @cwgoes ask if this can be done in a less complex way.
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 several questions regarding this ADR? It's not clear what's the current problem outlined here. How is this related to ICS26 (not only packet receiving but in general) and ICS04 ()? You also only mentioned handlePacketRecv
which is only one of the 2 packet relays from ICS26.
It'd be helpful to understand the underlying problem with respect to the packet lifecycle in the context of the SDK and how does this ADR solves it effectively.
also, what are the pros/cons of implementing this ADR instead of the full routing module? |
bump @mossid |
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-Authored-By: Christopher Goes <cwgoes@pluranimity.org>
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.
How acknowledgements are dealt with and how reversions work is still unclear to me.
Co-Authored-By: Christopher Goes <cwgoes@pluranimity.org> Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
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.
Split the dynamic capability question to a separate issue. One more concern that I missed last time.
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
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.
tldr:
- Let's merge or split the concept of the IBCAnteHandler for packet validation.
- From the cons stated here and the previous comments @cwgoes and I raised, I'd vote for not implementing the rest of this ADR and just implement the
routing
module from ICS26 from scratch.
ACKed concept of IBCAnteHandler
. I think that we can merge that regardless of the comments. As I commented prev, I'd like to see where is it going to be located in the chain of decorators (before or after which decorator):
func NewAnteHandler(ak keeper.AccountKeeper, supplyKeeper types.SupplyKeeper, sigGasConsumer SignatureVerificationGasConsumer) sdk.AnteHandler {
return sdk.ChainAnteDecorators(
NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first
NewMempoolFeeDecorator(),
NewValidateBasicDecorator(),
NewValidateMemoDecorator(ak),
NewConsumeGasForTxSizeDecorator(ak),
NewSetPubKeyDecorator(ak), // SetPubKeyDecorator must be called before all signature verification decorators
NewValidateSigCountDecorator(ak),
NewDeductFeeDecorator(ak, supplyKeeper),
NewSigGasConsumeDecorator(ak, sigGasConsumer),
NewSigVerificationDecorator(ak),
NewIncrementSequenceDecorator(ak), // innermost AnteDecorator
)
}
From the ICS26#synopsis
The routing module keeps a lookup table of modules, which it can use to look up and call a module when a packet is received, so that external relayers need only ever relay packets to the routing module.
Correct me if I'm wrong, but the lookup table is being replaced by implementing the func (k PortKeeper) ReceivePacket()
on this ADR?
Also, the callbacks are not being called rn on the ibc-alpha
(as they are supposed to be handled by the routing module). Are we going to deprecate them, what's the strategy there?
There's also the packet cleanup, which I don't see how is being addressed.
`baseapp.runMsgs` will break the loop over the messages if one of the handlers | ||
returns `!Result.IsOK()`. However, the outer logic will write the cached | ||
store if `Result.IsOK() || Result.Code.IsBreak()`. `Result.Code.IsBreak()` if | ||
`Result.Code == CodeTxBreak`. |
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.
Could you explain why we might want to write the cached logic sometimes even if one of the messages fails?
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.
Because we want to persist sequence number increments & commitments even if IBC packet execution fails. It's basically analogous to nonces on accounts incrementing even if the tx fails in DeliverTx
.
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.
Because we want to persist sequence number increments & commitments even if IBC packet execution fails. It's basically analogous to nonces on accounts incrementing even if the tx fails in DeliverTx.
this needs to be written down
Calling those functions implies that the application logic has successfully executed. | ||
However, the handlers can return `Result` with `CodeTxBreak` after calling those methods | ||
which will persist the state changes that has been already done but prevent any further | ||
messages to be executed in case of semantically invalid packet. In any case the |
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.
Why might this be useful in the IBC case?
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.
See above response (but this should be added to the ADR).
} | ||
``` | ||
|
||
Each application handler should call respective finalization methods on the `PortKeeper` |
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.
Will the finalization methods have to be called by each module handler or do we want to handle it automatically?
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.
Finalization methods have to be called by each module handler.
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.
Finalization methods have to be called by each module handler.
please write that down in the ADR
`baseapp.runMsgs` will break the loop over the messages if one of the handlers | ||
returns `!Result.IsOK()`. However, the outer logic will write the cached | ||
store if `Result.IsOK() || Result.Code.IsBreak()`. `Result.Code.IsBreak()` if | ||
`Result.Code == CodeTxBreak`. |
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.
Because we want to persist sequence number increments & commitments even if IBC packet execution fails. It's basically analogous to nonces on accounts incrementing even if the tx fails in DeliverTx
.
```go | ||
type AppModule struct {} | ||
|
||
func (module AppModule) CheckChannel(portID, channelID string, channel Channel) error { |
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.
Why is this a top-level function instead of a message handler?
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.
CheckChannel
will be provided to ChannelKeeper.Port()
at the top level application. Will add to the doc.
### Negative | ||
|
||
- Cannot support dynamic ports, routing is tied to the baseapp router | ||
Dynamic ports can be supported using hierarchical port identifier, see #5290 for detail |
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 isn't a negative.
Co-Authored-By: Aditya <adityasripal@gmail.com>
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.
ACK modulo minor comments
|
||
## Context | ||
|
||
[ICS 26 - Routing Module](https://github.com/cosmos/ics/tree/master/spec/ics-026-routing-module) defines a function [`handlePacketRecv`](https://github.com/cosmos/ics/tree/master/spec/ics-026-routing-module#packet-relay). |
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 line doesn't have any context. Is it incomplete?
|
||
`PortKeeper` will have the capability key that is able to access only the | ||
channels bound to the port. Entities that hold a `PortKeeper` will be | ||
able to call the corresponding `ChannelKeeper`s method, but only with the |
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.
call the corresponding
ChannelKeeper
s method
Which corresponding method?
easily construct a capability-safe `PortKeeper`. This will be addressed in | ||
another ADR and we will use unsecure `ChannelKeeper` for now. | ||
|
||
`baseapp.runMsgs` will break the loop over the messages if one of the handlers |
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 paragraph has no context
`baseapp.runMsgs` will break the loop over the messages if one of the handlers | ||
returns `!Result.IsOK()`. However, the outer logic will write the cached | ||
store if `Result.IsOK() || Result.Code.IsBreak()`. `Result.Code.IsBreak()` if | ||
`Result.Code == CodeTxBreak`. |
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.
Because we want to persist sequence number increments & commitments even if IBC packet execution fails. It's basically analogous to nonces on accounts incrementing even if the tx fails in DeliverTx.
this needs to be written down
|
||
```go | ||
// Pseudocode | ||
func (app *BaseApp) runTx(tx sdk.Tx) (result sdk.Result) { |
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.
func (app *BaseApp) runTx(tx sdk.Tx) (result sdk.Result) { | |
func (app *BaseApp) runTx(tx Tx) (result Result) { |
return res | ||
} | ||
|
||
msCache.Write() |
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.
where is this defined?
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.
be called by the application handlers after the execution. | ||
|
||
```go | ||
func (keeper ChannelKeeper) WriteAcknowledgement(ctx Context, packet Packet, ack []byte) { |
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 provide more context for these function definitions?
} | ||
``` | ||
|
||
Each application handler should call respective finalization methods on the `PortKeeper` |
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.
Finalization methods have to be called by each module handler.
please write that down in the ADR
In any case the application modules should never return state reverting result, | ||
which will make the channel unable to proceed. | ||
|
||
`ChannelKeeper.CheckOpen` method will be introduced. `ChannelChecker` function will be |
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.
ChannelKeeper.CheckOpen
method will be introduced.
why?
} | ||
``` | ||
|
||
The implementation of this ADR will also change the `Data` field of the `Packet` type from `[]byte` (i.e. arbitrary data) to `PacketDataI`. This also removes the `Timeout` field from the `Packet` struct. This is because the `PacketDataI` interface now contains this information. You can see details about this in [ICS04](https://github.com/cosmos/ics/tree/master/spec/ics-004-channel-and-packet-semantics#definitions). |
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.
why is this relevant to this ADR?
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.
ACK 🎉, pending merge conflicts being resolved
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.
🚢
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.
ACK -- fixed some typos and linting a bit
@AdityaSripal Any remaining comments / concerns? |
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.
ACK! Thank you @mossid, ADR is a lot clearer now
Codecov Report
@@ Coverage Diff @@
## master #5230 +/- ##
==========================================
- Coverage 54.53% 54.51% -0.02%
==========================================
Files 311 311
Lines 18740 18740
==========================================
- Hits 10219 10217 -2
- Misses 7741 7743 +2
Partials 780 780
|
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added a relevant changelog entry to the
Unreleased
section inCHANGELOG.md
Re-reviewed
Files changed
in the github PR explorerFor Admin Use: