-
Notifications
You must be signed in to change notification settings - Fork 625
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
feat(core/eureka): add timeout handler #7060
Conversation
suite.Require().NoError(err, "send packet failed") | ||
|
||
// set packet receipt to mock a valid past receive | ||
suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketReceipt(suite.chainB.GetContext(), packet.DestinationPort, packet.DestinationChannel, packet.Sequence) |
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.
Decided to just mock the receive here. I could do an actual receive but then would have to make sure the packet isn't timeout out for when the receive occurs but is timed out for the timeout and it ended up looking more confusing setup than just setting directly
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.
went through 04-channel's TimeoutPacket and VerifyPacketReceiptAbsence. Overall lgtm, only left nit-erinos
proofHeight exported.Height, | ||
nextSequenceRecv uint64, | ||
) error { | ||
// Lookup counterparty associated with our channel and ensure that destination channel |
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 we do protocol version check here too?
|
||
// delete packet commitment to prevent replay | ||
k.ChannelKeeper.DeletePacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) | ||
|
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.
nit: can also do the log here (think we also missed it for Send)
packet channeltypes.Packet, | ||
proof []byte, | ||
proofHeight exported.Height, | ||
nextSequenceRecv uint64, |
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.
considering its relevant only for ordered which we do not have
nextSequenceRecv uint64, | |
_ uint64, |
path := host.PacketReceiptKey(packet.DestinationPort, packet.DestinationChannel, packet.Sequence) | ||
merklePath := types.BuildMerklePath(counterparty.MerklePathPrefix, path) | ||
|
||
if err := k.ClientKeeper.VerifyNonMembership( |
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.
guess a client status check might also be nice here if working it in the tests isn't too much of a pain in the ass
proof, | ||
merklePath, | ||
); err != nil { | ||
return err |
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.
we could wrap errors as 03-connection does, think I missed doing that too.
Quality Gate passed for 'ibc-go'Issues Measures |
Description
closes: #6970
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/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.