feat(inflation): add burn method #1823
Conversation
WalkthroughThe recent updates focus on enhancing the inflation module of the Nibiru blockchain application by introducing a token burning capability. This includes adding permissions for burning tokens, introducing new protobuf definitions for the burn functionality, and implementing the burn method across various components. The changes aim to allow users to burn tokens, thereby affecting the token supply and potentially influencing inflation rates. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
x/inflation/types/tx.pb.gois excluded by:!**/*.pb.gox/inflation/types/tx.pb.gw.gois excluded by:!**/*.pb.gw.go
Files selected for processing (8)
- app/keepers.go (1 hunks)
- proto/nibiru/inflation/v1/tx.proto (3 hunks)
- x/inflation/keeper/keeper.go (1 hunks)
- x/inflation/keeper/keeper_test.go (1 hunks)
- x/inflation/keeper/msg_server.go (1 hunks)
- x/inflation/keeper/msg_server_test.go (1 hunks)
- x/inflation/types/interfaces.go (1 hunks)
- x/inflation/types/msgs.go (3 hunks)
Additional comments: 3
x/inflation/keeper/msg_server.go (1)
- 50-62: The implementation of the
Burnmethod in themsgServerstruct correctly unwraps the context, validates the sender address, and calls theBurnmethod on the keeper. However, there's no explicit validation for themsg.Coinfield within this method. WhileValidateBasicon the message likely covers this, relying solely on message validation for critical checks can be risky, especially if the message structure or validation logic changes in the future.Consider adding explicit validation for the
msg.Coinfield within theBurnmethod to ensure robustness.proto/nibiru/inflation/v1/tx.proto (1)
- 4-28: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [7-75]
The protobuf definitions for the new burn functionality are correctly structured and include all necessary components for the feature. However, it's important to ensure that the
cosmos/base/v1beta1/coin.protoimport is used consistently across the project to avoid version mismatches or deprecation issues in the future.Verification successful
The verification process has confirmed that the import statement
import "cosmos/base/v1beta1/coin.proto"is used consistently across various protobuf files within the project. This consistency is crucial for maintaining compatibility and avoiding version mismatches or deprecation issues with thecosmos/base/v1beta1/coin.protofile. Therefore, the original review comment regarding the need to ensure consistent use of this import across the project is validated by the evidence gathered from the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `cosmos/base/v1beta1/coin.proto` is used consistently across all protobuf files. rg "import \"cosmos/base/v1beta1/coin.proto\"" --type protoLength of output: 89
Script:
#!/bin/bash # Verify that `cosmos/base/v1beta1/coin.proto` is used consistently across all protobuf files by including .proto files explicitly in the search. rg "import \"cosmos/base/v1beta1/coin.proto\"" --glob "*.proto"Length of output: 1583
app/keepers.go (1)
- 797-797: The update to include
authtypes.Burnerpermission forinflationtypes.ModuleNameis correctly implemented and aligns with the PR's objective to introduce a coin burning feature. This change enables thex/inflationmodule to burn coins, enhancing the Nibiru Chain's capabilities in managing its monetary policy.
| SendCoinsFromAccountToModule( | ||
| ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins, | ||
| ) error |
There was a problem hiding this comment.
The addition of SendCoinsFromAccountToModule to the BankKeeper interface is crucial for the burn functionality. However, the method lacks documentation. It's important for interface methods, especially those that are part of public APIs, to have clear, concise documentation explaining their purpose, parameters, and expected behavior.
Add documentation to the SendCoinsFromAccountToModule method to improve code readability and maintainability.
| func (k Keeper) Burn(ctx sdk.Context, coins sdk.Coins, sender sdk.AccAddress) error { | ||
| if err := k.bankKeeper.SendCoinsFromAccountToModule( | ||
| ctx, sender, types.ModuleName, coins, | ||
| ); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return k.bankKeeper.BurnCoins(ctx, types.ModuleName, coins) |
There was a problem hiding this comment.
The Burn method in the Keeper struct correctly transfers coins from an account to the module before burning them. This method should ensure that the coins are indeed transferred to a "burn" account or somehow marked as burned to prevent their reuse. Additionally, consider adding more detailed logging for auditability, especially for operations like burning coins which are critical to the monetary policy.
Enhance the Burn method with detailed logging to improve traceability and auditability of burn operations.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (2 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 67-67: The documentation of the addition of the burn method to the inflation module is clear and follows the conventions of Keep a Changelog. It's correctly placed under the "State Machine Breaking" section, indicating its impact on the state machine.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/inflation/keeper/keeper_test.go (1 hunks)
- x/inflation/keeper/msg_server_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/inflation/keeper/keeper_test.go
- x/inflation/keeper/msg_server_test.go
| return err | ||
| } | ||
|
|
||
| return k.bankKeeper.BurnCoins(ctx, types.ModuleName, coins) |
There was a problem hiding this comment.
Is there logging done in the bank keeper so this can be picked up by heart monitor?
There was a problem hiding this comment.
We can use the query of the supply from the bank module
nibid q bank total
There was a problem hiding this comment.
That is one thing missing from the tests. They don't check if the total supply response changes after burning
* feat(inflation): add burn method * Update CHANGELOG.md
Purpose / Abstract
Allows x/inflation to burn any coin. We are currently missing this feature on Nibiru Chain.
There are plans to expose it on the native cosmos-sdk x/bank module, but it's not released yet.
Note that it requires adding the
authtypes.Burnerpermission to theinflationmodule account, since that's the account that receives the coins from the user and burns it.Summary by CodeRabbit