chore(ics29): attempt to refund fees on distribution failure #1245

damiannolan · 2022-04-11T13:57:33Z

Description

Attempt to refund fees on distribution failure

closes: #862

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 · 2022-04-11T13:58:06Z

Not sure if this requires more tests to be added.

damiannolan · 2022-04-11T13:58:59Z

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

+	if err != nil && !bytes.Equal(receiver, refundAccAddress) {
+		err := k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAccAddress, fee)
+		if err != nil {
+			return


Is there anything we can do here in the case of both failing? Emit some events?

What would be the goal for emitting the event? Events are mostly just used by relayers I guess.

I think it would be worth logging something to INFO or DEBUG logs. Although I haven't seen SDK modules utilizing logging much.

What would be the goal for emitting the event?

Just some kind of signalling I guess, was just thinking aloud really. I think logging at WARN or ERROR here may be useful, but like you say, most sdk modules don't seem to do much logging.
Feels a little wrong to just silently no-op, that's my only concern.

Yeah, I know what you mean. I guess the problem is that even if we returned an error here we would simply dismiss it at the level above anyway, otherwise, we would err on OnAcknowledgementPacket which would throw away the ack for the ics20 transfer or w/e as well.

I think we're limited in what we can do here so my vote would be some warn/err logs.

Seems like a bug if we cannot refund to the original sender? Maybe we should panic

Hmm, maybe we should... originally @AdityaSripal had suggested to just no-op in the issue.

I guess the no-op is for the case that the other fees were distributed fine but this one errors. If we err or panic the other fees won't ever get distributed.

seantking · 2022-04-11T14:53:35Z

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

-
-		// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
-		ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
+	if err != nil && !bytes.Equal(receiver, refundAccAddress) {


I think it would be worth adding a comment here explaining why this logic is necessary. I find it a bit confusing. 🤔

I think you need to separate these conditions as well. What happens if there is an err and refundAcc == receiver?

Maybe something like:

if err != nil { // fee distribution failed, try to refund to original incentivizer if bytes.Equal(receiver, refundAccAddress) { return err } // attempt to distribute fee to refund address }

Could we add a test case for this case?

If there's an error and refundAcc == receiver then sending to the refundAcc has already failed, right?
That's why I combined the conditions using the logical AND.
The issues writes:

If we are trying to refund to original sender and that fails, then we need to just no-op and continue.

In this case, writeFn() is still called but there would be no state changes to commit (no-op). Do you think this would cause any problems?
I can adapt this tomorrow tho if we feel its better separated.

I'll look at adding a testcase 👍

Tests require some refactoring. In progress!

edit: oh man, just realised some of the same refactoring was merged yesterday in #1239

To be on the safe side I think it makes sense to ensure writeFn() doesn't get called. Otherwise we need to verify that no partial state transitions occurred in x/bank. It might assume state changes are discarded once the error is returned. Nice catch on the refundAcc == receiver transfer already occurring.

Sure, I'll adapt the logic to represent that. I had similar concerns wrt writeFn() but wasn't 100%. Thanks :)

damiannolan · 2022-04-12T12:00:13Z

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

 				// check if the reverse relayer is paid
-				hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0]))


I don't think we should be using HasBalance as it just ensures it is greater than or equal to.

Updated to be explicit.
Fetch the account balances below, pre fee distribution, then assert the required amounts are added.

damiannolan · 2022-04-12T12:04:49Z

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

 			},
 		},
+		// {
+		// 	"invalid refund address: no-op, timeout fee remains in escrow",


I'll see if I can fix this test. Currently failing because we're relying on EscrowPacketFee and malleate() is called after this. If we call malleate() before escrowing the fees, then the RefundAddress (incentivizer) is the module account, which doesn't have balance.

Bit of a mouth full, but hope it makes sense.

The simplest solution is to mutate packetFee.RefundAddress in malleate() after the fees are initially escrowed and before DistributePacketFees is called 👍

colin-axner

Nice work! Thanks for following up on my suggested change

cosmos#1245)  ## Overview Closes: cosmos#1246  ## Checklist  - [x] 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

damiannolan added 2 commits April 11, 2022 15:51

attempt to refund fees if distribution fails

3d86e6d

Merge branch 'main' into damian/862-fee-distribution-failure

e3e29a9

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

damiannolan commented Apr 11, 2022

View reviewed changes

seantking reviewed Apr 11, 2022

View reviewed changes

seantking approved these changes Apr 11, 2022

View reviewed changes

damiannolan added 2 commits April 12, 2022 13:53

adapting logic and updating testcases

4e388d5

Merge branch 'main' into damian/862-fee-distribution-failure

27b42ca

damiannolan commented Apr 12, 2022

View reviewed changes

damiannolan added 2 commits April 12, 2022 14:06

updating inline comments

34c7506

updating refund address testcase

0632873

colin-axner approved these changes Apr 12, 2022

View reviewed changes

Merge branch 'main' into damian/862-fee-distribution-failure

ff6bcc6

damiannolan enabled auto-merge (squash) April 12, 2022 15:16

damiannolan added the automerge label Apr 12, 2022

damiannolan merged commit 960a49e into main Apr 12, 2022

damiannolan deleted the damian/862-fee-distribution-failure branch April 12, 2022 15:22

damiannolan mentioned this pull request Apr 12, 2022

If distribute fee fails, refund to original sender #862

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ics29): attempt to refund fees on distribution failure #1245

chore(ics29): attempt to refund fees on distribution failure #1245

damiannolan commented Apr 11, 2022

damiannolan commented Apr 11, 2022

damiannolan Apr 11, 2022

seantking Apr 11, 2022

damiannolan Apr 11, 2022

seantking Apr 11, 2022

colin-axner Apr 11, 2022

damiannolan Apr 11, 2022

seantking Apr 11, 2022

seantking Apr 11, 2022

colin-axner Apr 11, 2022

colin-axner Apr 11, 2022

damiannolan Apr 11, 2022 •

edited

Loading

damiannolan Apr 12, 2022 •

edited

Loading

colin-axner Apr 12, 2022 •

edited

Loading

damiannolan Apr 12, 2022

damiannolan Apr 12, 2022

damiannolan Apr 12, 2022

damiannolan Apr 12, 2022

colin-axner left a comment

		// check if the reverse relayer is paid
		hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0]))

chore(ics29): attempt to refund fees on distribution failure #1245

chore(ics29): attempt to refund fees on distribution failure #1245

Conversation

damiannolan commented Apr 11, 2022

Description

damiannolan commented Apr 11, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

damiannolan Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

colin-axner Apr 12, 2022 • edited Loading

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

damiannolan Apr 11, 2022 •

edited

Loading

damiannolan Apr 12, 2022 •

edited

Loading

colin-axner Apr 12, 2022 •

edited

Loading