Update WriteAcknowledgementV2 to not depend on channels #7238
Conversation
chatton
requested review from
colin-axner,
AdityaSripal,
damiannolan,
DimitrisJim,
crodriguezvega,
gjermundgaraba and
bznein
as code owners
testing/simapp/app.go
Outdated
| mockModuleV2A := ibcmock.NewIBCModuleV2(ibcmock.NewIBCV2App()) | ||
| ibcAppRouter.AddV2Route(ibcmock.ModuleNameV2+"A", mockModuleV2A) | ||
| app.MockV2ModuleA = mockModuleV2A | ||
|
|
||
| mockModuleV2B := ibcmock.NewIBCModuleV2(ibcmock.NewIBCV2App()) | ||
| ibcAppRouter.AddV2Route(ibcmock.ModuleNameV2+"B", mockModuleV2B) | ||
| app.MockV2ModuleB = mockModuleV2B |
There was a problem hiding this comment.
we could probably design a mock module that is a single module, but allows for dynamic modification of behaviour based on payload/version provided etc.
If anyone feels like we should do that now, happy to give it a go, otherwise can just use this dumber approach of having 2 instances.
| @@ -456,112 +456,6 @@ func (k *Keeper) WriteAcknowledgement( | |||
| return nil | |||
| } | |||
|
|
|||
| // WriteAcknowledgementAsyncV2 TODO: this has not been tested at all. | |||
| func (k *Keeper) WriteAcknowledgementAsyncV2( | |||
There was a problem hiding this comment.
moved to packetServerKeeper
There was a problem hiding this comment.
Great work, @chatton! The test with both mock modules is also very nice.
There are a couple TODOs in the code, do you plan to address them in a follow up soon or would you prefer to open issues to keep track of them?
I am guessing that if we implement channel aliasing (and the source/destination channel fields can be channel identifiers), then we will also have to adjust the code to handle that situation, right?
|
@crodriguezvega yes some of these TODOs are still pending design decisions, e.g. what will |
| @@ -150,7 +150,7 @@ func EmitSendPacketEventV2(ctx sdk.Context, packet types.PacketV2, channel types | |||
| ctx.EventManager().EmitEvents(sdk.Events{ | |||
| sdk.NewEvent( | |||
| types.EventTypeSendPacket, | |||
| //sdk.NewAttribute(types.AttributeKeyDataHex, hex.EncodeToString(packet.GetData())), TODO | |||
| // sdk.NewAttribute(types.AttributeKeyDataHex, hex.EncodeToString(packet.GetData())), TODO | |||
| func (k Keeper) WriteAcknowledgementV2( | ||
| ctx sdk.Context, | ||
| packet channeltypes.PacketV2, | ||
| multiAck channeltypes.MultiAcknowledgement, | ||
| ) error { | ||
| // TODO: this should probably error out if any of the acks are async. |
| @@ -283,9 +283,9 @@ func (k *Keeper) RecvPacketV2( | |||
| return errorsmod.Wrap(err, "couldn't verify counterparty packet commitment") | |||
| } | |||
|
|
|||
| //if err := k.applyReplayProtection(ctx, packet, channel); err != nil { | |||
| // if err := k.applyReplayProtection(ctx, packet, channel); err != nil { | |||
There was a problem hiding this comment.
do you know why this is commented out?
There was a problem hiding this comment.
I think it may have been commented out to keep diff low, it'll require a duplicate v2 version of all the functions down that code path.
I think we can probably come up with a cleaner solution in the end where the v2 code path is the only path anyway.
There was a problem hiding this comment.
also just realised this entire fn doesn't need to exist, channelKeeper.RecvPacketV2 was the impl that existed on the port router branch, we can remove this since the v2 flow will always go through the PacketServerKeeper
| } | ||
|
|
||
| //defer telemetry.ReportRecvPacket(msg.Packet) | ||
| // TODO; move to PacketServerKeeper this will blow up with no channels |
| @@ -283,9 +283,9 @@ func (k *Keeper) RecvPacketV2( | |||
| return errorsmod.Wrap(err, "couldn't verify counterparty packet commitment") | |||
| } | |||
|
|
|||
| //if err := k.applyReplayProtection(ctx, packet, channel); err != nil { | |||
| // if err := k.applyReplayProtection(ctx, packet, channel); err != nil { | |||
There was a problem hiding this comment.
do you know why this is commented out?
|
will merge this one after #7254 since there will be some conflicts |
|
Description
This PR
closes: #XXXX
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.