-
Notifications
You must be signed in to change notification settings - Fork 12
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
mint: Babylon inflation module #211
Conversation
|
||
// GenesisState defines the mint module's genesis state. | ||
message GenesisState { | ||
reserved 1; // 1 was previously used for the `Minter` field. |
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 not keep the minter? https://github.com/cosmos/cosmos-sdk/blob/b49f948b36bc991db5be431607b475633aed697e/proto/cosmos/mint/v1beta1/genesis.proto#L11
I think this is good information
// minter is a space for holding current inflation information.
Minter minter = 1 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true];
and we could migrate the info during the upgrade
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 this is because in this inflation protocol, the minter object does not depend on the minter object in the previous block, so the protocol does not need to remember the minter in genesis at all. This is different from Cosmos SDK's design where the new inflation depends on the inflation at the previous block.
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.
but what about the minted values during the previous version?
// Although ak.GetModuleAccount appears to be a no-op, it actually creates a | ||
// new module account in the x/auth account store if it doesn't exist. See | ||
// the x/auth keeper for more details. | ||
ak.GetModuleAccount(ctx, types.ModuleName) |
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.
ak.GetModuleAccount(ctx, types.ModuleName)
We should not do this during initGenesis, if there is a need to create a new module account should be done in app.go or during upgrade
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.
Hmm checking the code of other modules and Cosmos SDK, and it seems that this line is not necessary. Removing it then
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.
Tried, and found that we cannot remove it, otherwise will fail.
Checked Cosmos SDK code base and auth module is doing same https://github.com/cosmos/cosmos-sdk/blob/b49f948b36bc991db5be431607b475633aed697e/x/auth/keeper/genesis.go#L28-L29. Probably this is a bug of Cosmos SDK and Celestia people learned it in a hard way?
Wdyt about keeping 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.
okay
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.
LGTM
Resolves https://github.com/babylonlabs-io/pm/issues/92
This PR provides the implementation of the inflation module.
x/mint
, and serves as a drop-in replacement of Cosmos SDK'sx/mint
.Steps for implementation:
x/mint