[ADR: 015] IBC Packet Receiver #5230
Conversation
mossid
changed the title
fedekunze
added
T: ADR
There was a problem hiding this comment.
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.
Agree with @cwgoes ask if this can be done in a less complex way.
There was a problem hiding this comment.
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? |
mossid
changed the title
|
bump @mossid |
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-Authored-By: Christopher Goes <cwgoes@pluranimity.org>
Co-Authored-By: Christopher Goes <cwgoes@pluranimity.org> Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
There was a problem hiding this comment.
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
routingmodule 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.
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.
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.
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.
Why might this be useful in the IBC case?
There was a problem hiding this comment.
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.
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.
Finalization methods have to be called by each module handler.
There was a problem hiding this comment.
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.
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.
Why is this a top-level function instead of a message handler?
There was a problem hiding this comment.
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 |
Co-Authored-By: Aditya <adityasripal@gmail.com>
|
|
||
| ## 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.
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.
call the corresponding
ChannelKeepers 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.
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.
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.
| 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.
| 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.
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.
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.
ChannelKeeper.CheckOpenmethod 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.
why is this relevant to this ADR?
|
@AdityaSripal Any remaining comments / concerns? |
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
Unreleasedsection inCHANGELOG.mdRe-reviewed
Files changedin the github PR explorerFor Admin Use: