-
Notifications
You must be signed in to change notification settings - Fork 585
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
chore: update fee middleware docs to be more explicit about reasoning for removing invariant checks #1705
Merged
Merged
chore: update fee middleware docs to be more explicit about reasoning for removing invariant checks #1705
Changes from 15 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
96f3c49
fix broken link
charleenfei 250e45f
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei bf3b96b
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei ed4a153
fix: rm AllowUpdateAfter... check (#1118)
charleenfei 8018627
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei 08e71e7
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei 05f50c4
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei 169ead2
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei 7c46b84
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei 27998dd
erge branch 'main' of github.com:cosmos/ibc-go
charleenfei 952b114
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei f673132
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei 78d4616
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei ac4034d
updated fee middleware docs wrt invariants
charleenfei 261bf77
second review
charleenfei 822b415
Merge branch 'main' into charly/update_docs_invariants
charleenfei 16e24e1
Merge branch 'main' into charly/update_docs_invariants
charleenfei ce863d1
Merge branch 'main' into charly/update_docs_invariants
charleenfei 076dcaf
Merge branch 'main' into charly/update_docs_invariants
charleenfei fd5b39a
Merge branch 'main' into charly/update_docs_invariants
charleenfei dc88634
Merge branch 'main' into charly/update_docs_invariants
charleenfei a80b633
update docs to remove language about removing invariants
charleenfei 415921a
update docs/middleware/ics29-fee/fee-distribution.md
charleenfei a0cbad5
Merge branch 'main' into charly/update_docs_invariants
charleenfei ab103c9
Merge branch 'main' into charly/update_docs_invariants
charleenfei c006389
Merge branch 'main' into charly/update_docs_invariants
crodriguezvega File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I understand it correctly, there are two options here described that the use could choose from:
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we link instead to code that shows how to turn off invariants instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module accounts can be used, just module accounts which are blocked cannot be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?