Skip to content
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

fix: disallow incentivizing packets which have not been sent or have been already relayed #1347

Merged
merged 3 commits into from
May 12, 2022

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented May 11, 2022

Description

closes: #1318


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.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov-commenter
Copy link

Codecov Report

Merging #1347 (63c122f) into main (ab8ab42) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1347      +/-   ##
==========================================
+ Coverage   80.29%   80.31%   +0.01%     
==========================================
  Files         166      166              
  Lines       12047    12059      +12     
==========================================
+ Hits         9673     9685      +12     
  Misses       1918     1918              
  Partials      456      456              
Impacted Files Coverage Δ
modules/apps/29-fee/keeper/keeper.go 92.43% <100.00%> (+0.08%) ⬆️
modules/apps/29-fee/keeper/msg_server.go 94.54% <100.00%> (+1.21%) ⬆️

return nil, sdkerrors.Wrapf(channeltypes.ErrSequenceSendNotFound, "channel does not exist, portID: %s, channelID: %s", msg.PacketId.PortId, msg.PacketId.ChannelId)
}

// only allow incentivizing of packets which have been sent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also check if the channel is in an OPEN state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great question. The answer isn't immediately obvious. I would initially think it is a question of supporting optimistic sends, but we also need to consider the case where a channel closes.

If a channel closes after a packet is sent, it is possible a user may want to incentivize a timeout relayer to get their packet timed out. I think that is a strong enough reason to allow incentivization on any channel state (so long as the channel exists)

@seantking
Copy link
Contributor

Clean :) Just one question regarding checking if the channel is OPEN.

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@mergify mergify bot merged commit 10dc929 into main May 12, 2022
@mergify mergify bot deleted the colin/1318-error-packet-dne branch May 12, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return error when trying to incentivize an already relayed packet
4 participants