-
Notifications
You must be signed in to change notification settings - Fork 586
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
check fee module locked and enable fee before refunding all fees #1341
check fee module locked and enable fee before refunding all fees #1341
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1341 +/- ##
==========================================
+ Coverage 80.33% 80.35% +0.01%
==========================================
Files 166 166
Lines 12071 12083 +12
==========================================
+ Hits 9697 9709 +12
Misses 1918 1918
Partials 456 456
|
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 a couple more nits and then it's good to merge :)
Also, please add a changelog entry.
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.
Ack, pending suggestions for nits
Co-authored-by: Sean King <seantking@users.noreply.github.com>
@seantking thanks for your thorough reviewing. I updated ! |
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.
Approved but left a suggestion for the changelog :) Great work
Co-authored-by: Sean King <seantking@users.noreply.github.com>
if !im.keeper.IsFeeEnabled(ctx, portID, channelID) { | ||
return types.ErrFeeNotEnabled | ||
} |
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.
It is good for us to think about why we even have "fee enabled channels". This is because non fee enabled channels still use the same middleware stack as fee enabled channels. Thus this check prevents closing of a non fee enabled channel. A key requirement of the middleware design is that middleware not enabled for a specific channel must not affect the behaviour of that channel (this requirement is broken by returning an error).
We should:
- return nil if the fee is not enabled (exit early)
- move this check above
IsLocked
The reasoning for the second point is that we don't want non fee enabled channels to get affected by the fee module getting locked. A locked fee module should not affect non fee enabled channels
btw, thanks for the contribution! :)
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.
Thanks you. It helped me understand. I'll make the changes.
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.
Thanks!
@vuong177 thanks for the contribution :) You'll need to bring the pr up to date with main for it to be merged |
@colin-axner thank you ! |
Description
closes: #1321