-
Notifications
You must be signed in to change notification settings - Fork 340
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: discard pending rollapp ibc packets upon fraud #653
feat: discard pending rollapp ibc packets upon fraud #653
Conversation
…e-rollapp-should-be-jailed
…d-proposal-the-rollapp-should-be-jailed
…e-rollapp-should-be-jailed
…e-rollapp-should-be-jailed
…dle-fraudsubmitted-hooks
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #653 +/- ##
==========================================
+ Coverage 29.86% 29.97% +0.10%
==========================================
Files 220 221 +1
Lines 30812 30855 +43
==========================================
+ Hits 9202 9248 +46
+ Misses 20205 20195 -10
- Partials 1405 1412 +7 ☔ View full report in Codecov by Sentry. |
|
||
/* ---------------------------------- utils --------------------------------- */ | ||
|
||
func generatePackets(rollappId string, num uint64) []commontypes.RollappPacket { |
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.
imo cleaner to add it to the keeper_test (i.e on the test suite) as a public function.
rollappPacket, err := k.UpdateRollappPacketWithStatus(ctx, rollappPacket, commontypes.Status_REVERTED) | ||
if err != nil { | ||
logger.Error("Error reverting IBC rollapp packet", "rollappID", rollappID, "packetId", packetId, "type", rollappPacket.Type, "error", err.Error()) | ||
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.
here we'll revert fraud proof on first packet failure. possibly worth wrapping in applyFuncIfNoErr and continue to avoid one packet failing the reversion for all other. especially as the updateRollappPacketWithStatus
triggers various hook so doesn't necessarily mean store error. wdyt
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.
thought about it quite long.
I just think that the alternative of keeping Pending
packets of fraudulent rollapp may cause unexpected behaviour. maybe break invariant
Description
Closes #624
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
PR review checkboxes:
I have...
Unreleased
section inCHANGELOG.md
godoc
commentsSDK Checklist
map
time.Now()
sendCoin
and notSendCoins
Full security checklist here
----;
For Reviewer:
---;
After reviewer approval: