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

check fee module locked and enable fee before refunding all fees #1341

Merged
merged 11 commits into from
May 16, 2022

Conversation

vuong177
Copy link
Contributor

@vuong177 vuong177 commented May 5, 2022

Description

  • Add check locked fee module and fee is enabled before refunding all fees

closes: #1321


@codecov-commenter
Copy link

codecov-commenter commented May 5, 2022

Codecov Report

Merging #1341 (5e78870) into main (9f70a07) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
modules/apps/29-fee/ibc_middleware.go 94.18% <100.00%> (+0.43%) ⬆️

Copy link
Contributor

@seantking seantking left a 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.

Copy link
Member

@damiannolan damiannolan left a 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

@vuong177
Copy link
Contributor Author

vuong177 commented May 9, 2022

Just a couple more nits and then it's good to merge :)

Also, please add a changelog entry.

@seantking thanks for your thorough reviewing. I updated !

@vuong177 vuong177 requested a review from seantking May 9, 2022 17:46
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@seantking seantking left a 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

modules/apps/29-fee/ibc_middleware.go Outdated Show resolved Hide resolved
Comment on lines 175 to 177
if !im.keeper.IsFeeEnabled(ctx, portID, channelID) {
return types.ErrFeeNotEnabled
}
Copy link
Contributor

@colin-axner colin-axner May 11, 2022

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! :)

Copy link
Contributor Author

@vuong177 vuong177 May 12, 2022

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.

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Thanks!

@colin-axner
Copy link
Contributor

@vuong177 thanks for the contribution :) You'll need to bring the pr up to date with main for it to be merged

@vuong177
Copy link
Contributor Author

@colin-axner thank you !

@mergify mergify bot merged commit 5d83421 into cosmos:main May 16, 2022
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 fees on channel close if fee module is not locked and fee is enabled
6 participants