chore: update fee middleware docs to be more explicit about reasoning for removing invariant checks #1705

charleenfei · 2022-07-14T16:53:35Z

Description

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)
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

* update code & test * update proto and adr026 * update CHANGELOG * update cli docs * update broken milestone link

docs/middleware/ics29-fee/fee-distribution.md

crodriguezvega · 2022-07-18T07:48:27Z

docs/middleware/ics29-fee/fee-distribution.md

@@ -27,7 +27,7 @@ If a counterparty payee is not registered for the forward relayer on the destina
 A transaction must be submitted to the destination chain including a `CounterpartyPayee` address of an account on the source chain.
 The transaction must be signed by the `Relayer`.

-Note: If a module account address is used as the `CounterpartyPayee` it is recommended to [turn off invariant checks](https://github.com/cosmos/ibc-go/blob/71d7480c923f4227453e8a80f51be01ae7ee845e/testing/simapp/app.go#L659) for that module. 
+Note: If a module account address is used as the `CounterpartyPayee` it may be necessary to [turn off invariant checks](https://github.com/cosmos/ibc-go/blob/71d7480c923f4227453e8a80f51be01ae7ee845e/testing/simapp/app.go#L659) for that module, if the module has been set as a blocked address in the `BankKeeper`. This is because many modules use invariants to compare internal tracking of module account balances against the actual balance of the account stored in the `BankKeeper`. If a token transfer to the module account occurs without going through this module and updating the account balance of the module on the `BankKeeper`, then invariants may break and unknown behaviour could occur depending on the module implementation. Therefore, it is suggested that either invariant checks are turned off, or the module is removed from the blocked address list on the `BankKeeper` with the understanding that this may result in consequences for module balance accounting.


I know that this was already there, so not part of your PR, but I just realized that the code that is linked in the text turn off invariant checks is for not adding the module account to the list of blocked addresses, right? It's not pointing to code that shows how to turn off invariants, right?

So if I understand it correctly, there are two options here described that the use could choose from:

Either to turn off invariants.

Or to remove the module account from the list of blocked addresses.

If option 1 is chosen, is it still possible to use the module account as counterparty payee? Because in our code all the operations to transfer funds are using the bank keeper methods, so if the module is still in the list of blocked addresses, these operations will fail, right?

correct, the code that is linked is for the blocked addresses

regarding your second question, yes it is possible to use the module account as the counterparty payee. each chain handles blocked addresses differently, so the operation may or may not fail depending on if the module is in the blocked address.

correct, the code that is linked is for the blocked addresses

So should we link instead to code that shows how to turn off invariants instead?

I think I disagree with the recommended approach. If a module requires strict internal accounting (as shown by their usage of invariants), I don't think we can safely suggest to ignore that functionality. In my opinion, given the current state of SDK invariants/interaction with the bank keeper, we should say that a module which is blocked cannot be used as a counterparty payee

The lack of checks preventing payment to the community pool has led to multiple security releases. I think it is more sustainable to request changes to the SDK to support payment to modules without disrupting their internal tracking

Issues on the SDK, #10811, #11369, and #11388 would all help make the requested functionality possible. Essentially you only want modules and their sub functionality blocked if it is a strict requirement for security. Currently some modules might be overly secure (too strict invariants) or may have sub-functionality that doesn't need to follow the same invariant checking (community pool)

i see. one problem with waiting on the SDK to fix this though is that i think the usecase we have pinned down for the first deployments of fee middleware use module accounts as the payee, so it would be an issue if we somehow now technically block it.

the other approach, where we don't technically block it but just STRONGLY recommend against and explain why module accounts are risky to use, still allows this usecase.

Module accounts can be used, just module accounts which are blocked cannot be used

@crodriguezvega is correct that turning off invariants is not sufficient to allow a blocked module to be used with ics29. The module account must be unblocked. Could we recommend that only module accounts which are not blocked can be used with ics29?

docs/middleware/ics29-fee/fee-distribution.md

Co-authored-by: colin axnér <[email protected]>

colin-axner

Nice work, thanks for applying my suggestions. Appreciate the willingness to go back and forth

crodriguezvega

Thank you, @charleenfei!

… for removing invariant checks (#1705) * fix broken link * fix: rm AllowUpdateAfter... check (#1118) * update code & test * update proto and adr026 * update CHANGELOG * update cli docs * update broken milestone link * updated fee middleware docs wrt invariants * second review * update docs to remove language about removing invariants * update docs/middleware/ics29-fee/fee-distribution.md Co-authored-by: colin axnér <[email protected]> Co-authored-by: colin axnér <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> (cherry picked from commit 7f3d71b)

… for removing invariant checks (#1705) (#1994) * fix broken link * fix: rm AllowUpdateAfter... check (#1118) * update code & test * update proto and adr026 * update CHANGELOG * update cli docs * update broken milestone link * updated fee middleware docs wrt invariants * second review * update docs to remove language about removing invariants * update docs/middleware/ics29-fee/fee-distribution.md Co-authored-by: colin axnér <[email protected]> Co-authored-by: colin axnér <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> (cherry picked from commit 7f3d71b) Co-authored-by: Charly <[email protected]>

… for removing invariant checks (#1705) (#1995) * fix broken link * fix: rm AllowUpdateAfter... check (#1118) * update code & test * update proto and adr026 * update CHANGELOG * update cli docs * update broken milestone link * updated fee middleware docs wrt invariants * second review * update docs to remove language about removing invariants * update docs/middleware/ics29-fee/fee-distribution.md Co-authored-by: colin axnér <[email protected]> Co-authored-by: colin axnér <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> (cherry picked from commit 7f3d71b) Co-authored-by: Charly <[email protected]>

… for removing invariant checks (cosmos#1705) (cosmos#1994) * fix broken link * fix: rm AllowUpdateAfter... check (cosmos#1118) * update code & test * update proto and adr026 * update CHANGELOG * update cli docs * update broken milestone link * updated fee middleware docs wrt invariants * second review * update docs to remove language about removing invariants * update docs/middleware/ics29-fee/fee-distribution.md Co-authored-by: colin axnér <[email protected]> Co-authored-by: colin axnér <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> (cherry picked from commit 7f3d71b) Co-authored-by: Charly <[email protected]>

charleenfei added 14 commits June 10, 2022 14:22

fix broken link

96f3c49

Merge branch 'main' of github.com:cosmos/ibc-go

250e45f

Merge branch 'main' of github.com:cosmos/ibc-go

bf3b96b

fix: rm AllowUpdateAfter... check (#1118)

ed4a153

* update code & test * update proto and adr026 * update CHANGELOG * update cli docs * update broken milestone link

Merge branch 'main' of github.com:cosmos/ibc-go

8018627

Merge branch 'main' of github.com:cosmos/ibc-go

08e71e7

Merge branch 'main' of github.com:cosmos/ibc-go

05f50c4

Merge branch 'main' of github.com:cosmos/ibc-go

169ead2

Merge branch 'main' of github.com:cosmos/ibc-go

7c46b84

erge branch 'main' of github.com:cosmos/ibc-go

27998dd

Merge branch 'main' of github.com:cosmos/ibc-go

952b114

Merge branch 'main' of github.com:cosmos/ibc-go

f673132

Merge branch 'main' of github.com:cosmos/ibc-go

78d4616

updated fee middleware docs wrt invariants

ac4034d

crodriguezvega reviewed Jul 14, 2022

View reviewed changes

docs/middleware/ics29-fee/fee-distribution.md Outdated Show resolved Hide resolved

second review

261bf77

charleenfei marked this pull request as ready for review July 15, 2022 11:19

charleenfei requested review from colin-axner, AdityaSripal, seantking, damiannolan and chatton as code owners July 15, 2022 11:19

crodriguezvega reviewed Jul 18, 2022

View reviewed changes

Merge branch 'main' into charly/update_docs_invariants

822b415

crodriguezvega added the backport-to-v4.0.x label Jul 19, 2022

charleenfei added 5 commits July 19, 2022 14:51

Merge branch 'main' into charly/update_docs_invariants

16e24e1

Merge branch 'main' into charly/update_docs_invariants

ce863d1

Merge branch 'main' into charly/update_docs_invariants

076dcaf

Merge branch 'main' into charly/update_docs_invariants

fd5b39a

Merge branch 'main' into charly/update_docs_invariants

dc88634

update docs to remove language about removing invariants

a80b633

colin-axner reviewed Aug 9, 2022

View reviewed changes

docs/middleware/ics29-fee/fee-distribution.md Outdated Show resolved Hide resolved

charleenfei and others added 3 commits August 11, 2022 16:54

update docs/middleware/ics29-fee/fee-distribution.md

415921a

Co-authored-by: colin axnér <[email protected]>

Merge branch 'main' into charly/update_docs_invariants

a0cbad5

Merge branch 'main' into charly/update_docs_invariants

ab103c9

colin-axner approved these changes Aug 11, 2022

View reviewed changes

crodriguezvega approved these changes Aug 11, 2022

View reviewed changes

crodriguezvega added the backport-to-v5.0.x label Aug 11, 2022

Merge branch 'main' into charly/update_docs_invariants

c006389

crodriguezvega merged commit 7f3d71b into main Aug 12, 2022

crodriguezvega deleted the charly/update_docs_invariants branch August 12, 2022 08:36

mergify bot mentioned this pull request Aug 12, 2022

chore: update fee middleware docs to be more explicit about reasoning for removing invariant checks (backport #1705) #1994

Merged

mergify bot mentioned this pull request Aug 12, 2022

chore: update fee middleware docs to be more explicit about reasoning for removing invariant checks (backport #1705) #1995

Merged

chatton mentioned this pull request Aug 24, 2022

Consider using git-cliff to generate CHANGELOG.md #2106

Closed

3 tasks

chatton mentioned this pull request Nov 7, 2022

Add git cliff toml to support generation of changelog #2697

Merged

9 tasks

ulbqb mentioned this pull request Jul 31, 2023

Bump up to v4.3.1 Finschia/ibc-go#19

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update fee middleware docs to be more explicit about reasoning for removing invariant checks #1705

chore: update fee middleware docs to be more explicit about reasoning for removing invariant checks #1705

charleenfei commented Jul 14, 2022

crodriguezvega Jul 18, 2022

crodriguezvega Jul 18, 2022

charleenfei Jul 18, 2022 •

edited

Loading

crodriguezvega Jul 26, 2022

colin-axner Jul 27, 2022 •

edited

Loading

colin-axner Jul 28, 2022 •

edited

Loading

colin-axner Jul 28, 2022

charleenfei Jul 28, 2022

colin-axner Jul 28, 2022

colin-axner Aug 1, 2022

colin-axner left a comment

crodriguezvega left a comment

chore: update fee middleware docs to be more explicit about reasoning for removing invariant checks #1705

chore: update fee middleware docs to be more explicit about reasoning for removing invariant checks #1705

Conversation

charleenfei commented Jul 14, 2022

Description

crodriguezvega Jul 18, 2022

Choose a reason for hiding this comment

crodriguezvega Jul 18, 2022

Choose a reason for hiding this comment

charleenfei Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

crodriguezvega Jul 26, 2022

Choose a reason for hiding this comment

colin-axner Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

colin-axner Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

colin-axner Jul 28, 2022

Choose a reason for hiding this comment

charleenfei Jul 28, 2022

Choose a reason for hiding this comment

colin-axner Jul 28, 2022

Choose a reason for hiding this comment

colin-axner Aug 1, 2022

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

charleenfei Jul 18, 2022 •

edited

Loading

colin-axner Jul 27, 2022 •

edited

Loading

colin-axner Jul 28, 2022 •

edited

Loading