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

Find Elegant Solution if Fee Escrow account is out of balance #821

Closed
AdityaSripal opened this issue Feb 1, 2022 · 4 comments · Fixed by #1251
Closed

Find Elegant Solution if Fee Escrow account is out of balance #821

AdityaSripal opened this issue Feb 1, 2022 · 4 comments · Fixed by #1251
Assignees
Labels

Comments

@AdityaSripal
Copy link
Member

The current way to deal with the worst-case scenario of when escrow account is out of balance is to disable the fee module on all channels.

However, this will only happen on our channel end. The other channel end will continue with the fee module enabled since there isn't a good way to communicate that our side has disabled fees once the channel is already open.

This will lead to an invalid state where one side has fee enabled and the other side does not.

We should determine a better way to handle this case.

Perhaps we keep fee enabled, but just do not distribute the fee on our side.

Thanks to @soareschen for pointing this out

@AdityaSripal AdityaSripal added 29-fee needs discussion Issues that need discussion before they can be worked on labels Feb 1, 2022
@colin-axner
Copy link
Contributor

colin-axner commented Feb 22, 2022

Should we close the channel? That would allow the counterparty to close as well

I guess maybe we would assume in this case we want to pause the channel until a fix is hard forked in?

It is interesting to think about how a middleware could close a channel (overriding the connected base application)

@colin-axner
Copy link
Contributor

After some discussion:

  • receiving side continues function as normal since it does not use an escrow account
  • sending side disables incentivizing new packets and does not distribute from the escrow account

@colin-axner
Copy link
Contributor

I'm working on this in smaller pieces, but in general:

  • when escrow balance will be negative -> disable all channels
  • when writing an acknowledgement, if the channel is disabled + the fee version is set -> return ics29 ack

@colin-axner
Copy link
Contributor

After some more thought, the existence of insufficient balance of the escrow account doesn't need to disable all channels (whether it occurs on channel closure or fee distribution). Part of these channels will be on the receiving side and thus have no interaction with the escrow module

I'll add a single mapping to a boolean in the keeper which will indicate if the fee escrow module is locked (due to insufficient balance)

@crodriguezvega crodriguezvega added this to the Fee middleware beta milestone Mar 29, 2022
@mergify mergify bot closed this as completed in #1251 Apr 14, 2022
mergify bot pushed a commit that referenced this issue Apr 14, 2022
…istribution (#1251)

## 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.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [x] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [x] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [x] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [x] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [x] Re-reviewed `Files changed` in the Github PR explorer
- [x] Review `Codecov Report` in the comment section below once CI passes
@crodriguezvega crodriguezvega removed the needs discussion Issues that need discussion before they can be worked on label Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants