-
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
refactor: Fix RefundFeesOnChannel #1244
Conversation
// If the escrow account runs out of balance then fee module will become locked as this implies the presence | ||
// of a severe bug. When the fee module is locked, no fee distributions will be performed. | ||
// Please see ADR 004 for more information. | ||
func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID string) error { |
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'm starting to wonder if this function should never return an error. The only place it errors is with regards to the refund address. We could either panic or lock the fee module since this is a bug. If we decide to change it, I think it'd be best to do in a follow up pr
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 its fine for now 👍
Agree on follow up if things change!
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.
Nice work! 🚀
if k.bankKeeper.BlockedAddr(refundAddr) { | ||
continue |
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.
Clean solution!
modules/apps/29-fee/keeper/keeper.go
Outdated
packetFees := k.MustUnmarshalFees(iterator.Value()) | ||
if cb(packetFees) { | ||
break | ||
packetId, err := types.ParseKeyFeesInEscrow(string(iterator.Key())) |
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.
mega nit: packetID
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.
nice catch!
refundAcc := suite.chainA.SenderAccount.GetAddress() | ||
packetID1 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) | ||
packetID2 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 2) | ||
packetID5 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 51) |
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.
How come you skipped packetID 3 and 4? :D
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.
just to do a little fuzz testing. It isn't necessarily true that all packets on a channel will be incentivized so I figured it didn't hurt to put things out of order a little
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.
True, that's a good point!
fee := types.Fee{ | ||
AckFee: defaultAckFee, | ||
RecvFee: defaultReceiveFee, | ||
TimeoutFee: defaultTimeoutFee, | ||
} |
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: could use types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee)
feel free to ignore
fee = types.Fee{ | ||
RecvFee: validCoins, | ||
AckFee: validCoins2, | ||
TimeoutFee: validCoins3, | ||
} |
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.
should we rename validCoins
to defaultRecvFee
..etc, to be consistent with keeper tests?
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 thought about this as well. I think we should, but would prefer to have this be done in a followup since this pr already has a bit of diffs
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.
Yeah, no worries. Sounds good to me :)
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 decided to do it in this pr
// If the escrow account runs out of balance then fee module will become locked as this implies the presence | ||
// of a severe bug. When the fee module is locked, no fee distributions will be performed. | ||
// Please see ADR 004 for more information. | ||
func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID string) error { |
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 its fine for now 👍
Agree on follow up if things change!
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.
Nice work :)
…aultAckFee, rename validCoins3 to defaultTimeoutFee in tests
…bc-go into colin/860-refund-fees-onclosure
…osmos#1244) <!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. --> ## Overview This PR updates the release CI to trigger when a PR with the `create-release` label is merged. See tested action: https://github.com/MSevey/workflows/actions/runs/6489995567 <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. --> ## Checklist <!-- Please complete the checklist to ensure that the PR is ready to be reviewed. IMPORTANT: PRs should be left in Draft until the below checklist is completed. --> - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords
Description
read #1060
closes: #860
closes: #780
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