-
Notifications
You must be signed in to change notification settings - Fork 624
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
improve error messages, indicate already relayed packets #184
Conversation
Codecov Report
@@ Coverage Diff @@
## main #184 +/- ##
==========================================
+ Coverage 80.05% 80.23% +0.18%
==========================================
Files 109 109
Lines 6482 6491 +9
==========================================
+ Hits 5189 5208 +19
+ Misses 936 928 -8
+ Partials 357 355 -2
|
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.
Have a couple questions on error messages on ACK/Timeout
@@ -462,6 +470,10 @@ func (k Keeper) AcknowledgePacket( | |||
|
|||
commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) | |||
|
|||
if len(commitment) == 0 { | |||
return sdkerrors.Wrapf(types.ErrPacketCommitmentNotFound, "packet with sequence (%d) has been acknowledged, or timed out. In rare cases the packet was never sent or the packet sequence is incorrect", packet.GetSequence()) |
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.
What do you mean, "in rare cases"? In the case of incorrect relayer behaviour, perhaps?
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.
correct, should I rephrase? My thought process is it seems very unlikely a relayer would have a proof for a packet never sent (I'm not even sure how this would happen). The only case I can think of is if the relayer sets the packet sequence incorrectly
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.
I think it's worth noting that this indicates some relayer misconfiguration, yeah. Very minor though.
@@ -80,6 +80,10 @@ func (k Keeper) TimeoutPacket( | |||
|
|||
commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) | |||
|
|||
if len(commitment) == 0 { | |||
return sdkerrors.Wrapf(types.ErrPacketCommitmentNotFound, "packet with sequence (%d) has been acknowledged or timed out. In rare cases the packet was never sent or the packet sequence is incorrect", packet.GetSequence()) |
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.
Ditto on the rare cases question
Description
ref: #152
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/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes