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(x/mint)!: Replace InflationCalculationFn with MintFn + simple epoch minting #20363
feat(x/mint)!: Replace InflationCalculationFn with MintFn + simple epoch minting #20363
Changes from 41 commits
603d9a7
ba0b474
cf5fd6b
63a3a89
4cd5f15
6734cab
0959509
dc3e06e
b063cbb
848cbf5
f2f8213
5ae7ce6
7f26121
5abb75f
593baab
61caa0b
969cb1d
faa40de
f89c958
183fb37
67aac7d
9696890
61dab4a
62c9b22
007d50e
aa7fccf
3d73011
18a741f
3b4264d
d80fe8d
ce185c2
ee8d6a6
a497b96
281557d
1c4e6c1
32fe4d9
dc14d2b
ace7bca
e5425a4
2c80fed
ec56d49
1c62f85
0931da6
c002f55
61250af
29970a6
043dc71
d9e30cb
712d5aa
a66e1e8
226091c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
personal opinion: I have seen people just copying the app.go without thinking too much about the configuration. With this change, you make the new mint function default. This may cause some unexpected behaviour and risks.
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 it's fine if this becomes the new default for new apps, we'll get some good side effects from this. For example, we can stop producing empty blocks
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 is this in simapp? I would expect this code to be in the mint module.
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.
Because this is the example that developers can use to develop their own mint function. The idea here is that anyone creates their own minting function if they want to, and it works without requiring direct usage of keepers as it uses the query routers.
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.
Thanks for the explanation! Personally, I would prefer something like
x/mint/example
. I do understand your point of keeping it decoupled but it still relates more the mint package than to simapp for me. This is personal preference of course.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.
Some unit tests on the edge cases would be great
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 the increase ?
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.
Because now it mints the equivalent of a minute in the first block (the beginning of the epoch)
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 does it need to be a pointer?
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.
This was a bug with x/epochs, in which hooks weren't being set. The issue was that we were doing
(k Keeper) SetHooks(...
instead of(k *Keeper) SetHooks(...
. I copied what we do in x/gov to achieve the same