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

Return error if fee module is locked before distributing fees on acknowledgement and timeout #1340

Merged
merged 23 commits into from
May 16, 2022

Conversation

vuong177
Copy link
Contributor

@vuong177 vuong177 commented May 5, 2022

Description

  • Return error if fee module is locked
  • Move DeleteFeesInEscrow into DistributePacketFees func

closes: #1320

@vuong177 vuong177 changed the title Fee module Return error if fee module is locked before distributing fees on acknowledgement and timeout May 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 5, 2022

Codecov Report

Merging #1340 (e4adbd9) into main (ce7ac2e) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1340      +/-   ##
==========================================
+ Coverage   80.27%   80.33%   +0.05%     
==========================================
  Files         166      166              
  Lines       12023    12072      +49     
==========================================
+ Hits         9652     9698      +46     
- Misses       1916     1918       +2     
- Partials      455      456       +1     
Impacted Files Coverage Δ
modules/apps/29-fee/ibc_middleware.go 93.50% <100.00%> (-1.67%) ⬇️
modules/apps/29-fee/keeper/escrow.go 93.78% <100.00%> (+0.41%) ⬆️
modules/apps/29-fee/keeper/keeper.go 92.43% <0.00%> (+0.08%) ⬆️
modules/core/keeper/msg_server.go 58.29% <0.00%> (+0.09%) ⬆️
modules/apps/transfer/keeper/grpc_query.go 78.57% <0.00%> (+0.31%) ⬆️
modules/apps/29-fee/types/msgs.go 89.77% <0.00%> (+0.48%) ⬆️
...7-interchain-accounts/controller/ibc_middleware.go 87.03% <0.00%> (+0.49%) ⬆️
modules/apps/transfer/ibc_module.go 63.88% <0.00%> (+0.76%) ⬆️
... and 1 more

@crodriguezvega
Copy link
Contributor

Hey @vuong177, thanks for opening these PRs! I see that you're with Notional as well, just as @catShaark, right? Do you mind to reach me on Discord (I am Carlos | Interchain#9176) and we can talk about how to make your experience of contributing to ibc-go the best possible?

@vuong177
Copy link
Contributor Author

vuong177 commented May 5, 2022

Hey @vuong177, thanks for opening these PRs! I see that you're with Notional as well, just as @catShaark, right? Do you mind to reach me on Discord (I am Carlos | Interchain#9176) and we can talk about how to make your experience of contributing to ibc-go the best possible?

Yes. @catShaark and me are working at Notional. I'll contact you in discord.

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.

Looking good just a few nits :)

Also, can you add a changelog entry?

vuong177 and others added 7 commits May 9, 2022 18:24
Co-authored-by: Sean King <seantking@users.noreply.github.com>
Co-authored-by: Sean King <seantking@users.noreply.github.com>
Co-authored-by: Sean King <seantking@users.noreply.github.com>
Co-authored-by: Sean King <seantking@users.noreply.github.com>
@vuong177
Copy link
Contributor Author

Looking good just a few nits :)

Also, can you add a changelog entry?

@seantking thank you, i make code cleaner && updated changelog.

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.

Thanks! Great job!

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.

See comment below. The error should be removed from both distribution functions

modules/apps/29-fee/keeper/escrow.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
modules/apps/29-fee/ibc_middleware_test.go Show resolved Hide resolved
modules/apps/29-fee/keeper/escrow.go Outdated Show resolved Hide resolved
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.

I will open a pr with my suggested improvements since I think many of them come from insufficiency in the testing code we should have previously had

modules/apps/29-fee/ibc_middleware_test.go Outdated Show resolved Hide resolved
modules/apps/29-fee/ibc_middleware_test.go Show resolved Hide resolved
modules/apps/29-fee/keeper/escrow.go Outdated Show resolved Hide resolved
modules/apps/29-fee/ibc_middleware_test.go Show resolved Hide resolved
@colin-axner
Copy link
Contributor

see pr

@vuong177
Copy link
Contributor Author

thank you @colin-axner, i learn a lot from your reviews.

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 @vuong177! Just needs to be brought up to date with main

@mergify mergify bot merged commit b7b4400 into cosmos:main May 16, 2022
@crodriguezvega crodriguezvega mentioned this pull request May 30, 2022
9 tasks
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 if fee module is locked before distributing fees on acknowledgement and timeout
5 participants