feat(core/eureka): add writeack, ack handler. #7049
Conversation
DimitrisJim
force-pushed
the
jim/recv-packet-eureka
branch
3 times, most recently
from
d8a7adc to
7c982ec
Compare
DimitrisJim
force-pushed
the
jim/ack-packet-eureka
branch
from
9296097 to
859161b
Compare
| // NOTE: IBC app modules might have written the acknowledgement synchronously on | ||
| // the OnRecvPacket callback so we need to check if the acknowledgement is already | ||
| // set on the store and return an error if so. | ||
| if k.ChannelKeeper.HasPacketAcknowledgement(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence()) { |
There was a problem hiding this comment.
We should be also checking that a receipt does exist. Otherwise, you can write an acknowledgement for a packet you never received
There was a problem hiding this comment.
can see utility of check but can't see steps to get there? Is the idea that something other than the msg server handler will invoke this basically?
Also, this was not present previously unless I'm feeling a bit blind this day
There was a problem hiding this comment.
ah, I see in docs for WriteAck that it assumes a packet receipt has been written
guess we can stop assuming hah
There was a problem hiding this comment.
It was not previously present. Remains to be seen with how it gets called with port-router-refactor, but generally i would expect this to be called by an app. Perhaps not directly but through the msg server
DimitrisJim
force-pushed
the
jim/ack-packet-eureka
branch
from
ec02c8d to
24356d0
Compare
DimitrisJim
force-pushed
the
jim/ack-packet-eureka
branch
from
998ff78 to
a3e70ed
Compare
| packetI exported.PacketI, | ||
| ack exported.Acknowledgement, | ||
| ) error { | ||
| packet, ok := packetI.(channeltypes.Packet) |
There was a problem hiding this comment.
preemptively before #5774 lands, didn't want to need to add to the interface func for PacketI just to get the ProtocolVersion. (Plus, I do not like having to use getters everywhere)
(can rename to chanPacket if we want)
|
|
||
| // TODO: Clean up code when msg server handler routes correctly. | ||
|
|
||
| // need to update chainA's client representing chainB to prove missing ack |
There was a problem hiding this comment.
Ideally replaced soon with a err = path.EndpointB.RecvPacket(packet) call
DimitrisJim
requested review from
colin-axner,
damiannolan and
chatton
as code owners
DimitrisJim
force-pushed
the
jim/ack-packet-eureka
branch
from
a3e70ed to
203f0a6
Compare
| _, err := suite.chainA.App.GetPacketServer().SendPacket(suite.chainA.GetContext(), nil, packet.SourceChannel, packet.SourcePort, packet.DestinationPort, packet.TimeoutHeight, packet.TimeoutTimestamp, packet.AppVersion, packet.Data) | ||
| suite.Require().NoError(err) | ||
|
|
||
| // TODO: Clean up code when msg server handler routes correctly. |
There was a problem hiding this comment.
Should we create an issue to track?
DimitrisJim
requested review from
crodriguezvega,
gjermundgaraba and
bznein
as code owners
|
Description
closes: #6969
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.