ics29: gracefully handle escrow out of balance edge case during fee distribution #1251

colin-axner · 2022-04-13T11:12:56Z

Description

closes: #821

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

codecov-commenter · 2022-04-13T11:45:57Z

Codecov Report

Merging #1251 (558e852) into main (eaff98b) will increase coverage by 0.06%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##             main    #1251      +/-   ##
==========================================
+ Coverage   80.17%   80.23%   +0.06%     
==========================================
  Files         166      166              
  Lines       11942    11979      +37     
==========================================
+ Hits         9574     9611      +37     
  Misses       1911     1911              
  Partials      457      457

Impacted Files	Coverage Δ
modules/apps/29-fee/keeper/escrow.go	`91.55% <97.82%> (+2.66%)`	⬆️
modules/apps/29-fee/ibc_module.go	`97.84% <100.00%> (ø)`

modules/apps/29-fee/keeper/escrow_test.go

seantking · 2022-04-13T12:45:37Z

modules/apps/29-fee/keeper/escrow.go

@@ -50,50 +50,76 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId,
 }

 // DistributePacketFees pays the acknowledgement fee & receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee.
-func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, feesInEscrow []types.PacketFee) {
+func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, packetFees []types.PacketFee, isTimeout bool) {


The refactor here is great, I think in some ways splitting up the timeout and ack logic as you have is an improvement. I can't help but feel like I'd prefer to have two separate functions though rather than passing a boolean. I feel like the fact that we're adding a boolean here could be a sign this fn has too much responsibility.

I think I would probably prefer keeping two separate functions like DistributeAcknowledgementPacketFees & DistributeTimeoutPacketFees.

I'm pretty open to either way though :)

Yeah, I agree. I think it might be worth exploring the option of two separate functions in favour of the boolean arg, considering that the forward relayer is always an empty string for timeout scenarios also.

I do appreciate that it would be a decent bit of duplicate code nonetheless, but I would lean towards having explicit APIs for each scenario (happy path/timeout)

My main concern is the sensitivity of performing the cacheCtx. It can be hard to verify that we pass in cacheCtx and ctx in the correct functions. Doing this redundantly in two separate functions seems like a recipe for one of the functions to get out of sync with the other and potentially lead to a bug.

The actual distribution of fees on acknowledgement and fees on timeout are still separated. The logic this function is performing is wrapping packet fee distribution with an escrow out of balance check. The previously functionality not only distributed the packet fees for either ack or timeout but also wrapped it with an escrow out of balance check.

For reference you can see how this file differed before splitting. I actually find it harder to verify the distributeAcknowledgementPacketFees and distirbuteTimeoutPacketFees functionality in comparison to the proposed way distributePacketFeeOnAcknowledgement and distirbutePacketFeeOnTimeout

I agree with @seantking and @damiannolan. I think having two separate functions will make the code more explicit and easier to read.

If that's not possible because of the concerns that @colin-axner has regarding the context, then a couple of other alternatives could be:

To have an enum value instead of a boolean as parameter. Then at least reading the function at call site would look like ..., OnTimeout) or ..., OnAcknowledgement).

To pass the function that actually does the distribution instead of a boolean as parameter. So at call site you would have something like ..., distributePacketFeeOnAcknowledgement) or ..., distributePacketFeeOnTimeout). But then you need to have both functions with the same signature, which is possible to work around, but might not lead to the most beautiful result...

… into separate functions (#1253) ## Description Reduces the complexity contained in `DistributePacketFees` and `DistributePacketFeesOnAcknowledgement` in anticipation of #1251 closes: #XXXX --- 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](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] 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

colin-axner · 2022-04-14T10:45:52Z

modules/apps/29-fee/keeper/escrow_test.go


 			tc.expResult()
 		})
 	}
 }

-func (suite *KeeperTestSuite) TestDistributeTimeoutFee() {
-	suite.coordinator.Setup(suite.path) // setup channel
+func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {


duplicated TestDistributePacketFeesOnAcknowledgement and adjusted the tests

damiannolan

Awesome work! Thanks for going to the trouble of exploring both approaches with this, much appreciated! While there's a bit of duplication with the code I think from an API perspective having no forwardRelayer arg in the timeout scenario looks much cleaner imo.

❤️

modules/apps/29-fee/keeper/escrow.go

damiannolan · 2022-04-14T12:02:22Z

modules/apps/29-fee/keeper/keeper_test.go

@@ -15,7 +15,7 @@ import (
 )

 var (
-	defaultReceiveFee = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}}
+	defaultRecvFee    = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}}


Nice, prefer this too! :)

seantking

Great work!

## Overview Closes: cosmos#1249 Also, fixes `markdown-lint` CI checks.  ## Checklist  - [ ] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords Co-authored-by: Ganesha Upadhyaya <[email protected]>

port code from #1056

4d5a747

colin-axner mentioned this pull request Apr 13, 2022

handle escrow out of balance edge case #1056

Closed

9 tasks

refactor: deduplicate distribution code

ca65d4f

colin-axner added 2 commits April 13, 2022 13:54

add test case for on acknowledgement packet distribution

e91f849

add check for timeout packet distribution

317aaa1

colin-axner commented Apr 13, 2022

View reviewed changes

modules/apps/29-fee/keeper/escrow_test.go Outdated Show resolved Hide resolved

colin-axner marked this pull request as ready for review April 13, 2022 11:58

colin-axner requested review from AdityaSripal, crodriguezvega, seantking, charleenfei and damiannolan as code owners April 13, 2022 11:58

seantking reviewed Apr 13, 2022

View reviewed changes

colin-axner mentioned this pull request Apr 13, 2022

isolate packet fee distribution for on acknowledgement and on timeout into separate functions #1253

Merged

9 tasks

colin-axner added 3 commits April 14, 2022 12:21

merge main

27ccd3b

revert to duplicate code approach

8e73b5a

add tests for DistributePacketFeesOnTimeout

f968b78

colin-axner commented Apr 14, 2022

View reviewed changes

chore: remove unnecessary comment

558e852

damiannolan approved these changes Apr 14, 2022

View reviewed changes

colin-axner added 2 commits April 14, 2022 14:11

add back godoc

b73b439

Merge branch 'main' into colin/821-escrow-out-of-balance-check

3ac48a2

seantking approved these changes Apr 14, 2022

View reviewed changes

colin-axner added the automerge label Apr 14, 2022

mergify bot merged commit 63c5341 into main Apr 14, 2022

mergify bot deleted the colin/821-escrow-out-of-balance-check branch April 14, 2022 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ics29: gracefully handle escrow out of balance edge case during fee distribution #1251

ics29: gracefully handle escrow out of balance edge case during fee distribution #1251

colin-axner commented Apr 13, 2022 •

edited

Loading

codecov-commenter commented Apr 13, 2022 •

edited

Loading

seantking Apr 13, 2022 •

edited

Loading

damiannolan Apr 13, 2022

colin-axner Apr 13, 2022

crodriguezvega Apr 14, 2022

colin-axner Apr 14, 2022

damiannolan left a comment

damiannolan Apr 14, 2022

seantking left a comment

ics29: gracefully handle escrow out of balance edge case during fee distribution #1251

ics29: gracefully handle escrow out of balance edge case during fee distribution #1251

Conversation

colin-axner commented Apr 13, 2022 • edited Loading

Description

codecov-commenter commented Apr 13, 2022 • edited Loading

Codecov Report

seantking Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

damiannolan Apr 13, 2022

Choose a reason for hiding this comment

colin-axner Apr 13, 2022

Choose a reason for hiding this comment

crodriguezvega Apr 14, 2022

Choose a reason for hiding this comment

colin-axner Apr 14, 2022

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan Apr 14, 2022

Choose a reason for hiding this comment

seantking left a comment

Choose a reason for hiding this comment

colin-axner commented Apr 13, 2022 •

edited

Loading

codecov-commenter commented Apr 13, 2022 •

edited

Loading

seantking Apr 13, 2022 •

edited

Loading