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

chore: make EscrowPacketFee private #1252

Merged
merged 9 commits into from
Apr 14, 2022

Conversation

damiannolan
Copy link
Member

Description

  • Modified EscrowPacketFee to be unexported - escrowPacketFee
  • Moved IsFeeEnabled checks to msg_server.go
  • Removed all references to EscrowPacketFee in tests. Explicitly set fees in escrow and send coins to module account
  • Updated PayPacketFee and PayPacketFeeAsync tests to accommodate unexported escrowPacketFee

closes: #1059


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.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@damiannolan damiannolan marked this pull request as ready for review April 13, 2022 13:03
@seantking
Copy link
Contributor

Annoying chore task. Thanks for looking into it! I'm mostly concerned about leaving out the checks we have in the removed EscrowPacketFee test.

I'm fine with leaving EscrowPacketFee public if it's too much work/not feasible.

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.

ACK pending the additional testing checks in msg_server_test.go as requested by @seantking

Great work! Not a fun pr to take on, but I'm a fan of the code improvement. Makes our tests slightly less nice, but I like having a clear entry point exposed to this functionality. Will prevent external developers from making mistakes. I feel pretty confident that someone would try to directly call EscrowPacketFee instead of sending a PayPacketFee message - thus bypassing the checks we have in place

@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022

Codecov Report

Merging #1252 (eb2120b) into main (e59a6d2) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1252      +/-   ##
==========================================
+ Coverage   80.15%   80.23%   +0.08%     
==========================================
  Files         166      166              
  Lines       11943    11937       -6     
==========================================
+ Hits         9573     9578       +5     
+ Misses       1912     1905       -7     
+ Partials      458      454       -4     
Impacted Files Coverage Δ
modules/apps/29-fee/keeper/escrow.go 90.74% <100.00%> (+2.45%) ⬆️
modules/apps/29-fee/keeper/msg_server.go 93.33% <100.00%> (+15.28%) ⬆️
modules/apps/transfer/keeper/keeper.go 94.20% <0.00%> (-0.17%) ⬇️
...7-interchain-accounts/controller/keeper/account.go 72.72% <0.00%> (ø)
modules/apps/transfer/keeper/genesis.go 89.47% <0.00%> (+6.14%) ⬆️

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 I noticed we don't check for the stored fees in escrow anymore -> https://github.com/cosmos/ibc-go/pull/1252/files#r850491652

Is this something we want to keep? I get it's somewhat redundant but may be a good safety check. I think it's worth adding.

@damiannolan damiannolan enabled auto-merge (squash) April 14, 2022 14:41
@damiannolan damiannolan merged commit acb0e9f into main Apr 14, 2022
@damiannolan damiannolan deleted the damian/1059-private-escrow-packet-fee branch April 14, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make EscrowPacketFee private
4 participants