[Do not merge] Spike adr-8 #3359
Conversation
| } | ||
|
|
||
| // NewIBCMiddleware constructor | ||
| func NewIBCMiddleware[T CallbackPacketData](next porttypes.Middleware, decoder Decoder[T], executor Executor[T], limits GasLimits) IBCMiddleware[T] { |
There was a problem hiding this comment.
I choose next to be porttypes.Middleware as I plugged in ics29 middleware here. But it may make more sense to use porttypes.IBCModule instead.
| next: next, | ||
| decoder: decoder, | ||
| executor: executor, | ||
| limits: limits, |
There was a problem hiding this comment.
I configured limits with the middleware as this is in the adr-8 example. But when implemented into the executor, then it would be even more flexible
There was a problem hiding this comment.
Adr-8 defines the limits read from the packet not defined here. So this is wrong
| // see comments in https://github.com/cosmos/ibc-go/blob/main/docs/architecture/adr-008-app-caller-cbs/adr-008-app-caller-cbs.md | ||
| // todo: filter out events: https://github.com/cosmos/ibc-go/issues/3358 | ||
| // todo: set event with raw error for debug as defined in ErrorAcknowledgement? | ||
| return channeltypes.NewErrorAcknowledgement(err) |
There was a problem hiding this comment.
There are some downsides with returning error ack, that are outlined well in in the adr:
// NOTE: by returning an error acknowledgement, it is assumed that the
// base IBC application on the counterparty callback stack will be able
// to properly unmarshal the error acknowledgement. It should not expect
// some custom error acknowledgement. If it does, failed acknowledgements
// will be unsuccessfully processed which can be catastrophic in processing
// refund logic.
An alternative would be raising panic to abort the process and reverting state. The data packet would eventually run into a timeout instead
| // see comments in https://github.com/cosmos/ibc-go/blob/main/docs/architecture/adr-008-app-caller-cbs/adr-008-app-caller-cbs.md | ||
| // todo: filter out events: https://github.com/cosmos/ibc-go/issues/3358 | ||
| // todo: set event with raw error for debug as defined in ErrorAcknowledgement? | ||
| return channeltypes.NewErrorAcknowledgement(err) |
There was a problem hiding this comment.
Same as above. Should the callback be able to overwrite the ack or is panic the way to go?
| OnTimeoutPacket(ctx sdk.Context, obj T, relayer sdk.AccAddress) error | ||
| } | ||
| type GasLimits struct { | ||
| OnRecvPacketLimit *sdk.Gas |
There was a problem hiding this comment.
Usage of pointers for value unset instead of 0.
| ) | ||
|
|
||
| // MyICS20Payload my random payload example | ||
| type MyICS20Payload struct { |
There was a problem hiding this comment.
Some example data that would be extracted from memo
| } | ||
|
|
||
| func (m MyICS20Decoder) Decode(packet channeltypes.Packet) (*MyICS20Payload, error) { | ||
| var data types.FungibleTokenPacketData |
There was a problem hiding this comment.
This decoder is for ics-20 packets only and can not be plugged into ics-29 for example
There was a problem hiding this comment.
it would not be hard to provide a generic decoder. Question is if the payload type still makes sense.
Example:
type Memoable interface {
gogoproto.Message
GetMemo() string
}
type MyGenericMemoDecoder [T Memoable] struct {}
func (m MyGenericMemoDecoder[T]) Decode(packet channeltypes.Packet) (*MyICS20Payload, error) {
var data T
if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), data); err != nil {
return nil, err
}
// do we need a sanity check that type was unpacked proper? like denom not empty
if data.GetMemo() == "" {
return nil, nil
}
...| return &r, nil | ||
| } | ||
|
|
||
| var _ Executor[MyICS20Payload] = &MyICS20Callbacks{} |
There was a problem hiding this comment.
Executor for ics20 only. my assumption was that chains may want to customize the callbacks for each of the ibc apps.
| @@ -471,7 +472,12 @@ func NewSimApp( | |||
| // create IBC module from bottom to top of stack | |||
| var transferStack porttypes.IBCModule | |||
| transferStack = transfer.NewIBCModule(app.TransferKeeper) | |||
| transferStack = ibcfee.NewIBCMiddleware(transferStack, app.IBCFeeKeeper) | |||
| transferStack = callbacks.NewIBCMiddleware[callbacks.MyICS20Payload]( | |||
| ibcfee.NewIBCMiddleware(transferStack, app.IBCFeeKeeper), | |||
There was a problem hiding this comment.
This is how the middleware would be integrated
|
Thanks @alpe 🙏 I'm going to close this pr since there won't be any more work done for it and the design discussion has progressed past it. |
Design spike for discussion only.
Ref: