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.
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
feat: Allow to restrict MintCoins from app.go #10771
feat: Allow to restrict MintCoins from app.go #10771
Changes from 1 commit
153274c
7a345b3
39c8037
5f2fc6b
f41cd99
2014b9e
f7ddd0a
95b6fdd
14140be
fa2b7ee
77e5c4d
8fd7bd9
7dade4b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Wait, why are we calling both old and new restriction functions?
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.
We want it to nest. So I can do
bankKeeper.WithMintCoinsRestriction(restriction1).WithMintCoinsRestriction(restriction2)
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.
Ok the godoc should clearly reflect this, because looking at the method, that's not clear at all.
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.
And I'm not even sure this is the best approach. With this approach, you don't have a clear way of reseting it. Also, you could just instead create a single restriction function that nests all of the "inner" ones.
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.
Why do you need to reset it though? Idea is to give a separate keeper instance to each module in its init. E.g. in Osmosis for the Gamm keepers initialization: https://github.com/osmosis-labs/osmosis/blob/main/app/keepers.go#L205-L208
I want to change that to:
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'm also down for a reset method if thats helpful though!)
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.
In that case you're just providing one function. Where do you nest them? I just don't think this approach is clean. But I'm not suggesting we block on it.
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.
Oh so an example of nesting is then for another module
I'd realistically intend on using it such that we define standard_module_bk, that nests more restrictions as they apply.
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.
Yeah so in my alternative proposal (#10771 (comment)), you could easily create a function that calls both
NewBlockPrefixMintCoinsRestrictionFn
andNewBlockPrefixMintCoinsRestrictionFn
.I personally think this approach is cleaner and easier to understand. But proceed with either approach. I approved the PR.
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.
@ValarDragon proposes a middleware approach, so if we have a bank keeper with default setting we make sure the restriction fn will always be called.
With @alexanderbez approach we need to remember to put include the default check function in all custom functions.