imp(fee): capital efficient fee middleware #5571

srdtrk · 2024-01-10T16:20:10Z

Description

closes: #5509

Commit Message / Changelog Entry

imp(fee): capital efficient fee middleware

see the guidelines for commit messages. (view raw markdown for examples)

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 the 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 and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

codecov-commenter · 2024-01-10T16:26:39Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f3c76cd) 81.22% compared to head (c689769) 81.19%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5571      +/-   ##
==========================================
- Coverage   81.22%   81.19%   -0.04%     
==========================================
  Files         197      198       +1     
  Lines       15258    15284      +26     
==========================================
+ Hits        12394    12410      +16     
- Misses       2399     2406       +7     
- Partials      465      468       +3

Files	Coverage Δ
modules/apps/29-fee/keeper/escrow.go	`92.76% <100.00%> (-0.05%)`	⬇️
modules/apps/29-fee/keeper/migrations.go	`100.00% <100.00%> (ø)`
modules/apps/29-fee/types/fee.go	`82.35% <100.00%> (+0.35%)`	⬆️
modules/apps/29-fee/module.go	`48.93% <60.00%> (+0.09%)`	⬆️

... and 1 file with indirect coverage changes

charleenfei

generally lgtm! just a few small nits about testing/godocs/inline docs

charleenfei · 2024-01-11T15:40:25Z

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

+			"success: some refund amount",
+			func() {
+				// set the recv + ack fee to be greater than timeout fee so that the refund amount is non-zero
+				fee = types.NewFee(sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(200))), defaultAckFee, defaultTimeoutFee)


do we need a new fee testing var here? can't we just use continue using packetFee and a const for fee?

It was defined previously. I didn't wanna remove it and break tests.

charleenfei · 2024-01-11T15:44:52Z

modules/apps/29-fee/transfer_test.go

@@ -65,7 +65,7 @@ func (suite *FeeTestSuite) TestFeeTransfer() {
 	)

 	suite.Require().Equal(
-		fee.AckFee.Add(fee.TimeoutFee...), // ack fee paid, timeout fee refunded
+		fee.AckFee, // ack fee paid, no refund needed since timeout_fee = recv_fee + ack_fee


i find this comment slightly confusing, it's more that the timeout fee wasn't escrowed since it was smaller than the recv+ack, correct?

In this case, there is equality due to hard-coded values

charleenfei · 2024-01-11T15:51:10Z

modules/apps/29-fee/types/fee.go

@@ -60,8 +60,10 @@ func NewFee(recvFee, ackFee, timeoutFee sdk.Coins) Fee {
 }

 // Total returns the total amount for a given Fee
+// The total amount is the Max(TimeoutFee, AckFee + RecvFee)


maybe just a bit more explicit that only the max of these two will actually be escrowed?

Maybe also indicate the reasoning for this decision (to efficiently escrow capital)?

colin-axner

Superb work! I'm very happy with how few diffs there are for non test code! I think it'd be nice if we added some documentation for this behaviour? Maybe a quick section on the main docs? Feels like a behaviour that would be useful for users to find out about

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

modules/apps/29-fee/ibc_middleware_test.go

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

colin-axner · 2024-01-11T16:49:48Z

modules/apps/29-fee/types/fee.go

@@ -60,8 +60,10 @@ func NewFee(recvFee, ackFee, timeoutFee sdk.Coins) Fee {
 }

 // Total returns the total amount for a given Fee
+// The total amount is the Max(TimeoutFee, AckFee + RecvFee)


Maybe also indicate the reasoning for this decision (to efficiently escrow capital)?

modules/apps/29-fee/types/fee.go

AdityaSripal

Great work!! I noticed the tests on refund are doing exactly the same logic as the distribution logic.

While this is correct, if for whatever reason the functions are not doing what we expected them to do, we won't catch it. Effectively, I don't think we're testing anything real since we're checking if assert(Z - A - B = Z - A - B)

I think I'd be a bit more confident, if you hardcoded the expected refund amount in the tests, at least where you are explicitly trying to assert refund logic is correct.
So something more like assert(Z - A - B = 15)

Everything else looks great, approved after that

modules/apps/29-fee/types/fee.go

AdityaSripal · 2024-01-11T17:27:18Z

Also, if the fees are all in one-denom, then we should definitely have tests that the multi-denom case works as we expect

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

modules/apps/29-fee/ibc_middleware_test.go

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

srdtrk · 2024-01-15T17:04:10Z

@colin-axner and @crodriguezvega, I will open a separate issue and PR for docs, since this PR is time sensitive and should not be blocked on documentation nits

AdityaSripal

Migration logic looks good! still have my point about tests. happy to defer to team on this

bump: #5571 (review)

srdtrk · 2024-01-16T09:01:30Z

@AdityaSripal I think the tests have a healthy mix of hard-coded (like defaultFees) and computed values. If all values were hard-coded, it'd have been a nightmare to update tests.

Also, the current state of tests reflects the state of tests before the PR. I think such a refactor should be a separate issue if others agree to it

damiannolan

Code looks great, making my way through the tests. Figured I share this initial comment on the fee Total() func

modules/apps/29-fee/types/fee.go

damiannolan

Awesome work @srdtrk! I think I've finished looking through all the tests. Migration code looks great. We should wire up an upgrade handler and add an e2e test in a follow up.

I share the same feeling as @AdityaSripal wrt to hardcoding expected values in tests, might make them easier to parse!

damiannolan · 2024-01-16T10:30:22Z

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

-				// check if the refund acc has been refunded the timeoutFee & recvFee
-				expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultRecvFee[0]).Add(defaultTimeoutFee[0]).Add(defaultRecvFee[0])
+				// check if the refund acc has been refunded the recvFee
+				expectedRefundAccBal := refundAccBal.Add(defaultRecvFee[0]).Add(defaultRecvFee[0])


should this be adding the recv fee twice? or should it add the recv fee + ack fee?

It should be recv_fee twice, this is because, in this test case, only the forward relayer address is invalid, therefore only the recv_fee is refunded. The reason it is added twice is that, packetFee is paid twice in line 153

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

damiannolan · 2024-01-16T10:52:37Z

modules/apps/29-fee/types/fee_test.go

+			sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(0))),
+		},
+		{
+			"success: multiple denoms",


We could add another test or two maybe we some different mutli denom variants too

maybe an example where there's 3 different denoms and one of the fees doesn't contain it? something along those lines

I added another test case. Can you check?

modules/apps/29-fee/types/fee.go

AdityaSripal

Approving to not block merge. As @srdtrk says, we can come to consensus on tests later and apply uniformly

* feat: initial implementation * test: fixed keeper tests * test: fixed types tests * test: all tests passing * test: fixed fee callbacks test * feat: implemented migration * test: added migration tests * docs: updated godocs * imp: review items * imp: review items * imp: review items * docs: updated godocs * test: added multiple denoms test case for total * specify what the migration that fails does in error message --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 6fc8159)

* feat: initial implementation * test: fixed keeper tests * test: fixed types tests * test: all tests passing * test: fixed fee callbacks test * feat: implemented migration * test: added migration tests * docs: updated godocs * imp: review items * imp: review items * imp: review items * docs: updated godocs * test: added multiple denoms test case for total * specify what the migration that fails does in error message --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 6fc8159) Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

colin-axner · 2024-01-17T10:28:53Z

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

+			},
+		},
+		{
+			"success: many fees with multiple denoms in escrow",


I think it'd be nice to have a test case for different incentivized packets. If I understand correctly, this does only a single iteration. If I have the following diffs (removing iteration), the tests pass:

diff --git a/modules/apps/29-fee/keeper/migrations.go b/modules/apps/29-fee/keeper/migrations.go index 3eae34f7e..5d5701e8f 100644 --- a/modules/apps/29-fee/keeper/migrations.go +++ b/modules/apps/29-fee/keeper/migrations.go @@ -27,19 +27,17 @@ func (m Migrator) Migrate1to2(ctx sdk.Context) error { iterator := storetypes.KVStorePrefixIterator(store, []byte(types.FeesInEscrowPrefix)) defer sdk.LogDeferred(ctx.Logger(), func() error { return iterator.Close() }) - for ; iterator.Valid(); iterator.Next() { - feesInEscrow := m.keeper.MustUnmarshalFees(iterator.Value()) + feesInEscrow := m.keeper.MustUnmarshalFees(iterator.Value()) - for _, packetFee := range feesInEscrow.PacketFees { - refundCoins := legacyTotal(packetFee.Fee).Sub(packetFee.Fee.Total()...) + for _, packetFee := range feesInEscrow.PacketFees { + refundCoins := legacyTotal(packetFee.Fee).Sub(packetFee.Fee.Total()...) - refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) - if err != nil { - return err - } - - m.keeper.distributeFee(ctx, refundAddr, refundAddr, refundCoins) + refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) + if err != nil { + return err } + + m.keeper.distributeFee(ctx, refundAddr, refundAddr, refundCoins) }

srdtrk added 5 commits January 10, 2024 17:04

feat: initial implementation

bb6e677

test: fixed keeper tests

f62d6ff

test: fixed types tests

c6b1915

test: all tests passing

07e0798

Merge branch 'main' into serdar/issue#5509-capital-efficient-fee-2

fc00be5

srdtrk added 29-fee change: state machine breaking Issues or PRs that break consensus (need to be released in at least a new minor version) labels Jan 10, 2024

srdtrk added 2 commits January 11, 2024 10:37

test: fixed fee callbacks test

e1beb80

Merge branch 'main' into serdar/issue#5509-capital-efficient-fee-2

a3c0d0c

srdtrk marked this pull request as ready for review January 11, 2024 07:47

srdtrk requested review from colin-axner, AdityaSripal, damiannolan and charleenfei as code owners January 11, 2024 07:47

srdtrk mentioned this pull request Jan 11, 2024

Capital Efficient Fee Escrow #5509

Closed

crodriguezvega added priority PRs that need prompt reviews backport-to-v8.1.x labels Jan 11, 2024

srdtrk added 2 commits January 11, 2024 13:55

Merge branch 'main' into serdar/issue#5509-capital-efficient-fee-2

6748789

Merge branch 'main' into serdar/issue#5509-capital-efficient-fee-2

869fe9e

charleenfei approved these changes Jan 11, 2024

View reviewed changes

Merge branch 'main' into serdar/issue#5509-capital-efficient-fee-2

73907c1

colin-axner approved these changes Jan 11, 2024

View reviewed changes

AdityaSripal requested changes Jan 11, 2024

View reviewed changes

modules/apps/29-fee/types/fee.go Show resolved Hide resolved

colin-axner reviewed Jan 11, 2024

View reviewed changes

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

colin-axner self-requested a review January 11, 2024 18:05

Merge branch 'main' into serdar/issue#5509-capital-efficient-fee-2

28a644f

crodriguezvega reviewed Jan 14, 2024

View reviewed changes

Merge branch 'main' into serdar/issue#5509-capital-efficient-fee-2

005d81d

feat: implemented migration

9eae03e

srdtrk marked this pull request as draft January 15, 2024 14:13

srdtrk added 6 commits January 15, 2024 18:53

test: added migration tests

61b9a6e

Merge branch 'main' into serdar/issue#5509-capital-efficient-fee-2

5275908

docs: updated godocs

4680e3e

imp: review items

4bf0a63

imp: review items

fe72521

imp: review items

2afc9ca

srdtrk marked this pull request as ready for review January 15, 2024 17:03

AdityaSripal reviewed Jan 15, 2024

View reviewed changes

damiannolan reviewed Jan 16, 2024

View reviewed changes

modules/apps/29-fee/types/fee.go Show resolved Hide resolved

damiannolan approved these changes Jan 16, 2024

View reviewed changes

AdityaSripal approved these changes Jan 16, 2024

View reviewed changes

srdtrk and others added 4 commits January 17, 2024 08:52

Merge branch 'main' into serdar/issue#5509-capital-efficient-fee-2

bd33451

docs: updated godocs

351ec27

test: added multiple denoms test case for total

60b8708

specify what the migration that fails does in error message

c689769

srdtrk merged commit 6fc8159 into main Jan 17, 2024
61 of 62 checks passed

srdtrk deleted the serdar/issue#5509-capital-efficient-fee-2 branch January 17, 2024 09:27

mergify bot mentioned this pull request Jan 17, 2024

imp(fee): capital efficient fee middleware (backport #5571) #5634

Merged

colin-axner reviewed Jan 17, 2024

View reviewed changes

colin-axner mentioned this pull request Jan 17, 2024

Add test case for multiple iterations in capital efficient fee migration #5637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imp(fee): capital efficient fee middleware #5571

imp(fee): capital efficient fee middleware #5571

srdtrk commented Jan 10, 2024

codecov-commenter commented Jan 10, 2024 •

edited

Loading

charleenfei left a comment

charleenfei Jan 11, 2024

srdtrk Jan 15, 2024

charleenfei Jan 11, 2024

srdtrk Jan 15, 2024

charleenfei Jan 11, 2024

colin-axner Jan 11, 2024

colin-axner left a comment

colin-axner Jan 11, 2024

AdityaSripal left a comment

AdityaSripal commented Jan 11, 2024

srdtrk commented Jan 15, 2024

AdityaSripal left a comment •

edited

Loading

srdtrk commented Jan 16, 2024

damiannolan left a comment

damiannolan left a comment

damiannolan Jan 16, 2024

srdtrk Jan 17, 2024

damiannolan Jan 16, 2024

damiannolan Jan 16, 2024

srdtrk Jan 17, 2024

AdityaSripal left a comment

colin-axner Jan 17, 2024 •

edited

Loading

imp(fee): capital efficient fee middleware #5571

imp(fee): capital efficient fee middleware #5571

Conversation

srdtrk commented Jan 10, 2024

Description

Commit Message / Changelog Entry

codecov-commenter commented Jan 10, 2024 • edited Loading

Codecov Report

charleenfei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

AdityaSripal commented Jan 11, 2024

srdtrk commented Jan 15, 2024

AdityaSripal left a comment • edited Loading

Choose a reason for hiding this comment

srdtrk commented Jan 16, 2024

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

colin-axner Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jan 10, 2024 •

edited

Loading

AdityaSripal left a comment •

edited

Loading

colin-axner Jan 17, 2024 •

edited

Loading