feat(bank/v2): port tokenfactory into bank/v2 #22264
Conversation
hieuvubk
requested review from
julienrbrt,
akhilkumarpilli,
a team and
sontrinh16
as code owners
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several new message types and modifies existing structures in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@hieuvubk your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 31
🧹 Outside diff range and nitpick comments (21)
x/bank/v2/types/authorityMetadata.go (1)
7-15: LGTM: TheValidate()method is well-implemented.The
Validate()method correctly checks theAdminfield of theDenomAuthorityMetadatatype. It properly handles both empty and non-empty cases, and the error handling is correct. The implementation follows the Uber Go Style Guide by keeping the happy path on the left.A minor suggestion for improvement:
Consider adding a comment to explain the purpose of this validation, especially why an empty
Adminfield is considered valid. This would enhance the code's readability and maintainability.+// Validate checks if the Admin field is either empty or a valid Bech32 address. +// An empty Admin field is considered valid. func (metadata DenomAuthorityMetadata) Validate() error { // ... (rest of the function remains the same) }x/bank/v2/keeper/params.go (2)
9-12: Update the comment to match the method's functionality.The current comment doesn't accurately describe the
SetParamsmethod. Consider updating it to:// SetParams sets the parameters for the bank moduleThe method implementation looks good and correctly sets the parameters using
k.params.Set.
14-21: Update the comment and consider logging the error.
- The current comment doesn't accurately describe the
GetParamsmethod. Consider updating it to:// GetParams returns the current parameters for the bank module
- Consider logging the error before returning the default value. This can help with debugging in case of unexpected issues:
if err != nil { k.Logger(ctx).Error("failed to get params", "error", err) return types.Params{} }The overall implementation of the method looks good, retrieving the parameters and handling potential errors appropriately.
x/bank/proto/cosmos/bank/v2/authorityMetadata.proto (1)
9-17: LGTM: Well-definedDenomAuthorityMetadatamessage with room for future extensions.The
DenomAuthorityMetadatamessage is clearly defined and well-commented. The use ofgogoprotooptions for equality checks and YAML serialization is appropriate. Theadminfield correctly allows for an empty string to represent "no admin".For future consideration:
As the comment mentions planned extensions, consider adding areservedblock for future fields. This can help maintain backwards compatibility as the message evolves.Example:
message DenomAuthorityMetadata { // ... existing fields ... // Reserved for future use reserved 2 to 10; reserved "future_field1", "future_field2"; }x/bank/v2/types/errors.go (2)
3-3: Reconsider the use of// DONTCOVERcommentThe
// DONTCOVERcomment is typically used to exclude files from test coverage reports. However, for an errors file, it's generally beneficial to include it in coverage reports to ensure all error cases are properly tested.Consider removing this comment unless there's a specific reason for excluding this file from coverage reports.
11-19: Error definitions look good, with a minor suggestion for consistencyThe error definitions are well-structured and follow best practices:
- Each error has a unique code and descriptive message.
- The use of
errorsmod.Registeris correct for defining sentinel errors.- Error codes are sequential, which is good for maintainability.
For consistency, consider using
fmt.Sprintffor all error messages, even those without dynamic values. This would make future additions of dynamic content easier. For example:ErrDenomExists = errorsmod.Register(ModuleName, 2, fmt.Sprintf("attempting to create a denom that already exists (has bank metadata)")) ErrUnauthorized = errorsmod.Register(ModuleName, 3, fmt.Sprintf("unauthorized account")) ErrInvalidDenom = errorsmod.Register(ModuleName, 4, fmt.Sprintf("invalid denom")) ErrInvalidCreator = errorsmod.Register(ModuleName, 5, fmt.Sprintf("invalid creator"))x/bank/v2/types/keys.go (1)
35-37: LGTM! Consider adding comments for new prefixes.The new variables
DenomMetadataPrefixandDenomAuthorityPrefixare correctly implemented and follow the existing naming conventions and prefix numbering sequence. This is consistent with the Uber Golang style guide.To improve code documentation, consider adding brief comments explaining the purpose of these new prefixes, similar to the existing comments for other prefixes in this file.
+// DenomMetadataPrefix is the prefix for storing denomination metadata DenomMetadataPrefix = collections.NewPrefix(6) +// DenomAuthorityPrefix is the prefix for storing denomination authority information DenomAuthorityPrefix = collections.NewPrefix(7)x/bank/proto/cosmos/bank/v2/genesis.proto (3)
30-31: LGTM: New denom_metadata field is well-defined.The addition of the
denom_metadatafield to the GenesisState message is appropriate and well-structured. It allows for the inclusion of metadata for different coins in the genesis state.Consider adding a brief comment explaining the purpose and usage of this field, similar to the comments for other fields in the message. For example:
// denom_metadata defines the metadata of the different coins. repeated Metadata denom_metadata = 4 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true];
33-33: LGTM: New factory_denoms field is well-defined.The addition of the
factory_denomsfield to the GenesisState message is appropriate and well-structured. It allows for the inclusion of token factory denominations in the genesis state.Consider adding a brief comment explaining the purpose and usage of this field, similar to the comments for other fields in the message. For example:
// factory_denoms defines the denominations created through the tokenfactory module. repeated GenesisDenom factory_denoms = 5 [(gogoproto.nullable) = false];
54-61: LGTM: New GenesisDenom message is well-defined.The addition of the GenesisDenom message is appropriate and well-structured. It provides a clear representation of token factory denominations in the genesis state.
Consider renaming the
authority_metadatafield toadmin_metadatafor consistency with theadminterminology used in other parts of the Cosmos SDK. This would make the field name more intuitive and aligned with the broader SDK conventions. For example:message GenesisDenom { option (gogoproto.equal) = true; string denom = 1; DenomAuthorityMetadata admin_metadata = 2 [(gogoproto.nullable) = false]; }x/bank/v2/module.go (1)
101-102: LGTM! Consider grouping related handlers together.The addition of
MsgCreateDenomandMsgChangeAdminhandlers is correct and follows the existing pattern. These new handlers align with the PR objectives of introducing functionalities for creating denominations.For better code organization, consider grouping related handlers together. For example:
appmodulev2.RegisterMsgHandler(router, handlers.MsgUpdateParams) appmodulev2.RegisterMsgHandler(router, handlers.MsgSend) appmodulev2.RegisterMsgHandler(router, handlers.MsgMint) appmodulev2.RegisterMsgHandler(router, handlers.MsgCreateDenom) appmodulev2.RegisterMsgHandler(router, handlers.MsgChangeAdmin)This grouping improves readability by keeping denomination-related handlers adjacent to each other.
x/bank/v2/types/params.go (2)
4-4: Remove unnecessary import alias forfmtpackage.The alias
fmt "fmt"is redundant since the package name is the same as the imported package. You can simplify the import statement to:import ( "fmt" // other imports )
18-18: Define a constant for the default gas consumption value.Using a constant for the default gas consumption improves readability and maintainability.
Example:
const DefaultDenomCreationGasConsume uint64 = 1_000_000 func DefaultParams() Params { return NewParams(sdk.NewCoins(), DefaultDenomCreationGasConsume) }x/bank/proto/cosmos/bank/v2/bank.proto (2)
16-18: Ensure consistent use of(gogoproto.nullable)optionFor the field
denom_creation_fee, you've set(gogoproto.nullable) = false, making it non-nullable. Please verify that this is intentional and consistent with how other fields are handled. Non-nullable fields generate more straightforward Go code without pointer references.
26-26: Consider setting(gogoproto.nullable)to false for scalar typesFor the field
denom_creation_gas_consume, you've set(gogoproto.nullable) = true. Sinceuint64is a scalar type, making it nullable will generate a pointer touint64in the Go code, which may not be necessary. For consistency and to simplify the generated code, consider setting(gogoproto.nullable) = false.x/bank/v2/keeper/createdenom.go (1)
59-60: Enhance TODO comment with actionable detailsThe TODO comment lacks a ticket reference or a clear description of the task. As per the Uber Go Style Guide, TODO comments should include a reference or a concise explanation of the work to be done.
Consider updating the TODO comment:
-// TODO: do we need map creator => denom +// TODO(#IssueNumber): Assess the necessity of mapping from creator to denom.Replace
#IssueNumberwith the relevant issue tracker number for better traceability.x/bank/proto/cosmos/bank/v2/tx.proto (2)
52-53: Add field comments for 'sender' and 'subdenom'.For clarity and better documentation, please add comments to the
senderandsubdenomfields explaining their purpose and any expected formats or constraints.
66-68: Add field comments for 'sender', 'denom', and 'new_admin'.Including descriptive comments for the
sender,denom, andnew_adminfields will enhance readability and assist developers in understanding the role and expected values of each field.x/bank/v2/keeper/handlers.go (2)
55-66: Add function documentation forMsgCreateDenomAccording to the Uber Go Style Guide, all exported functions should have a preceding comment that explains their purpose and usage. Please add a documentation comment for the
MsgCreateDenommethod to enhance code readability and maintainability.Example:
// MsgCreateDenom handles the creation of a new denomination.
68-68: Clarify the TODO comment regarding governanceThe TODO comment
// TODO: should be gov?suggests uncertainty about whether theMsgChangeAdminhandler should be managed through governance. Consider clarifying this decision to ensure the code aligns with the intended design. If this action should be moved to the governance module, I can assist with the refactoring process.Would you like help in integrating this functionality with the governance module or opening a GitHub issue to track this task?
x/bank/v2/keeper/keeper.go (1)
184-185: Update function comment forGetDenomMetaDatato follow Go conventionsThe comment for
GetDenomMetaDatashould start with the function name and be properly capitalized and punctuated to conform with Go standards.Consider updating the comment to:
// GetDenomMetaData retrieves the denomination metadata. It returns the metadata and true if the denom exists, false otherwise.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
x/bank/v2/types/authorityMetadata.pb.gois excluded by!**/*.pb.gox/bank/v2/types/bank.pb.gois excluded by!**/*.pb.gox/bank/v2/types/genesis.pb.gois excluded by!**/*.pb.gox/bank/v2/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
- x/bank/proto/cosmos/bank/v2/authorityMetadata.proto (1 hunks)
- x/bank/proto/cosmos/bank/v2/bank.proto (1 hunks)
- x/bank/proto/cosmos/bank/v2/genesis.proto (3 hunks)
- x/bank/proto/cosmos/bank/v2/tx.proto (1 hunks)
- x/bank/v2/keeper/admins.go (1 hunks)
- x/bank/v2/keeper/createdenom.go (1 hunks)
- x/bank/v2/keeper/creators.go (1 hunks)
- x/bank/v2/keeper/genesis.go (1 hunks)
- x/bank/v2/keeper/handlers.go (1 hunks)
- x/bank/v2/keeper/keeper.go (4 hunks)
- x/bank/v2/keeper/keeper_test.go (1 hunks)
- x/bank/v2/keeper/params.go (1 hunks)
- x/bank/v2/module.go (1 hunks)
- x/bank/v2/types/authorityMetadata.go (1 hunks)
- x/bank/v2/types/denoms.go (1 hunks)
- x/bank/v2/types/errors.go (1 hunks)
- x/bank/v2/types/keys.go (1 hunks)
- x/bank/v2/types/params.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/bank/v2/keeper/creators.go
🧰 Additional context used
📓 Path-based instructions (13)
x/bank/v2/keeper/admins.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/createdenom.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/handlers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/bank/v2/keeper/params.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/authorityMetadata.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/denoms.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/errors.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/keys.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/params.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 buf
x/bank/proto/cosmos/bank/v2/authorityMetadata.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
x/bank/proto/cosmos/bank/v2/bank.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (10)
x/bank/v2/types/authorityMetadata.go (2)
1-5: LGTM: Package declaration and imports are correct.The package is correctly declared as
types, which is appropriate for a types file in the bank module. The import of thesdkpackage is properly aliased and appears to be the only necessary import for this file.
1-15: Excellent implementation of theValidate()method.The overall implementation of this file is clean, concise, and follows Go best practices. It adheres to the Single Responsibility Principle by focusing solely on validating the
DenomAuthorityMetadata. The error handling is appropriate, and the code is easy to read and understand.x/bank/v2/keeper/params.go (1)
3-7: LGTM: Import statements are correct and well-organized.The import statements are properly structured and include only the necessary packages.
x/bank/proto/cosmos/bank/v2/authorityMetadata.proto (2)
2-2: LGTM: Package declaration and go_package option are correct.The package declaration
cosmos.bank.v2and thego_packageoption"cosmossdk.io/x/bank/v2/types"are correctly specified and follow the expected format for Cosmos SDK modules.Also applies to: 7-7
1-17: Overall: Well-structured and focused Protocol Buffers file.This new file,
authorityMetadata.proto, is well-structured and focused on its purpose of defining theDenomAuthorityMetadatamessage. It follows Protocol Buffers best practices and Cosmos SDK conventions. The message is clearly commented, and the use ofgogoprotooptions enhances its functionality.Key points:
- Correct syntax and package declarations.
- Appropriate imports (pending verification of
gogoproto).- Well-defined message with clear comments and correct use of options.
- Consideration for future extensibility.
Great job on maintaining clarity and following best practices in this new addition to the
cosmos.bank.v2package.🧰 Tools
🪛 buf
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
x/bank/v2/types/errors.go (1)
1-19: Overall, well-structured and comprehensive error definitionsThis file provides a clear and well-organized set of error definitions for the
x/tokenfactorymodule. The errors cover various scenarios and are registered with unique codes, which is excellent for error handling and debugging.The error messages are descriptive and will be helpful for developers using this module. Good job on maintaining consistency and following best practices in error definition.
x/bank/proto/cosmos/bank/v2/genesis.proto (1)
6-6: LGTM: New import statement is correct and necessary.The addition of the import statement for "cosmos/bank/v2/authorityMetadata.proto" is appropriate and necessary to support the new GenesisDenom message which uses DenomAuthorityMetadata.
x/bank/v2/keeper/admins.go (1)
10-13: FunctionGetAuthorityMetadatais correctly implementedThe function effectively retrieves the authority metadata for a given denom and handles errors appropriately.
x/bank/v2/keeper/genesis.go (1)
47-52: SetDenomMetaData error handling checkThe loop correctly iterates over
state.DenomMetadataand attempts to set the denomination metadata usingk.SetDenomMetaData(ctx, meta). However, ensure thatSetDenomMetaDataproperly validates the metadata before setting it to prevent any invalid entries in the state.x/bank/proto/cosmos/bank/v2/bank.proto (1)
47-65: Review thecosmos_proto.field_added_inannotationsThe fields
name,symbol,uri, anduri_hashin theMetadatamessage have annotations indicating they were added in earlier versions ("cosmos-sdk 0.43" and "cosmos-sdk 0.46"). Since these fields are being introduced now, please verify whether these annotations accurately reflect the version in which they are added. If these fields are new in this version, update the annotations accordingly.
| syntax = "proto3"; | ||
| package cosmos.bank.v2; | ||
|
|
||
| import "gogoproto/gogo.proto"; | ||
| import "cosmos/base/v1beta1/coin.proto"; |
There was a problem hiding this comment.
💡 Codebase verification
Import gogoproto/gogo.proto not found in the repository.
The import gogoproto/gogo.proto is specified in authorityMetadata.proto, but the file does not exist in the repository. Please ensure that the gogoproto dependency is correctly included and that the gogo.proto file is present in the expected location. Additionally, verify that the protobuf generator protoc-gen-gogo is installed in your development environment to support the compilation process.
🔗 Analysis chain
Verify the gogoproto import.
The import of gogoproto/gogo.proto is common in Cosmos SDK projects, but our static analysis tool reports that this file does not exist. Please ensure that the gogoproto package is correctly set up in your development environment and build process.
Run the following script to check for the presence of gogoproto in the project:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the presence of gogoproto in the project
# Test 1: Search for gogoproto in go.mod
echo "Searching for gogoproto in go.mod:"
rg 'github.com/gogo/protobuf' go.mod
# Test 2: Search for other imports of gogoproto
echo "Searching for other imports of gogoproto:"
rg --type proto 'import "gogoproto/gogo.proto"'
# Test 3: Check if protoc-gen-gogo is installed
echo "Checking if protoc-gen-gogo is installed:"
which protoc-gen-gogo || echo "protoc-gen-gogo not found in PATH"
Length of output: 601
Script:
#!/bin/bash
# Description: Verify the existence of gogoproto/gogo.proto in the repository
echo "Searching for gogoproto/gogo.proto in the repository:"
fd 'gogoproto/gogo.proto' . --case-sensitive
Length of output: 161
🧰 Tools
🪛 buf
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
| _, ok := i.(uint64) | ||
| if !ok { | ||
| return fmt.Errorf("invalid parameter type: %T", i) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate that DenomCreationGasConsume is greater than zero.
Currently, validateDenomCreationGasConsume only checks the type of the parameter. To ensure meaningful values, consider validating that DenomCreationGasConsume is greater than zero.
You can add a check like:
value, _ := i.(uint64)
if value == 0 {
return fmt.Errorf("denom creation gas consume must be greater than zero")
}
There was a problem hiding this comment.
Actionable comments posted: 29
🧹 Outside diff range and nitpick comments (21)
x/bank/v2/types/authorityMetadata.go (1)
7-15: LGTM: TheValidate()method is well-implemented.The method correctly validates the
Adminfield ofDenomAuthorityMetadatawhen it's not empty, using the SDK'sAccAddressFromBech32function. Error handling is appropriate, and the method returnsnilwhen there are no validation errors.Consider adding a comment to explain the purpose of this validation method, especially for other developers who might use this type. For example:
// Validate checks if the Admin field, if set, is a valid Bech32 address. // It returns an error if the Admin is set but invalid, otherwise nil. func (metadata DenomAuthorityMetadata) Validate() error { // ... (existing implementation) }x/bank/v2/keeper/params.go (2)
9-12: Approve implementation, but fix comment.The
SetParamsmethod is correctly implemented. However, the comment above the function doesn't match its functionality.Please update the comment to accurately describe the
SetParamsfunction:-// GetAuthorityMetadata returns the authority metadata for a specific denom +// SetParams sets the parameters for the bank module func (k Keeper) SetParams(ctx context.Context, params types.Params) error {
14-21: Approve implementation, but fix comment.The
GetParamsmethod is correctly implemented with proper error handling. However, the comment above the function doesn't match its functionality.Please update the comment to accurately describe the
GetParamsfunction:-// setAuthorityMetadata stores authority metadata for a specific denom +// GetParams retrieves the parameters for the bank module func (k Keeper) GetParams(ctx context.Context) types.Params {x/bank/proto/cosmos/bank/v2/authorityMetadata.proto (1)
5-5: Remove unused importThe import
cosmos/base/v1beta1/coin.protois not used in this file. Consider removing it to keep the imports clean and avoid potential confusion.x/bank/v2/types/errors.go (2)
1-9: LGTM! Consider clarifying the DONTCOVER comment.The package declaration and imports look good. The
errorsmodimport with a custom path is correctly used.Consider adding a brief explanation for the
// DONTCOVERcomment, e.g.:// DONTCOVER - This file is excluded from test coverage reportsThis will help other developers understand why this file is excluded from coverage.
11-19: LGTM! Consider updating the module name in the comment.The error variable definitions look good. Each error has a unique code and a descriptive message, which is excellent for error handling and debugging.
The comment on line 11 mentions the "x/tokenfactory" module, but this file is in the "bank" module. Consider updating the comment to:
// x/bank module sentinel errorsThis will ensure consistency with the actual module name.
x/bank/v2/types/keys.go (1)
35-37: Consider adding comments for the new prefix variables.The new variables
DenomMetadataPrefixandDenomAuthorityPrefixare consistent with the existing naming conventions and initialization patterns. However, to improve code readability and maintainability, consider adding brief comments explaining the purpose of these prefixes, similar to the comments for other prefix variables in this file.Here's a suggested addition:
// DenomMetadataPrefix is the prefix for storing denomination metadata DenomMetadataPrefix = collections.NewPrefix(6) // DenomAuthorityPrefix is the prefix for storing denomination authority information DenomAuthorityPrefix = collections.NewPrefix(7)x/bank/v2/module.go (1)
101-102: LGTM! Consider grouping related handlers.The addition of
MsgCreateDenomandMsgChangeAdminhandlers is consistent with the existing pattern and adheres to the Uber Golang style guide.For improved readability and organization, consider grouping related handlers together. For example:
// Token-related handlers appmodulev2.RegisterMsgHandler(router, handlers.MsgMint) appmodulev2.RegisterMsgHandler(router, handlers.MsgCreateDenom) appmodulev2.RegisterMsgHandler(router, handlers.MsgChangeAdmin) // Transaction-related handlers appmodulev2.RegisterMsgHandler(router, handlers.MsgSend) // Admin-related handlers appmodulev2.RegisterMsgHandler(router, handlers.MsgUpdateParams)This grouping can make it easier to understand the different categories of handlers at a glance.
x/bank/v2/keeper/admins.go (2)
25-34: Add a comment to unexported function for clarityAlthough
setAdminis an unexported function, adding a brief comment explaining its purpose enhances code readability and maintenance.Consider adding a comment like:
// setAdmin updates the admin field in the authority metadata for a specific denom.
3-7: Group import statements according to Go conventionsThe import statements should be grouped with standard library packages separated from external packages by a blank line. According to the Uber Go Style Guide, this improves readability.
Apply this diff to group imports properly:
import ( "context" + "cosmossdk.io/x/bank/v2/types" )x/bank/v2/types/params.go (1)
3-7: Organize import statements according to style guidelinesPer the Uber Go Style Guide, import statements should be grouped into standard library packages, third-party packages, and then project packages, separated by blank lines. Ensure that imports are properly organized.
Apply this change:
import ( - fmt "fmt" - sdk "github.com/cosmos/cosmos-sdk/types" + "fmt" + sdk "github.com/cosmos/cosmos-sdk/types" )x/bank/proto/cosmos/bank/v2/genesis.proto (3)
33-33: Consider adding '(amino.dont_omitempty) = true' to 'factory_denoms'In the
GenesisStatemessage, other repeated fields likebalances,supply, anddenom_metadatahave the option(amino.dont_omitempty) = true. For consistency and to prevent omitting empty fields during amino encoding, consider adding(amino.dont_omitempty) = trueto thefactory_denomsfield.
54-56: Enhance documentation for 'GenesisDenom' to improve clarityThe current comment for
GenesisDenomcontains redundancy and could be clearer. Consider rephrasing it to:// GenesisDenom represents a token factory denomination included in the genesis // state. It contains DenomAuthorityMetadata, which defines the denom's admin.This revision eliminates redundancy and provides a clearer explanation.
61-61: Consider adding '(amino.dont_omitempty) = true' to 'authority_metadata'The field
authority_metadatainGenesisDenomis marked with[(gogoproto.nullable) = false]. For consistency with other non-nullable fields and to ensure proper amino encoding, consider adding(amino.dont_omitempty) = trueto this field's options.x/bank/v2/types/denoms.go (1)
53-55: Standardize error message formattingConsider rephrasing the error message to align with Go error handling conventions, which recommend starting messages with a lowercase letter and avoiding punctuation at the end.
Apply this diff to adjust the error message:
- return "", "", errors.Wrapf(ErrInvalidDenom, "denom prefix is incorrect. Is: %s. Should be: %s", strParts[0], ModuleDenomPrefix) + return "", "", errors.Wrapf(ErrInvalidDenom, "denom prefix is incorrect: expected %s, got %s", ModuleDenomPrefix, strParts[0])x/bank/proto/cosmos/bank/v2/bank.proto (1)
47-65: Ensure 'cosmos_proto.field_added_in' annotations are up-to-dateThe
Metadatamessage includes fields with the(cosmos_proto.field_added_in)annotation specifying versions like "cosmos-sdk 0.43" and "cosmos-sdk 0.46". Since this file is forcosmos.bank.v2, please confirm that these annotations accurately reflect when these fields were added relative to this module version.x/bank/v2/keeper/createdenom.go (1)
59-60: Address the TODO comment or create a tracking issueThere's a TODO comment at line 59 regarding the mapping from creator to denom. It's recommended to resolve TODOs or create GitHub issues to track them before merging.
Would you like me to open a GitHub issue to track this task?
x/bank/v2/keeper/handlers.go (2)
68-68: Consider handling admin changes via governanceThe comment
// TODO: should be gov?suggests that changing the admin of a denomination might be better managed through the governance module. Since changing the admin can have significant implications, routing this action through governance could ensure broader consensus and security.Would you like assistance in proposing a governance-based implementation for admin changes?
55-66: Wrap errors with contextual informationWhen returning errors, it's helpful to wrap them with additional context to aid in debugging and error tracing. This practice aligns with the Uber Go Style Guide's recommendations for error handling.
For example, in line 60~:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create denom: %w", err) }This approach provides clarity on where and why the error occurred.
x/bank/v2/keeper/keeper.go (2)
31-32: Maintain consistent ordering of struct fieldsIn the
Keeperstruct, consider grouping related fields together for better readability. SincedenomMetadataanddenomAuthorityare related to denominations, they can be placed adjacent to other denomination-related fields.Adjust the struct field ordering:
type Keeper struct { appmodulev2.Environment authority []byte addressCodec address.Codec schema collections.Schema params collections.Item[types.Params] + denomMetadata collections.Map[string, types.Metadata] + denomAuthority collections.Map[string, types.DenomAuthorityMetadata] balances *collections.IndexedMap[collections.Pair[[]byte, string], math.Int, BalancesIndexes] supply collections.Map[string, math.Int] sendRestriction *sendRestriction }
187-188: Use consistent naming conventionsThe term "metadata" is used inconsistently in method names (
GetDenomMetaDatavs.denomMetadata). For clarity and consistency, consider using eitherMetaDataorMetadatathroughout.Rename the method to match the field name:
-func (k Keeper) GetDenomMetaData(ctx context.Context, denom string) (types.Metadata, error) { +func (k Keeper) GetDenomMetadata(ctx context.Context, denom string) (types.Metadata, error) {Apply similar changes to other methods and variables where applicable.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
x/bank/v2/types/authorityMetadata.pb.gois excluded by!**/*.pb.gox/bank/v2/types/bank.pb.gois excluded by!**/*.pb.gox/bank/v2/types/genesis.pb.gois excluded by!**/*.pb.gox/bank/v2/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
- x/bank/proto/cosmos/bank/v2/authorityMetadata.proto (1 hunks)
- x/bank/proto/cosmos/bank/v2/bank.proto (1 hunks)
- x/bank/proto/cosmos/bank/v2/genesis.proto (3 hunks)
- x/bank/proto/cosmos/bank/v2/tx.proto (1 hunks)
- x/bank/v2/keeper/admins.go (1 hunks)
- x/bank/v2/keeper/createdenom.go (1 hunks)
- x/bank/v2/keeper/creators.go (1 hunks)
- x/bank/v2/keeper/genesis.go (1 hunks)
- x/bank/v2/keeper/handlers.go (1 hunks)
- x/bank/v2/keeper/keeper.go (4 hunks)
- x/bank/v2/keeper/keeper_test.go (1 hunks)
- x/bank/v2/keeper/params.go (1 hunks)
- x/bank/v2/module.go (1 hunks)
- x/bank/v2/types/authorityMetadata.go (1 hunks)
- x/bank/v2/types/denoms.go (1 hunks)
- x/bank/v2/types/errors.go (1 hunks)
- x/bank/v2/types/keys.go (1 hunks)
- x/bank/v2/types/params.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/bank/v2/keeper/creators.go
🧰 Additional context used
📓 Path-based instructions (13)
x/bank/v2/keeper/admins.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/createdenom.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/handlers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/bank/v2/keeper/params.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/authorityMetadata.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/denoms.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/errors.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/keys.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/params.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 buf
x/bank/proto/cosmos/bank/v2/authorityMetadata.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
x/bank/proto/cosmos/bank/v2/bank.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (18)
x/bank/v2/types/authorityMetadata.go (2)
1-5: LGTM: Package declaration and imports are correct.The package is correctly declared as
types, and the necessarysdkpackage is properly imported from cosmos-sdk.
1-15: Overall, excellent implementation ofDenomAuthorityMetadatavalidation.This file is well-structured, concise, and follows Go best practices. It adheres to the Single Responsibility Principle by focusing solely on validating the
DenomAuthorityMetadata. The code is clean, readable, and efficiently implements the necessary validation logic.x/bank/v2/keeper/params.go (2)
1-7: LGTM: Package declaration and imports are correct.The package declaration and imports are appropriate for this keeper file. The necessary packages are imported, and the import block is correctly formatted.
1-21: LGTM: Overall structure and style are good.The file structure, formatting, and style adhere to the Uber Golang style guide. The code is clean, well-organized, and easy to read.
x/bank/proto/cosmos/bank/v2/authorityMetadata.proto (1)
9-14: LGTM: Clear message definition with appropriate optionThe
DenomAuthorityMetadatamessage is well-defined with a clear and informative comment. The use of thegogoproto.equaloption is appropriate for enabling equality checks on this message type.x/bank/v2/keeper/admins.go (1)
1-34: Verify the accessibility of unexported methodsThe methods
setAuthorityMetadataandsetAdminare unexported. If these methods need to be accessed from other packages, they should be exported. Confirm whether they are intended to be unexported.Please ensure that these methods are correctly scoped. If they are only used within the
keeperpackage, then unexported methods are appropriate.x/bank/v2/keeper/genesis.go (2)
55-59: Validate the creator's address format when deconstructing denomWhen deconstructing the denom using
types.DeconstructDenom, ensure that thecreatoraddress is valid and conforms to the expected format. An invalid address could cause issues downstream.Add a validation step to confirm the
creatoraddress:creator, _, err := types.DeconstructDenom(genDenom.GetDenom()) if err != nil { return fmt.Errorf("failed to deconstruct denom %s: %w", genDenom.GetDenom(), err) } +if _, err := sdk.AccAddressFromBech32(creator); err != nil { + return fmt.Errorf("invalid creator address %s for denom %s: %w", creator, genDenom.GetDenom(), err) +}
54-67:⚠️ Potential issueEnsure validation and uniqueness of factory denominations
While processing factory denominations, there is no explicit check for duplicate denominations or invalid entries. This could lead to issues during runtime if duplicates exist or if denominations are malformed.
Consider adding validation to ensure all factory denominations are unique and valid. You might also want to handle errors with more context.
for _, genDenom := range state.GetFactoryDenoms() { creator, _, err := types.DeconstructDenom(genDenom.GetDenom()) if err != nil { - return err + return fmt.Errorf("failed to deconstruct denom %s: %w", genDenom.GetDenom(), err) } err = k.createDenomAfterValidation(ctx, creator, genDenom.GetDenom()) if err != nil { - return err + return fmt.Errorf("failed to create denom %s: %w", genDenom.GetDenom(), err) } err = k.setAuthorityMetadata(ctx, genDenom.GetDenom(), genDenom.GetAuthorityMetadata()) if err != nil { - return err + return fmt.Errorf("failed to set authority metadata for denom %s: %w", genDenom.GetDenom(), err) } }Run the following script to check for duplicate factory denominations:
This script assumes that your
genesis.jsonhas anapp_state.bank.factory_denomsfield containing the factory denominations.x/bank/proto/cosmos/bank/v2/genesis.proto (2)
58-58: Verify the necessity of '(gogoproto.equal) = true' in 'GenesisDenom'In the
Balancemessage,(gogoproto.equal) = falseis set, and(gogoproto.goproto_getters) = falseis included to control code generation. Ensure that setting(gogoproto.equal) = trueinGenesisDenomis intentional and that equality methods are required for this message. If not necessary, consider aligning with theBalancemessage settings for consistency.
6-6: Import statement for 'authorityMetadata.proto' seems appropriateThe addition of the import statement for
"cosmos/bank/v2/authorityMetadata.proto"correctly brings in dependencies required forDenomAuthorityMetadata. This import is necessary for the newly addedauthority_metadatafield inGenesisDenom.x/bank/v2/types/denoms.go (1)
25-37:GetTokenDenomfunction looks goodThe function correctly constructs the denom and validates it properly.
x/bank/proto/cosmos/bank/v2/bank.proto (1)
25-26: Verify the necessity of 'nullable' option on scalar fieldThe
denom_creation_gas_consumefield is of typeuint64, which is a scalar. The(gogoproto.nullable) = trueoption may not have the intended effect on scalar types in Protobuf 3, as scalar fields are non-nullable by default. Please review whether this option is needed.Run the following script to search for instances where
nullableis used with scalar fields:✅ Verification successful
Nullable option unnecessary on scalar field
The
(gogoproto.nullable) = trueoption on theuint64scalar fielddenom_creation_gas_consumeinx/bank/proto/cosmos/bank/v2/bank.proto(lines 25-26) is unnecessary, as scalar fields are non-nullable by default in Protobuf 3.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of 'nullable' option with scalar types in .proto files. # Expected: Such usages should be rare; this helps verify if it's a common practice in the codebase. grep -nr "(gogoproto.nullable)" x/Length of output: 42691
x/bank/v2/keeper/createdenom.go (2)
85-86: Check for nil pointers inparams.DenomCreationFeeAt line 89, you check if
params.DenomCreationFee != nilto determine if there is a fee to charge. Ensure thatDenomCreationFeeis a pointer type and correctly initialized to prevent nil pointer dereferences.Run the following script to inspect the definition of
DenomCreationFee:#!/bin/bash # Description: Find the definition of DenomCreationFee in Params. rg --type go 'DenomCreationFee\s+\*sdk\.Coins' -A 5
104-108: EnsureGasMeteris properly initialized before consumptionIn the
chargeForCreateDenomfunction, you consume gas usingk.Environment.GasService.GasMeter(ctx). Please ensure thatGasServiceandGasMeterare properly initialized in the context to avoid runtime panics.Run the following script to verify the initialization of
GasService:x/bank/proto/cosmos/bank/v2/tx.proto (1)
71-72: Definition ofMsgChangeAdminResponseis appropriateThe
MsgChangeAdminResponsemessage is correctly defined and follows the pattern established in the module.x/bank/v2/keeper/keeper.go (1)
47-48:⚠️ Potential issueCheck for errors during schema building
In the
NewKeeperfunction, after building the schema, any errors are checked and cause a panic. Consider handling the error instead of panicking to maintain application stability.However, if panicking is the accepted practice in this initialization context within your codebase, this can be acceptable. Please ensure it aligns with your project's error-handling strategy.
If not already established, confirm whether panicking is acceptable here.
x/bank/v2/keeper/keeper_test.go (2)
240-350: Comprehensive test cases inTestCreateDenomThe added test cases thoroughly cover various scenarios for denom creation, including edge cases and error conditions, which enhances the robustness of the denom creation logic and ensures proper validation.
352-415: Accurate gas consumption testing inTestCreateDenom_GasConsumeThe test effectively verifies that the gas consumed during denom creation falls within expected ranges under different gas parameters. This is crucial for performance optimization and ensures that gas consumption adheres to expected norms.
| // Can be empty for no admin, or a valid osmosis address | ||
| string admin = 1 [(gogoproto.moretags) = "yaml:\"admin\""]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a validation rule for the admin address
The admin field is well-defined and commented. However, to ensure data integrity, consider adding a validation rule to check if the admin address is either empty or a valid osmosis address. This can be done using the validate.rules option from the protoc-gen-validate plugin.
Example:
string admin = 1 [
(gogoproto.moretags) = "yaml:\"admin\"",
(validate.rules).string = {
pattern: "^$|^osmo[a-zA-Z0-9]{39}$"
}
];This regex pattern allows either an empty string or a valid osmosis address.
Would you like me to provide more information on implementing this validation?
| syntax = "proto3"; | ||
| package cosmos.bank.v2; | ||
|
|
||
| import "gogoproto/gogo.proto"; |
There was a problem hiding this comment.
💡 Codebase verification
Missing gogoproto Dependency
The gogoproto/gogo.proto import is present in multiple .proto files, but the gogoproto directory and gogo.proto file are not found in the repository. This indicates that the necessary gogoproto dependency is missing and needs to be properly installed or included in the project.
- Ensure that
gogoprotois added to your project's dependency management system. - Verify that all dependencies are correctly installed and included in the repository.
🔗 Analysis chain
Verify the existence of the gogoproto import
The static analysis tool indicates that the file gogoproto/gogo.proto does not exist. Please ensure that this import is correctly set up in your project. If it's a third-party dependency, make sure it's properly installed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the gogoproto import
# Test: Check if the gogoproto directory exists
if [ -d "gogoproto" ]; then
echo "gogoproto directory exists"
# Check if the gogo.proto file exists
if [ -f "gogoproto/gogo.proto" ]; then
echo "gogo.proto file exists"
else
echo "gogo.proto file not found in gogoproto directory"
fi
else
echo "gogoproto directory not found"
fi
# Test: Search for other occurrences of gogoproto import
echo "Other files importing gogoproto:"
rg --type proto 'import "gogoproto/gogo.proto"'
Length of output: 246
Script:
#!/bin/bash
# Description: Re-verify the existence of the gogoproto import with corrected search
# Search for the gogo.proto file anywhere in the repository
echo "Searching for gogo.proto file in the repository..."
fd "gogo.proto" --type f
# Check if any gogo.proto files were found
if fd "gogo.proto" --type f | grep -q "gogo.proto"; then
echo "gogo.proto file found."
else
echo "gogo.proto file not found in the repository."
fi
# Search for import statements importing gogo.proto
echo "Searching for import statements with gogoproto/gogo.proto..."
rg 'import "gogoproto/gogo.proto"' --glob "*.proto" --no-filename
Length of output: 3176
🧰 Tools
🪛 buf
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
| func (k Keeper) GetAuthorityMetadata(ctx context.Context, denom string) (types.DenomAuthorityMetadata, error) { | ||
| authority, err := k.denomAuthority.Get(ctx, denom) | ||
| return authority, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent variable naming for clarity
In the GetAuthorityMetadata function, the variable authority is assigned the DenomAuthorityMetadata, but in other functions like setAdmin, the same type is referred to as metadata. For consistency and readability, consider renaming authority to metadata.
Apply this diff to rename the variable:
func (k Keeper) GetAuthorityMetadata(ctx context.Context, denom string) (types.DenomAuthorityMetadata, error) {
- authority, err := k.denomAuthority.Get(ctx, denom)
- return authority, err
+ metadata, err := k.denomAuthority.Get(ctx, denom)
+ return metadata, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (k Keeper) GetAuthorityMetadata(ctx context.Context, denom string) (types.DenomAuthorityMetadata, error) { | |
| authority, err := k.denomAuthority.Get(ctx, denom) | |
| return authority, err | |
| } | |
| func (k Keeper) GetAuthorityMetadata(ctx context.Context, denom string) (types.DenomAuthorityMetadata, error) { | |
| metadata, err := k.denomAuthority.Get(ctx, denom) | |
| return metadata, err | |
| } |
| // setAuthorityMetadata stores authority metadata for a specific denom | ||
| func (k Keeper) setAuthorityMetadata(ctx context.Context, denom string, metadata types.DenomAuthorityMetadata) error { | ||
| err := metadata.Validate() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return k.denomAuthority.Set(ctx, denom, metadata) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling by wrapping validation errors
In the setAuthorityMetadata function, when metadata.Validate() returns an error, it is returned directly. To provide more context and improve debugging, consider wrapping the error with additional information.
Apply this diff to wrap the error:
func (k Keeper) setAuthorityMetadata(ctx context.Context, denom string, metadata types.DenomAuthorityMetadata) error {
err := metadata.Validate()
if err != nil {
- return err
+ return fmt.Errorf("failed to validate metadata for denom %s: %w", denom, err)
}
return k.denomAuthority.Set(ctx, denom, metadata)
}Ensure you have imported the fmt package at the top of the file:
import (
"context"
+ "fmt"
"cosmossdk.io/x/bank/v2/types"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // setAuthorityMetadata stores authority metadata for a specific denom | |
| func (k Keeper) setAuthorityMetadata(ctx context.Context, denom string, metadata types.DenomAuthorityMetadata) error { | |
| err := metadata.Validate() | |
| if err != nil { | |
| return err | |
| } | |
| return k.denomAuthority.Set(ctx, denom, metadata) | |
| } | |
| // setAuthorityMetadata stores authority metadata for a specific denom | |
| func (k Keeper) setAuthorityMetadata(ctx context.Context, denom string, metadata types.DenomAuthorityMetadata) error { | |
| err := metadata.Validate() | |
| if err != nil { | |
| return fmt.Errorf("failed to validate metadata for denom %s: %w", denom, err) | |
| } | |
| return k.denomAuthority.Set(ctx, denom, metadata) | |
| } |
x/bank/v2/types/params.go
Outdated
| } | ||
|
|
||
| // DefaultParams is the default parameter configuration for the bank/v2 module | ||
| func DefaultParams() Params { | ||
| return NewParams() | ||
| return NewParams(sdk.NewCoins(), 1_000_000) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define default gas consumption as a constant for clarity
In the DefaultParams function, the hard-coded value 1_000_000 for DenomCreationGasConsume can be extracted into a named constant. This enhances readability and makes it easier to manage default values.
Apply this change:
+const DefaultDenomCreationGasConsume uint64 = 1_000_000
func DefaultParams() Params {
- return NewParams(sdk.NewCoins(), 1_000_000)
+ return NewParams(sdk.NewCoins(), DefaultDenomCreationGasConsume)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return NewParams(sdk.NewCoins(), 1_000_000) | |
| const DefaultDenomCreationGasConsume uint64 = 1_000_000 | |
| func DefaultParams() Params { | |
| return NewParams(sdk.NewCoins(), DefaultDenomCreationGasConsume) | |
| } |
| denomMetaData = append(denomMetaData, metadata) | ||
| return false | ||
| }) | ||
|
|
||
| return denomMetaData |
There was a problem hiding this comment.
Handle errors returned by IterateAllDenomMetaData in GetAllDenomMetaData
With IterateAllDenomMetaData now returning an error, GetAllDenomMetaData should handle this error to ensure proper error propagation.
Update GetAllDenomMetaData as follows:
-func (k Keeper) GetAllDenomMetaData(ctx context.Context) []types.Metadata {
+func (k Keeper) GetAllDenomMetaData(ctx context.Context) ([]types.Metadata, error) {
denomMetaData := make([]types.Metadata, 0)
- k.IterateAllDenomMetaData(ctx, func(metadata types.Metadata) bool {
+ err := k.IterateAllDenomMetaData(ctx, func(metadata types.Metadata) bool {
denomMetaData = append(denomMetaData, metadata)
return false
})
+ if err != nil {
+ // Handle or log the error as needed
+ return nil, err
+ }
return denomMetaData
}Ensure that any callers of GetAllDenomMetaData are updated to handle the returned error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| denomMetaData = append(denomMetaData, metadata) | |
| return false | |
| }) | |
| return denomMetaData | |
| func (k Keeper) GetAllDenomMetaData(ctx context.Context) ([]types.Metadata, error) { | |
| denomMetaData := make([]types.Metadata, 0) | |
| err := k.IterateAllDenomMetaData(ctx, func(metadata types.Metadata) bool { | |
| denomMetaData = append(denomMetaData, metadata) | |
| return false | |
| }) | |
| if err != nil { | |
| // Handle or log the error as needed | |
| return nil, err | |
| } | |
| return denomMetaData, nil | |
| } |
| panic(err) | ||
| } |
There was a problem hiding this comment.
Avoid using panic in IterateAccountBalances
In the IterateAccountBalances method, calling panic when an error occurs is discouraged. Using panic can cause the entire application to crash, which is undesirable in production code.
Modify the method to return the error instead:
-func (k Keeper) IterateAccountBalances(ctx context.Context, addr sdk.AccAddress, cb func(sdk.Coin) bool) {
+func (k Keeper) IterateAccountBalances(ctx context.Context, addr sdk.AccAddress, cb func(sdk.Coin) bool) error {
err := k.balances.Walk(ctx, collections.NewPrefixedPairRange[[]byte, string](addr), func(key collections.Pair[[]byte, string], value math.Int) (stop bool, err error) {
return cb(sdk.NewCoin(key.K2(), value)), nil
})
if err != nil {
- panic(err)
+ return err
}
+ return nil
}Remember to update any callers of this method to handle the returned error appropriately.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| panic(err) | |
| } | |
| func (k Keeper) IterateAccountBalances(ctx context.Context, addr sdk.AccAddress, cb func(sdk.Coin) bool) error { | |
| err := k.balances.Walk(ctx, collections.NewPrefixedPairRange[[]byte, string](addr), func(key collections.Pair[[]byte, string], value math.Int) (stop bool, err error) { | |
| return cb(sdk.NewCoin(key.K2(), value)), nil | |
| }) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
| m, err := k.denomMetadata.Get(ctx, denom) | ||
| return m, err == nil | ||
| } |
There was a problem hiding this comment.
Return errors explicitly in GetDenomMetaData
In GetDenomMetaData, the error from k.denomMetadata.Get(ctx, denom) is converted to a boolean. For clearer error handling, consider returning the error directly.
Modify the method:
-func (k Keeper) GetDenomMetaData(ctx context.Context, denom string) (types.Metadata, bool) {
+func (k Keeper) GetDenomMetaData(ctx context.Context, denom string) (types.Metadata, error) {
m, err := k.denomMetadata.Get(ctx, denom)
- return m, err == nil
+ if err != nil {
+ return types.Metadata{}, err
+ }
+ return m, nil
}Update any code that calls GetDenomMetaData to handle the returned error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| m, err := k.denomMetadata.Get(ctx, denom) | |
| return m, err == nil | |
| } | |
| func (k Keeper) GetDenomMetaData(ctx context.Context, denom string) (types.Metadata, error) { | |
| m, err := k.denomMetadata.Get(ctx, denom) | |
| if err != nil { | |
| return types.Metadata{}, err | |
| } | |
| return m, nil | |
| } |
| balances := sdk.NewCoins() | ||
| k.IterateAccountBalances(ctx, addr, func(balance sdk.Coin) bool { | ||
| balances = balances.Add(balance) | ||
| return false | ||
| }) | ||
|
|
||
| return balances.Sort() | ||
| } |
There was a problem hiding this comment.
Handle errors returned by IterateAccountBalances in GetAllBalances
After modifying IterateAccountBalances to return an error, the GetAllBalances method should handle this error to maintain robustness.
Update GetAllBalances to handle the error:
func (k Keeper) GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins {
balances := sdk.NewCoins()
- k.IterateAccountBalances(ctx, addr, func(balance sdk.Coin) bool {
+ err := k.IterateAccountBalances(ctx, addr, func(balance sdk.Coin) bool {
balances = balances.Add(balance)
return false
})
+ if err != nil {
+ // Handle or log the error as needed
+ return balances
+ }
return balances.Sort()
}Committable suggestion was skipped due to low confidence.
| // the range [gasConsume, gasConsume + offset]. If the actual gas consumption | ||
| // falls within the range for all test cases, we consider the test passed. | ||
| // | ||
| // In experience, the total amount of gas consumed should consume be ~30k more |
There was a problem hiding this comment.
Typo in comment: Remove duplicate word 'consume'
In the comment on line 361, the word "consume" is duplicated.
Apply this diff to correct the typo:
-// In experience, the total amount of gas consumed should consume be ~30k more
+// In experience, the total amount of gas consumed should be ~30k more📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // In experience, the total amount of gas consumed should consume be ~30k more | |
| // In experience, the total amount of gas consumed should be ~30k more |
julienrbrt
changed the title
|
I'm out for a week, let's not block this, and get some reviews. @facundomedica or @testinginprod, could you give a look? |
|
I will add tokenfatory |
I'm not sure we want force transfer tbh |
yes agree |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (12)
x/bank/v2/types/errors.go (1)
5-9: Consider removing unnecessary import alias for fmt.
According to the Uber Go Style Guide, we should avoid renaming imports except to avoid naming conflicts. The fmt package doesn't need an alias.
- fmt "fmt"
+ "fmt"x/bank/v2/keeper/handlers.go (5)
55-66: Add documentation for the exported method MsgCreateDenom.
The method MsgCreateDenom is exported but lacks a comment. According to the Uber Go Style Guide, all exported functions and methods should have a comment starting with the function name.
You may add a comment like:
// MsgCreateDenom handles the creation of a new denomination.68-68: Clarify or address the TODO comment regarding governance.
The TODO comment // TODO: should be gov? suggests uncertainty about whether MsgChangeAdmin should be handled via governance. Please clarify this point. If governance integration is required, consider implementing it or create an issue to track this task.
Would you like assistance in integrating the governance module or opening a GitHub issue to track this requirement?
69-87: Add documentation for the exported method MsgChangeAdmin.
The method MsgChangeAdmin is exported but lacks a comment. As per the Uber Go Style Guide, adding a comment improves code readability and maintainability.
You may add a comment like:
// MsgChangeAdmin allows changing the admin of a specific denomination.170-170: Address the TODO regarding minting to the module account.
The comment // TODO: should mint to mint module then transfer? suggests a consideration to mint tokens to the mint module before transferring them. Implementing this change could align the minting process with standard practices and improve security.
Would you like assistance in implementing this change or opening a GitHub issue to track this task?
178-230: Add documentation for the exported method MsgBurn.
The method MsgBurn is exported but lacks a comment. As recommended by the Uber Go Style Guide, documenting exported methods enhances code clarity.
You may add a comment like:
// MsgBurn handles burning tokens from a specified address.x/bank/v2/keeper/keeper_test.go (6)
Line range hint 86-86: Correct typo in function names
The function names contain a typo. "Acount" should be corrected to "Account" for clarity and consistency.
Apply these diffs:
-func (suite *KeeperTestSuite) TestSendCoins_Acount_To_Account() {
+func (suite *KeeperTestSuite) TestSendCoins_Account_To_Account() {-func (suite *KeeperTestSuite) TestSendCoins_Acount_To_Module() {
+func (suite *KeeperTestSuite) TestSendCoins_Account_To_Module() {Also applies to: 120-120
Line range hint 218-220: Error messages should start with lowercase letters
In Go, error messages should start with a lowercase letter and should not end with punctuation. This aligns with the Go error formatting guidelines.
Apply these diffs:
- return nil, fmt.Errorf("Can not send to same address")
+ return nil, fmt.Errorf("cannot send to same address")- return nil, fmt.Errorf("Allow only one denom per one send")
+ return nil, fmt.Errorf("allow only one denom per one send")Also applies to: 227-229
240-351: Consider using predefined constants for magic numbers
In the test cases for TestCreateDenom, magic numbers like 50_000_000 and 5_000_000_000 are used directly. Defining these values as constants would improve readability and maintainability.
For example:
const (
defaultCreationFeeAmount = 50_000_000
largeCreationFeeAmount = 5_000_000_000
)And then use these constants in your Params definitions.
487-500: Ensure error messages are checked in test cases
In the TestMintHandler test cases, when an error is expected (expErr: true), consider checking the specific error message to ensure the correct error is being returned.
For example:
require.Error(err)
require.Contains(err.Error(), "expected error message")603-604: Remove unnecessary blank lines at the end of functions
There are extra blank lines at the end of the TestBurnHandler, TestSendHandler_TokenfactoryDenom, and possibly other functions. Removing them can keep the code clean and consistent.
Apply these diffs:
- }
-
-}
+ }
+}Also applies to: 621-622, 685-686
241-351: Consider adding edge case test for maximum subdenom length
While there is a test case for a subdenom that is too long, consider adding a test case for a subdenom that is exactly at the maximum allowed length to ensure boundary conditions are handled correctly.
For example:
{
desc: "subdenom at maximum length",
denomCreationFee: defaultDenomCreationFee,
subdenom: strings.Repeat("a", maxSubdenomLength),
valid: true,
},📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/bank/v2/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
- x/bank/proto/cosmos/bank/v2/tx.proto (2 hunks)
- x/bank/v2/keeper/handlers.go (3 hunks)
- x/bank/v2/keeper/keeper.go (5 hunks)
- x/bank/v2/keeper/keeper_test.go (2 hunks)
- x/bank/v2/types/errors.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
x/bank/v2/keeper/handlers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/keeper/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/keeper/keeper_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/bank/v2/types/errors.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (13)
x/bank/v2/types/errors.go (1)
13-20: Verify error code sequence and constant definitions.
A few concerns about the error definitions:
- Error codes have gaps (missing 6,7). Consider using sequential error codes for better maintenance.
- The code references undefined constants (
ModuleName,MaxSubdenomLength,MaxCreatorLength).
Let's verify the constant definitions:
x/bank/proto/cosmos/bank/v2/tx.proto (3)
82-83: Verify impact of changing from repeated to single Coin
The change from repeated Coins to a single Coin in the amount field is a breaking change. Please ensure:
- This aligns with other mint message implementations
- All clients and dependent code are updated
#!/bin/bash
# Search for other mint message implementations
ast-grep --pattern 'message $$$Mint {
$$$
repeated cosmos.base.v1beta1.Coin $$$
$$$
}'47-54: Consider adding length and character set validations for subdenom
The subdenom field should have additional protobuf validations to ensure it meets the denomination requirements:
- Maximum length constraints
- Allowed character set restrictions
#!/bin/bash
# Search for existing denom validation patterns in the codebase
rg -g '*.proto' '(validate|validator).*denom'61-69: Verify consistency with admin change messages in other modules
Please ensure this message structure aligns with similar admin change messages in other Cosmos SDK modules for consistency.
✅ Verification successful
Message structure aligns with SDK patterns for admin changes
The MsgChangeAdmin message structure follows consistent patterns found in other Cosmos SDK modules, particularly the group module:
- Both use the
cosmos.AddressStringscalar for address fields - Similar field structure with current admin/sender and new admin
- Consistent empty response message pattern
The main differences are contextual and justified:
- Bank module uses "sender" vs "admin" to align with its domain
- Bank module includes "denom" field specific to its token context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar admin change messages in other modules
ast-grep --pattern 'message MsgChange$$$Admin {
$$$
}'
Length of output: 94
Script:
#!/bin/bash
# Search for admin-related messages in proto files using ripgrep
rg -U "message Msg.*Admin.*\{[\s\S]*?\}" --type proto -A 10
# Also search for specific change admin patterns
rg "MsgChangeAdmin|MsgUpdateAdmin" --type proto
Length of output: 171
Script:
#!/bin/bash
# Search for admin-related messages in proto files
rg -U "message Msg.*Admin.*\{[\s\S]*?\}" "*.proto" -A 10
# Also search for specific change admin patterns in all files
rg "MsgChangeAdmin|MsgUpdateAdmin" --type-list
rg "MsgChangeAdmin|MsgUpdateAdmin"
Length of output: 5759
Script:
#!/bin/bash
# Let's search for similar patterns in other modules to find comparable message structures
fd -e proto -x cat {} \; | rg -U "message Msg.*Admin.*\{[\s\S]*?\}" -A 10
# Also search specifically in well-known module locations
fd -e proto -g "**/gov/**" -g "**/group/**" -g "**/authz/**" -x cat {} \; | rg -U "message Msg.*Admin.*\{[\s\S]*?\}" -A 10
Length of output: 3338
x/bank/v2/keeper/handlers.go (1)
165-165: Ensure amount validation consistently uses IsPositive().
The code correctly uses msg.Amount.IsPositive() to check that the amount is strictly positive. This change from IsAllPositive() ensures that single coin amounts are validated properly.
Also applies to: 220-221
x/bank/v2/keeper/keeper.go (6)
25-32: Fields added to the Keeper struct are appropriate
The new fields added to the Keeper struct for managing denomination metadata and authority (denomMetadata and denomAuthority) are correctly defined and enhance the module's functionality.
47-48: Initialization of new maps in NewKeeper is correct
The denomMetadata and denomAuthority maps are properly initialized in the NewKeeper function, ensuring they are ready for use within the keeper.
181-185: Handle errors explicitly in HasSupply
The HasSupply function suppresses errors from k.supply.Has by returning has && err == nil, which can mask underlying issues that should be handled.
211-218: Avoid panicking in IterateAccountBalances; return errors instead
The IterateAccountBalances function panics if an error occurs during iteration. Panicking in library code is discouraged as it can cause unexpected crashes. Errors should be returned and handled appropriately.
222-225: Return errors explicitly in GetDenomMetaData
In GetDenomMetaData, the error from k.denomMetadata.Get is converted to a boolean, which may suppress important error information.
247-254: Avoid panicking in IterateAllDenomMetaData; return errors instead
The IterateAllDenomMetaData function panics on errors during iteration. It's recommended to return errors and allow the caller to handle them.
x/bank/v2/keeper/keeper_test.go (2)
353-416:
Update comment to fix grammatical error
In the comment describing the TestCreateDenom_GasConsume function, there's a duplicate word "consume" that should be removed for clarity.
Apply this diff:
-// In experience, the total amount of gas consumed should consume be ~30k more
+// In experience, the total amount of gas consumed should be ~30k moreLikely invalid or redundant comment.
625-687: Verify handling of invalid addresses in TestSendHandler_TokenfactoryDenom
The current test cases do not cover scenarios where the FromAddress or ToAddress are invalid (e.g., improperly formatted or nonexistent). Consider adding test cases to cover these situations.
Run the following script to check for improper address handling:
| ErrSubdenomTooLong = errorsmod.Register(ModuleName, 8, fmt.Sprintf("subdenom too long, max length is %d bytes", MaxSubdenomLength)) | ||
| ErrCreatorTooLong = errorsmod.Register(ModuleName, 9, fmt.Sprintf("creator too long, max length is %d bytes", MaxCreatorLength)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using constant error messages.
For error messages that use fmt.Sprintf, consider defining the formatted strings as constants to avoid runtime formatting overhead, especially since the max lengths are constants.
+const (
+ errSubdenomTooLong = "subdenom too long, max length is %d bytes"
+ errCreatorTooLong = "creator too long, max length is %d bytes"
+)
var (
// ... other errors ...
- ErrSubdenomTooLong = errorsmod.Register(ModuleName, 8, fmt.Sprintf("subdenom too long, max length is %d bytes", MaxSubdenomLength))
- ErrCreatorTooLong = errorsmod.Register(ModuleName, 9, fmt.Sprintf("creator too long, max length is %d bytes", MaxCreatorLength))
+ ErrSubdenomTooLong = errorsmod.Register(ModuleName, 8, fmt.Sprintf(errSubdenomTooLong, MaxSubdenomLength))
+ ErrCreatorTooLong = errorsmod.Register(ModuleName, 9, fmt.Sprintf(errCreatorTooLong, MaxCreatorLength))
// ... other errors ...
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ErrSubdenomTooLong = errorsmod.Register(ModuleName, 8, fmt.Sprintf("subdenom too long, max length is %d bytes", MaxSubdenomLength)) | |
| ErrCreatorTooLong = errorsmod.Register(ModuleName, 9, fmt.Sprintf("creator too long, max length is %d bytes", MaxCreatorLength)) | |
| const ( | |
| errSubdenomTooLong = "subdenom too long, max length is %d bytes" | |
| errCreatorTooLong = "creator too long, max length is %d bytes" | |
| ) | |
| var ( | |
| ErrSubdenomTooLong = errorsmod.Register(ModuleName, 8, fmt.Sprintf(errSubdenomTooLong, MaxSubdenomLength)) | |
| ErrCreatorTooLong = errorsmod.Register(ModuleName, 9, fmt.Sprintf(errCreatorTooLong, MaxCreatorLength)) | |
| ) |
| if msg.Authority != authorityMetadata.GetAdmin() { | ||
| return nil, types.ErrUnauthorized | ||
| } |
There was a problem hiding this comment.
Ensure proper address comparison in authority checks.
Comparing addresses as strings may lead to incorrect results due to formatting differences or case sensitivity. It's safer to decode the addresses and compare their byte representations using bytes.Equal to ensure accurate validation.
Consider modifying the code in both MsgMint and MsgBurn as follows:
authorityAddr, err := h.addressCodec.StringToBytes(msg.Authority)
if err != nil {
return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid authority address: %s", err)
}
adminAddr, err := h.addressCodec.StringToBytes(authorityMetadata.GetAdmin())
if err != nil {
return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid admin address: %s", err)
}
if !bytes.Equal(authorityAddr, adminAddr) {
return nil, types.ErrUnauthorized
}Also applies to: 192-194
| from, err := h.addressCodec.StringToBytes(msg.BurnFromAddress) | ||
| if err != nil { | ||
| return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid to address: %s", err) | ||
| } |
There was a problem hiding this comment.
Correct the error message for invalid address in MsgBurn.
The error message references "invalid to address" when decoding msg.BurnFromAddress. It should say "invalid from address" to accurately reflect the source of the error.
Apply this diff to fix the error message:
if err != nil {
- return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid to address: %s", err)
+ return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid from address: %s", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from, err := h.addressCodec.StringToBytes(msg.BurnFromAddress) | |
| if err != nil { | |
| return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid to address: %s", err) | |
| } | |
| from, err := h.addressCodec.StringToBytes(msg.BurnFromAddress) | |
| if err != nil { | |
| return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid from address: %s", err) | |
| } |
| for _, amount := range amounts { | ||
| supply := k.GetSupply(ctx, amount.GetDenom()) | ||
| supply = supply.Sub(amount) | ||
| k.setSupply(ctx, supply) | ||
| } |
There was a problem hiding this comment.
Prevent negative total supply when burning coins
When burning coins, the supply is reduced by the burn amount without checking if the supply will become negative. This could lead to inconsistencies if the burn amount exceeds the current supply.
Add a check to ensure that the new supply does not become negative:
for _, amount := range amounts {
supply := k.GetSupply(ctx, amount.GetDenom())
+ if supply.IsLT(amount) {
+ return errorsmod.Wrapf(sdkerrors.ErrInvalidCoins, "burn amount exceeds current supply for denom %s", amount.Denom)
+ }
- supply = supply.Sub(amount)
+ newSupply := supply.Sub(amount)
k.setSupply(ctx, newSupply)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, amount := range amounts { | |
| supply := k.GetSupply(ctx, amount.GetDenom()) | |
| supply = supply.Sub(amount) | |
| k.setSupply(ctx, supply) | |
| } | |
| for _, amount := range amounts { | |
| supply := k.GetSupply(ctx, amount.GetDenom()) | |
| if supply.IsLT(amount) { | |
| return errorsmod.Wrapf(sdkerrors.ErrInvalidCoins, "burn amount exceeds current supply for denom %s", amount.Denom) | |
| } | |
| newSupply := supply.Sub(amount) | |
| k.setSupply(ctx, newSupply) | |
| } |
| expErr: true, | ||
| }, | ||
| { | ||
| desc: "Mint tokenfatory denom, valid", |
There was a problem hiding this comment.
Correct typo in test case descriptions
The word "tokenfatory" in the test case descriptions should be corrected to "tokenfactory" to ensure clarity.
Apply these diffs:
- desc: "Mint tokenfatory denom, valid",
+ desc: "Mint tokenfactory denom, valid",- desc: "Mint tokenfatory denom, invalid admin",
+ desc: "Mint tokenfactory denom, invalid admin",- desc: "Mint tokenfatory denom, denom not created",
+ desc: "Mint tokenfactory denom, denom not created",Also applies to: 469-469, 478-478
| desc: "Burn foo denom, insufficient funds", | ||
| msg: &types.MsgBurn{ | ||
| Authority: authority.String(), | ||
| BurnFromAddress: accAddrs[0].String(), | ||
| Amount: sdk.NewCoin(fooDenom, math.NewInt(200)), | ||
| }, | ||
| expErr: true, | ||
| }, | ||
| { | ||
| desc: "Burn bar denom, invalid denom", | ||
| msg: &types.MsgBurn{ | ||
| Authority: authority.String(), | ||
| BurnFromAddress: accAddrs[0].String(), | ||
| Amount: sdk.NewCoin(barDenom, math.NewInt(50)), | ||
| }, | ||
| expErr: true, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test case for burning with invalid amount
Currently, there is no test case for attempting to burn a negative or zero amount. Adding such a test case would ensure that the code correctly handles invalid burn amounts.
Consider adding a new test case:
{
desc: "Burn with invalid amount",
msg: &types.MsgBurn{
Authority: authority.String(),
BurnFromAddress: accAddrs[0].String(),
Amount: sdk.NewCoin(fooDenom, math.NewInt(0)),
},
expErr: true,
},
x/bank/v2/keeper/keeper_test.go
Outdated
| func (s *KeeperTestSuite) TestSendHandler_TokenfactoryDenom() { | ||
| s.SetupTest() | ||
| require := s.Require() | ||
| s.bankKeeper.SetParams(s.ctx, types.Params{ | ||
| DenomCreationFee: sdk.NewCoins(sdk.NewCoin(fooDenom, math.NewInt(10))), | ||
| }) | ||
| handler := keeper.NewHandlers(&s.bankKeeper) | ||
| require.NoError(banktestutil.FundAccount(s.ctx, s.bankKeeper, accAddrs[0], sdk.NewCoins(sdk.NewCoin(fooDenom, math.NewInt(100))))) | ||
|
|
||
| resp, err := handler.MsgCreateDenom(s.ctx, &types.MsgCreateDenom{ | ||
| Sender: accAddrs[0].String(), | ||
| Subdenom: "test", | ||
| }) | ||
| require.NoError(err) | ||
|
|
||
| newDenom := resp.NewTokenDenom | ||
|
|
||
| _, err = handler.MsgMint(s.ctx, &types.MsgMint{ | ||
| Authority: accAddrs[0].String(), | ||
| ToAddress: accAddrs[0].String(), | ||
| Amount: sdk.NewCoin(newDenom, math.NewInt(100)), | ||
| }) | ||
| require.NoError(err) | ||
|
|
||
| for _, tc := range []struct { | ||
| desc string | ||
| msg *types.MsgSend | ||
| expErr bool | ||
| }{ | ||
| { | ||
| desc: "valid", | ||
| msg: &types.MsgSend{ | ||
| FromAddress: accAddrs[0].String(), | ||
| ToAddress: accAddrs[1].String(), | ||
| Amount: sdk.NewCoins(sdk.NewCoin(newDenom, math.NewInt(50))), | ||
| }, | ||
| expErr: false, | ||
| }, | ||
| { | ||
| desc: "insufficient funds", | ||
| msg: &types.MsgSend{ | ||
| FromAddress: accAddrs[0].String(), | ||
| ToAddress: accAddrs[1].String(), | ||
| Amount: sdk.NewCoins(sdk.NewCoin(newDenom, math.NewInt(150))), | ||
| }, | ||
| expErr: true, | ||
| }, | ||
| } { | ||
| s.Run(fmt.Sprintf("Case %s", tc.desc), func() { | ||
| _, err := handler.MsgSend(s.ctx, tc.msg) | ||
| if tc.expErr { | ||
| require.Error(err) | ||
| } else { | ||
| require.NoError(err) | ||
| // Check ToAddress balances after | ||
| toAddr, err := s.addressCodec.StringToBytes(tc.msg.ToAddress) | ||
| require.NoError(err) | ||
| balances := s.bankKeeper.GetAllBalances(s.ctx, toAddr) | ||
| require.Equal(balances, tc.msg.Amount) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test cases for sending zero or negative amounts
In the TestSendHandler_TokenfactoryDenom, there are no test cases for sending zero or negative amounts. Adding these tests can ensure the code correctly rejects invalid transactions.
For example:
{
desc: "send zero amount",
msg: &types.MsgSend{
FromAddress: accAddrs[0].String(),
ToAddress: accAddrs[1].String(),
Amount: sdk.NewCoins(sdk.NewCoin(newDenom, math.NewInt(0))),
},
expErr: true,
},
{
desc: "send negative amount",
msg: &types.MsgSend{
FromAddress: accAddrs[0].String(),
ToAddress: accAddrs[1].String(),
Amount: sdk.NewCoins(sdk.NewCoin(newDenom, math.NewInt(-10))),
},
expErr: true,
},| @@ -16,6 +16,7 @@ | |||
| storetypes "cosmossdk.io/store/types" | |||
| "cosmossdk.io/x/bank/v2/keeper" | |||
| banktestutil "cosmossdk.io/x/bank/v2/testutil" | |||
| "cosmossdk.io/x/bank/v2/types" | |||
There was a problem hiding this comment.
Remove duplicate import of the same package
The package cosmossdk.io/x/bank/v2/types is imported twice, once without an alias and once as banktypes. This redundancy can lead to confusion and is unnecessary.
Apply this diff to remove the redundant import:
- "cosmossdk.io/x/bank/v2/types"Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (8)
x/bank/v2/types/errors.go (3)
3-3: Document the purpose of DONTCOVER.
The DONTCOVER comment typically indicates that this file should be excluded from coverage reports, but it's best practice to explain why coverage is not needed here.
Add a comment explaining why this file is excluded from coverage:
// DONTCOVER
+// This file is excluded from test coverage because error definitions are not testable.1-2: Add package documentation.
The package lacks documentation explaining its purpose and usage.
Add package documentation:
package types
+// Package types provides type definitions and error handling for the bank/v2 module.
+// It includes sentinel errors for token factory operations and related functionality.
// DONTCOVERAlso applies to: 5-9
13-19: Improve error message consistency.
The error messages could be more consistent and descriptive:
- ErrDenomExists = errorsmod.Register(ModuleName, 2, "attempting to create a denom that already exists (has bank metadata)")
- ErrUnauthorized = errorsmod.Register(ModuleName, 3, "unauthorized account")
- ErrInvalidDenom = errorsmod.Register(ModuleName, 4, "invalid denom")
- ErrInvalidCreator = errorsmod.Register(ModuleName, 5, "invalid creator")
+ ErrDenomExists = errorsmod.Register(ModuleName, 2, "denomination already exists")
+ ErrUnauthorized = errorsmod.Register(ModuleName, 3, "unauthorized account: missing required permissions")
+ ErrInvalidDenom = errorsmod.Register(ModuleName, 4, "invalid denomination format")
+ ErrInvalidCreator = errorsmod.Register(ModuleName, 5, "invalid creator address")x/bank/v2/keeper/handlers.go (2)
68-68: Address the TODO comment regarding governance
The comment // TODO: should be gov? suggests uncertainty about whether MsgChangeAdmin should be governed by governance procedures. Consider whether changing the admin of a denomination should require governance approval to enhance security and align with protocol standards.
Would you like assistance in evaluating if this operation should be controlled by governance, possibly by integrating it with the governance module?
124-125: Typo in comment
The comment // Check if is a tokenfatory denom contains a typo. It should be "tokenfactory" instead of "tokenfatory".
Apply this diff to fix the typo:
- // Check if is a tokenfatory denom
+ // Check if it is a tokenfactory denomx/bank/v2/keeper/keeper_test.go (3)
257-259: Consider Adding Test Case Descriptions
Adding comments to explain each test case enhances readability and provides context for future maintainers.
For example:
{
desc: "subdenom too long",
// Test with a subdenom exceeding the maximum allowed length.
// Expectation: The creation should fail.
...
},460-466: Typo in Test Description and Variable Name
There's a typo in the test description and variable name "tokenfatory" instead of "tokenfactory".
Correct the typos:
-desc: "Mint tokenfatory denom, valid",
+desc: "Mint tokenfactory denom, valid",Also, ensure consistency in comments and variable naming throughout the code.
Line range hint 567-567: Ensure Test Coverage for Negative Cases
While the tests include cases for insufficient funds, consider adding test cases for invalid addresses or unsupported denominations to cover additional negative scenarios.
Add test cases like:
{
desc: "invalid to address",
msg: &types.MsgSend{
FromAddress: accAddrs[0].String(),
ToAddress: "invalid_address",
Amount: sdk.NewCoins(sdk.NewCoin(newDenom, math.NewInt(50))),
},
expErr: true,
},
{
desc: "unsupported denomination",
msg: &types.MsgSend{
FromAddress: accAddrs[0].String(),
ToAddress: accAddrs[1].String(),
Amount: sdk.NewCoins(sdk.NewCoin("unsupported_denom", math.NewInt(50))),
},
expErr: true,
},📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/bank/v2/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
- x/bank/proto/cosmos/bank/v2/tx.proto (2 hunks)
- x/bank/v2/keeper/handlers.go (3 hunks)
- x/bank/v2/keeper/keeper_test.go (2 hunks)
- x/bank/v2/types/errors.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
x/bank/v2/keeper/handlers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/keeper/keeper_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/bank/v2/types/errors.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (8)
x/bank/v2/types/errors.go (1)
11-20:
Review error definitions and constants.
Several issues to address:
- Error codes should be documented and sequential (currently jumps from 5 to 8)
- References undefined constants (
ModuleName,MaxSubdenomLength,MaxCreatorLength) - Consider using constants for error codes for better maintainability
Let's verify the missing constants:
Suggested improvements:
+// Error codes for bank/v2 module
+const (
+ ErrCodeDenomExists = 2
+ ErrCodeUnauthorized = 3
+ ErrCodeInvalidDenom = 4
+ ErrCodeInvalidCreator = 5
+ ErrCodeSubdenomTooLong = 8
+ ErrCodeCreatorTooLong = 9
+ ErrCodeDenomDoesNotExist = 10
+)
// x/tokenfactory module sentinel errors
var (
- ErrDenomExists = errorsmod.Register(ModuleName, 2, "attempting to create a denom that already exists (has bank metadata)")
+ ErrDenomExists = errorsmod.Register(ModuleName, ErrCodeDenomExists, "attempting to create a denom that already exists (has bank metadata)")
// ... apply similar changes for other errors
)x/bank/v2/keeper/handlers.go (3)
55-66: Function MsgCreateDenom implementation looks good
The implementation correctly handles the creation of a new denomination and returns the appropriate response.
77-79: Duplicate comment: The issue regarding address comparison has been previously noted and is still applicable.
165-165: Validation check is appropriate
The use of msg.Amount.IsPositive() correctly ensures that the minted amount is positive.
x/bank/v2/keeper/keeper_test.go (4)
353-416: Clarify Gas Consumption Comments and Calculations
The comments and calculations around gas consumption could be clarified to enhance understanding.
- Correct the typo in the comment on line 362 by removing the duplicate word "consume".
- Provide examples or references for where the ~30k additional gas consumption comes from.
- Ensure that the test accurately reflects the intended gas usage checks.
Apply this diff to fix the typo:
-// In experience, the total amount of gas consumed should consume be ~30k more
+// In experience, the total amount of gas consumed should be ~30k more494-496: 🛠️ Refactor suggestion
Simplify Address Conversion
The conversion from string to bytes for addresses can be simplified by using sdk.AccAddressFromBech32.
Update the code as follows:
-toAddr, err := s.addressCodec.StringToBytes(tc.msg.ToAddress)
+toAddr, err := sdk.AccAddressFromBech32(tc.msg.ToAddress)This utilizes the SDK's built-in function for address conversion and improves code reliability.
Likely invalid or redundant comment.
313-314:
Check for Errors When Setting Params
Currently, the code assumes that SetParams will succeed without error. It's safer to check for errors to handle unexpected failures.
Apply this change to handle potential errors:
-require.NoError(s.bankKeeper.SetParams(s.ctx, tc.denomCreationFee))
+err := s.bankKeeper.SetParams(s.ctx, tc.denomCreationFee)
+require.NoError(err)Likely invalid or redundant comment.
295-299: Verify Insufficient Funds Handling
Ensure that the test case correctly verifies the behavior when the account doesn't have enough funds to pay the denom creation fee.
Run the following script to check if the account balance is less than the required denom creation fee:
Ensure that the account balance is indeed less than 5,000,000,000 units of the primary denom.
✅ Verification successful
Test Case Correctly Verifies Insufficient Funds Scenario
The test case is properly structured to verify insufficient funds handling. Here's why:
- The account is funded with
twoDenomCreationFee(50,000,000 tokens) - The test attempts to create a denom with
largeCreationFee(5,000,000,000 tokens) - The
valid: falseexpectation correctly anticipates the failure - The difference in amounts (5,000,000,000 > 50,000,000) ensures the account has insufficient funds
The test case effectively validates that denom creation fails when the account balance is less than the required fee.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify account balance prior to denom creation.
# Test: Check account balance.
# Expect: Balance is less than the largeCreationFee.
# Retrieve the account balance for accAddrs[0].
curl -s 'http://localhost:26657/abci_query?path="account/balance"&data="'$(echo -n "${accAddrs[0]}" | base64)'"' | jq '.result.response.value' | base64 --decode
# Compare the balance with the largeCreationFee (5,000,000,000).
# Ensure that the balance is insufficient.
Length of output: 294
Script:
#!/bin/bash
# Let's examine the test file to understand how the test case is structured
rg -A 20 "account doesn't have enough to pay for denom creation fee" x/bank/v2/keeper/keeper_test.go
# Also check how largeCreationFee is defined
rg "largeCreationFee.*:=" x/bank/v2/keeper/keeper_test.go
Length of output: 991
Script:
#!/bin/bash
# Let's find how largeCreationFee is defined in the test file
rg -B 10 "largeCreationFee" x/bank/v2/keeper/keeper_test.go
# Also check the test setup and funding
rg "FundAccount.*accAddrs" x/bank/v2/keeper/keeper_test.go
Length of output: 1744
| // MsgCreateDenom represents a message to create new denom. | ||
| message MsgCreateDenom { | ||
| option (cosmos.msg.v1.signer) = "sender"; | ||
| option (amino.name) = "cosmos-sdk/x/bank/v2/MsgCreateDenom"; | ||
|
|
||
| string sender = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | ||
| string subdenom = 2; | ||
| } |
There was a problem hiding this comment.
Add validation rules for the subdenom field
To prevent malicious denom creation and ensure consistency, consider adding validation rules for the subdenom field using the gogoproto.moretags option with JSON validation tags.
Apply this addition to the subdenom field:
- string subdenom = 2;
+ string subdenom = 2 [(gogoproto.moretags) = "validate:\"subdenom\""];This will enforce the standard Cosmos SDK denomination format rules.
Committable suggestion was skipped due to low confidence.
| // MsgChangeAdmin represents a message to change admin of denom. | ||
| message MsgChangeAdmin { | ||
| option (cosmos.msg.v1.signer) = "sender"; | ||
| option (amino.name) = "cosmos-sdk/x/bank/v2/MsgChangeAdmin"; | ||
|
|
||
| string sender = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | ||
| string denom = 2; | ||
| string new_admin = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | ||
| } |
There was a problem hiding this comment.
Add validation and event documentation
Two improvements needed:
- The
denomfield should have proper validation - Missing documentation about emitted events
Apply these changes:
// MsgChangeAdmin represents a message to change admin of denom.
+// Events emitted:
+// - EventType: "change_admin"
+// Attributes:
+// - "denom": the denomination being modified
+// - "old_admin": the previous admin address
+// - "new_admin": the new admin address
message MsgChangeAdmin {
option (cosmos.msg.v1.signer) = "sender";
option (amino.name) = "cosmos-sdk/x/bank/v2/MsgChangeAdmin";
string sender = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
- string denom = 2;
+ string denom = 2 [(cosmos_proto.scalar) = "cosmos.DenomString"];
string new_admin = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // MsgChangeAdmin represents a message to change admin of denom. | |
| message MsgChangeAdmin { | |
| option (cosmos.msg.v1.signer) = "sender"; | |
| option (amino.name) = "cosmos-sdk/x/bank/v2/MsgChangeAdmin"; | |
| string sender = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | |
| string denom = 2; | |
| string new_admin = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | |
| } | |
| // MsgChangeAdmin represents a message to change admin of denom. | |
| // Events emitted: | |
| // - EventType: "change_admin" | |
| // Attributes: | |
| // - "denom": the denomination being modified | |
| // - "old_admin": the previous admin address | |
| // - "new_admin": the new admin address | |
| message MsgChangeAdmin { | |
| option (cosmos.msg.v1.signer) = "sender"; | |
| option (amino.name) = "cosmos-sdk/x/bank/v2/MsgChangeAdmin"; | |
| string sender = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | |
| string denom = 2 [(cosmos_proto.scalar) = "cosmos.DenomString"]; | |
| string new_admin = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | |
| } |
| if msg.Authority != authorityMetadata.GetAdmin() { | ||
| return nil, types.ErrUnauthorized | ||
| } |
There was a problem hiding this comment.
Ensure proper address comparison in MsgMint
Comparing addresses as strings (if msg.Authority != authorityMetadata.GetAdmin()) may lead to incorrect results due to formatting differences. To ensure accurate validation, decode the addresses and compare their byte representations using bytes.Equal.
Apply this diff to compare addresses correctly:
+ senderAddr, err := h.addressCodec.StringToBytes(msg.Authority)
+ if err != nil {
+ return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid authority address: %s", err)
+ }
+ adminAddr, err := h.addressCodec.StringToBytes(authorityMetadata.GetAdmin())
+ if err != nil {
+ return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid admin address: %s", err)
+ }
- if msg.Authority != authorityMetadata.GetAdmin() {
+ if !bytes.Equal(senderAddr, adminAddr) {
return nil, types.ErrUnauthorized
}Committable suggestion was skipped due to low confidence.
x/bank/v2/keeper/keeper_test.go
Outdated
| var ( | ||
| primaryDenom = "foo" | ||
| secondaryDenom = "bar" | ||
| defaultDenomCreationFee = banktypes.Params{DenomCreationFee: sdk.NewCoins(sdk.NewCoin(primaryDenom, math.NewInt(50_000_000)))} | ||
| twoDenomCreationFee = banktypes.Params{DenomCreationFee: sdk.NewCoins(sdk.NewCoin(primaryDenom, math.NewInt(50_000_000)), sdk.NewCoin(secondaryDenom, math.NewInt(50_000_000)))} | ||
| nilCreationFee = banktypes.Params{DenomCreationFee: nil} | ||
| largeCreationFee = banktypes.Params{DenomCreationFee: sdk.NewCoins(sdk.NewCoin(primaryDenom, math.NewInt(5_000_000_000)))} | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Denom Creation Fee Initialization
The initialization of the denom creation fees can be made more readable by defining the fees upfront and using them directly in the test cases. This improves clarity and reduces redundancy.
Consider defining the fees like so:
defaultFee := sdk.NewCoins(sdk.NewCoin(primaryDenom, math.NewInt(50_000_000)))
twoFees := sdk.NewCoins(
sdk.NewCoin(primaryDenom, math.NewInt(50_000_000)),
sdk.NewCoin(secondaryDenom, math.NewInt(50_000_000)),
)
largeFee := sdk.NewCoins(sdk.NewCoin(primaryDenom, math.NewInt(5_000_000_000)))Then, update the params accordingly:
defaultDenomCreationFee: banktypes.Params{DenomCreationFee: defaultFee},
twoDenomCreationFee: banktypes.Params{DenomCreationFee: twoFees},
largeCreationFee: banktypes.Params{DenomCreationFee: largeFee},| s.Run(fmt.Sprintf("Case %s", tc.desc), func() { | ||
| if tc.setup != nil { | ||
| tc.setup() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Repeated Setup in Loop
Calling s.SetupTest() inside the loop resets the entire test suite state for each test case, which might not be necessary and could increase test execution time.
Consider moving s.SetupTest() to the beginning of the TestCreateDenom function or resetting only the necessary parts within each test case.
| resp, err := handler.MsgCreateDenom(s.ctx, &types.MsgCreateDenom{ | ||
| Sender: accAddrs[0].String(), | ||
| Subdenom: "test", | ||
| }) | ||
| require.NoError(err) | ||
|
|
||
| newDenom := resp.NewTokenDenom | ||
| authority := authtypes.NewModuleAddress("gov") |
There was a problem hiding this comment.
Handle Potential Errors After MsgCreateDenom
After calling MsgCreateDenom, it's good practice to check if newDenom is not empty and handle any unexpected scenarios.
Add a check to ensure newDenom is valid:
require.NotEmpty(newDenom, "NewTokenDenom should not be empty")| desc: "Mint tokenfatory denom, denom not created", | ||
| msg: &types.MsgMint{ | ||
| Authority: accAddrs[0].String(), | ||
| ToAddress: accAddrs[1].String(), | ||
| Amount: sdk.NewCoin(newDenom+"s", math.NewInt(100)), | ||
| }, | ||
| expErr: true, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test Case for Non-Existent Denom Should Verify Error Message
In the test case where the denom is not created, it would be beneficial to check that the error received is the expected one to ensure the code fails for the right reason.
Modify the test case to verify the error message:
if tc.expErr {
require.Error(err)
require.Contains(err.Error(), "denomination does not exist")
} else {
...
}| toAddr, err := s.addressCodec.StringToBytes(tc.msg.ToAddress) | ||
| require.NoError(err) | ||
| balances := s.bankKeeper.GetAllBalances(s.ctx, toAddr) | ||
| require.Equal(balances, tc.msg.Amount) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify Complete Balance After Transfer
After sending coins, it's good practice to verify the sender's balance as well to ensure that the coins were deducted properly.
Add checks for the sender's balance:
fromAddr, err := sdk.AccAddressFromBech32(tc.msg.FromAddress)
require.NoError(err)
balances := s.bankKeeper.GetAllBalances(s.ctx, fromAddr)
expectedBalance := sdk.NewCoins(sdk.NewCoin(newDenom, math.NewInt(100).Sub(tc.msg.Amount[0].Amount)))
require.Equal(balances, expectedBalance)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (18)
x/bank/proto/cosmos/bank/v2/query.proto (1)
40-44: Consider adding scalar validation for denom fieldWhile the request message is well-structured, consider adding the cosmos_proto.scalar option to validate the denom field format, similar to how address fields are validated in other messages.
message QueryDenomAuthorityMetadataRequest { - string denom = 1 [(gogoproto.moretags) = "yaml:\"denom\""]; + string denom = 1 [ + (gogoproto.moretags) = "yaml:\"denom\"", + (cosmos_proto.scalar) = "cosmos.DenomString" + ]; }tests/systemtests/genesis_io.go (2)
12-13: Consider consolidating duplicate math importsThe file imports the same package twice with different aliases. Consider using the existing
sdkmathalias consistently throughout the file to avoid confusion.-import ( - "cosmossdk.io/math" - sdkmath "cosmossdk.io/math" -) +import ( + sdkmath "cosmossdk.io/math" -)
46-53: Implementation looks good with minor consistency suggestionThe function follows the established pattern for genesis mutators and properly handles the creation of denomination fee settings.
Consider returning
statedirectly instead of converting to []byte for consistency with other mutator functions:func SetDenomCreationFee(t *testing.T, denom string, amount math.Int) GenesisMutator { t.Helper() return func(genesis []byte) []byte { state, err := sjson.SetBytes(genesis, "app_state.bankv2.params.denom_creation_fee", sdk.NewCoins(sdk.NewCoin(denom, amount))) require.NoError(t, err) - return []byte(state) + return state } }tests/systemtests/bankv2_test.go (3)
89-89: Remove debug print statements.Debug print statements should be removed from the test code.
- fmt.Println("valBalance", valBalanceBefore) - fmt.Println("txResult", txResult) - fmt.Println("valBalanceAfter", valBalanceAfter)Also applies to: 93-93, 99-99
61-102: Enhance test coverage for denom creation.While the test verifies the basic functionality and fee deduction, consider adding these additional test cases:
- Verify that the created denom exists and has correct metadata
- Test creation with invalid subdenom
- Test creation without sufficient fee balance
- Verify that the same subdenom cannot be created twice
Here's a suggested addition to verify the created denom exists:
// Verify the created denom exists denomPath := fmt.Sprintf("factory/%s/%s", valAddr, subDenom) raw = cli.CustomQuery("q", "bankv2", "denom-metadata", denomPath) require.True(t, gjson.Get(raw, "metadata.base").Exists(), "denom metadata should exist")
76-79: Consider extracting test configuration.The fee configuration could be moved to a helper function or test constants for better reusability across tests.
const ( defaultDenomCreationFee = 1000000 defaultFeeDenom = "stake" ) func setupDenomCreationFee(t *testing.T, sut *SystemUnderTest) { sut.ModifyGenesisJSON( t, SetDenomCreationFee(t, defaultFeeDenom, math.NewInt(defaultDenomCreationFee)), ) }x/bank/proto/cosmos/bank/v2/tx.proto (3)
47-54: Add event documentation for MsgCreateDenomPlease document the events emitted by this message handler. This helps clients properly handle and track denom creation events.
// MsgCreateDenom represents a message to create new denom. +// Events emitted: +// - EventType: "create_denom" +// Attributes: +// - "sender": the address creating the denom +// - "new_token_denom": the full denomination of the created token message MsgCreateDenom {
82-83: Add validation for mint amountConsider adding validation rules to ensure the mint amount is positive and within acceptable limits:
- cosmos.base.v1beta1.Coin amount = 3 [(gogoproto.nullable) = false]; + cosmos.base.v1beta1.Coin amount = 3 [ + (gogoproto.nullable) = false, + (validate.rules).message = { + required: true + } + ];
89-99: Enhance MsgBurn documentationPlease add comprehensive documentation about:
- Events emitted during burn operation
- Authority requirements and restrictions
- Validation rules for burn amount
// MsgBurn is the Msg/Burn request type. +// Events emitted: +// - EventType: "burn" +// Attributes: +// - "authority": the address authorized to burn tokens +// - "burn_from_address": the address from which tokens are burned +// - "amount": the amount of tokens burned +// +// The message fails if: +// - The authority is not properly authorized +// - The burn amount exceeds the balance of burn_from_address +// - The burn amount is zero or negative message MsgBurn {x/bank/v2/keeper/keeper_test.go (7)
244-251: Consider using test constants for better maintainabilityThe fee configurations could be moved to package-level constants to improve maintainability and reusability across tests.
+const ( + defaultFeeAmount = 50_000_000 + largeFeeAmount = 5_000_000_000 +) - defaultDenomCreationFee = banktypes.Params{DenomCreationFee: sdk.NewCoins(sdk.NewCoin(primaryDenom, math.NewInt(50_000_000)))} + defaultDenomCreationFee = banktypes.Params{DenomCreationFee: sdk.NewCoins(sdk.NewCoin(primaryDenom, math.NewInt(defaultFeeAmount)))} - largeCreationFee = banktypes.Params{DenomCreationFee: sdk.NewCoins(sdk.NewCoin(primaryDenom, math.NewInt(5_000_000_000)))} + largeCreationFee = banktypes.Params{DenomCreationFee: sdk.NewCoins(sdk.NewCoin(primaryDenom, math.NewInt(largeFeeAmount)))}
253-306: Add test cases for additional edge casesConsider adding test cases for:
- Empty subdenom
- Subdenom with only spaces
- Unicode characters in subdenom
- Maximum allowed subdenom length (boundary test)
364-365: Improve documentation for the offset constantThe
offsetconstant's purpose and calculation basis should be documented.- const offset = 50000 + // offset represents the maximum additional gas that may be consumed by auxiliary operations + // during denom creation (e.g., state operations, string manipulations). + // This value was determined empirically through testing. + const offset = 50000
411-414: Add assertion messages for better test diagnosticsThe require statements should include messages to clarify failures.
- s.Require().Greater(gasConsumed, tc.gasConsume) - s.Require().Less(gasConsumed, tc.gasConsume+offset) + s.Require().Greater(gasConsumed, tc.gasConsume, + "gas consumed should be greater than specified amount") + s.Require().Less(gasConsumed, tc.gasConsume+offset, + "gas consumed should be less than specified amount plus offset")
436-501: Enhance test coverage for MintHandlerConsider adding test cases for:
- Minting zero amount
- Minting to an invalid address
- Minting with empty denom
- Verifying total supply changes after mint
Also, enhance balance verification:
require.NoError(err) // Check ToAddress balance after toAddr, err := s.addressCodec.StringToBytes(tc.msg.ToAddress) require.NoError(err) balance := s.bankKeeper.GetBalance(s.ctx, toAddr, tc.msg.Amount.Denom) require.Equal(balance, tc.msg.Amount) + // Verify total supply + supply := s.bankKeeper.GetSupply(s.ctx, tc.msg.Amount.Denom) + require.Equal(supply.Amount, tc.msg.Amount.Amount)
608-620: Enhance balance verification after burnThe balance verification could be more thorough by checking the total supply reduction.
require.NoError(err) // Check ToAddress balance after afterBalances := s.bankKeeper.GetAllBalances(s.ctx, fromAddr) require.Equal(beforeBalances.Sub(afterBalances...), sdk.NewCoins(tc.msg.Amount)) + // Verify total supply reduction + supply := s.bankKeeper.GetSupply(s.ctx, tc.msg.Amount.Denom) + require.Equal(supply.Amount, + beforeBalances.AmountOf(tc.msg.Amount.Denom).Sub(tc.msg.Amount.Amount))
649-673: Add more test cases for send handlerConsider adding test cases for:
- Sending to self
- Sending to a module account
- Sending with invalid addresses
- Sending multiple denominations
x/bank/v2/keeper/handlers.go (2)
66-66: Clarify the responsibility ofMsgChangeAdminfunctionThe TODO comment on line 66 suggests that changing the admin may be more appropriate within the governance module (
gov). Consider whether this functionality should be handled via governance proposals to ensure proper community oversight and adherence to protocol governance practices.Would you like assistance in integrating this functionality with the governance module or opening a GitHub issue to discuss this further?
122-122: Fix typo in comment: 'tokenfatory' should be 'token factory'Minor typo on line 122. Correcting it enhances code readability.
Apply this diff to fix the typo:
- // Check if is a tokenfatory denom + // Check if it is a token factory denom
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/bank/v2/types/query.pb.gois excluded by!**/*.pb.gox/bank/v2/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
tests/systemtests/bankv2_test.go(2 hunks)tests/systemtests/genesis_io.go(2 hunks)x/bank/proto/cosmos/bank/v2/query.proto(2 hunks)x/bank/proto/cosmos/bank/v2/tx.proto(2 hunks)x/bank/v2/keeper/handlers.go(4 hunks)x/bank/v2/keeper/keeper.go(5 hunks)x/bank/v2/keeper/keeper_test.go(2 hunks)x/bank/v2/types/codec.go(1 hunks)x/bank/v2/types/params.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
tests/systemtests/bankv2_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/systemtests/genesis_io.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
x/bank/v2/keeper/handlers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/keeper/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/keeper/keeper_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/bank/v2/types/codec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/types/params.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (18)
x/bank/v2/types/codec.go (1)
12-12: LGTM: Clean interface registration for MsgCreateDenom
The addition of MsgCreateDenom to the interface registry follows the established pattern and is necessary for the new denomination management functionality.
x/bank/proto/cosmos/bank/v2/query.proto (2)
9-9: LGTM: Import statement is correctly structured
The new import for authority metadata is properly formatted and follows the module's package structure.
46-50: Verify DenomAuthorityMetadata type definition
The response message structure looks good with appropriate protobuf options. Let's verify the imported DenomAuthorityMetadata type definition.
✅ Verification successful
DenomAuthorityMetadata type is properly defined and imported
The DenomAuthorityMetadata message is well-defined in x/bank/proto/cosmos/bank/v2/authorityMetadata.proto and correctly imported in the query.proto file. The type contains an admin field with appropriate protobuf options and documentation. The usage in the QueryDenomAuthorityMetadataResponse is consistent with its definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the DenomAuthorityMetadata type definition
# Expected: Should find the type definition in the imported proto file
# Search for the DenomAuthorityMetadata message definition
rg -t proto "message DenomAuthorityMetadata" --glob "x/bank/proto/**/*.proto"
Length of output: 109
Script:
#!/bin/bash
# Let's try a different approach to find the DenomAuthorityMetadata definition
# Using fd to find proto files and grep for the definition
fd -e proto . x/bank/proto | xargs grep -l "message DenomAuthorityMetadata"
# Also let's check the imports in the query.proto file to see where this type might be defined
cat x/bank/proto/cosmos/bank/v2/query.proto | grep "import"
Length of output: 410
Script:
#!/bin/bash
# Let's verify the DenomAuthorityMetadata definition in authorityMetadata.proto
cat x/bank/proto/cosmos/bank/v2/authorityMetadata.proto
Length of output: 635
tests/systemtests/bankv2_test.go (1)
9-9: LGTM!
The new import is necessary for the math operations in the test and is correctly placed.
x/bank/v2/keeper/keeper_test.go (1)
19-21: Duplicate import already flagged.
x/bank/v2/types/params.go (2)
11-15: Proper Initialization of Parameters in NewParams.
The NewParams function correctly initializes the Params struct with DenomCreationFee and DenomCreationGasConsume, enhancing the configurability of the module.
23-33: Comprehensive Validation in Params.Validate().
The Validate method appropriately calls validateDenomCreationFee and validateDenomCreationGasConsume to ensure all parameters are properly validated, maintaining the integrity of the module's configuration.
x/bank/v2/keeper/keeper.go (11)
31-32: LGTM!
The addition of denomMetadata and denomAuthority fields to the Keeper struct is a good way to manage denomination metadata and authority metadata. The use of collections.Map ensures efficient storage and retrieval of this data.
47-48: LGTM!
The initialization of denomMetadata and denomAuthority fields in the NewKeeper constructor is correct. Using collections.NewMap with appropriate prefixes and codecs ensures proper storage and serialization of the metadata.
181-185: ****
Handle errors explicitly in HasSupply method.
In the HasSupply method, the error returned from k.supply.Has(ctx, denom) is ignored. According to Go best practices and the Uber Golang style guide, errors should be handled explicitly to prevent unexpected behaviors.
Consider modifying the method to handle errors appropriately:
func (k Keeper) HasSupply(ctx context.Context, denom string) bool {
has, err := k.supply.Has(ctx, denom)
- return has && err == nil
+ if err != nil {
+ // Handle or log the error as needed
+ return false
+ }
+ return has
}This change ensures that any errors are not silently ignored, which could mask underlying issues with the supply store.
197-206: ****
Handle errors returned by IterateAccountBalances in GetAllBalances.
After modifying IterateAccountBalances to return an error, the GetAllBalances method should handle this error to maintain robustness.
Update GetAllBalances to handle the error:
func (k Keeper) GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins {
balances := sdk.NewCoins()
- k.IterateAccountBalances(ctx, addr, func(balance sdk.Coin) bool {
+ err := k.IterateAccountBalances(ctx, addr, func(balance sdk.Coin) bool {
balances = balances.Add(balance)
return false
})
+ if err != nil {
+ // Handle or log the error as needed
+ return balances
+ }
return balances.Sort()
}211-218: ****
Avoid using panic in IterateAccountBalances; return errors instead.
In IterateAccountBalances, the function panics if an error occurs during iteration. Per the Uber Go Style Guide, panics should be avoided in library code.
Consider modifying the function to return an error:
-func (k Keeper) IterateAccountBalances(ctx context.Context, addr sdk.AccAddress, cb func(sdk.Coin) bool) {
+func (k Keeper) IterateAccountBalances(ctx context.Context, addr sdk.AccAddress, cb func(sdk.Coin) bool) error {
err := k.balances.Walk(ctx, collections.NewPrefixedPairRange[[]byte, string](addr), func(key collections.Pair[[]byte, string], value math.Int) (stop bool, err error) {
return cb(sdk.NewCoin(key.K2(), value)), nil
})
if err != nil {
- panic(err)
+ return err
}
+ return nil
}This approach allows callers to handle errors without unexpected panics.
220-225: ****
Return errors explicitly in GetDenomMetaData.
In GetDenomMetaData, the error from k.denomMetadata.Get(ctx, denom) is converted to a boolean. For clearer error handling, consider returning the error directly.
Modify the method:
-func (k Keeper) GetDenomMetaData(ctx context.Context, denom string) (types.Metadata, bool) {
+func (k Keeper) GetDenomMetaData(ctx context.Context, denom string) (types.Metadata, error) {
m, err := k.denomMetadata.Get(ctx, denom)
- return m, err == nil
+ if err != nil {
+ return types.Metadata{}, err
+ }
+ return m, nil
}Update any code that calls GetDenomMetaData to handle the returned error.
227-231: ****
Handle errors explicitly in HasDenomMetaData method.
Similar to HasSupply, the HasDenomMetaData function implicitly handles errors, which could suppress important error information.
Modify the function to handle and return errors explicitly:
func (k Keeper) HasDenomMetaData(ctx context.Context, denom string) bool {
has, err := k.denomMetadata.Has(ctx, denom)
- return has && err == nil
+ if err != nil {
+ // Handle or log the error as needed
+ return false
+ }
+ return has
}This change ensures that any errors are properly propagated to the caller.
233-242: ****
Handle errors returned by IterateAllDenomMetaData in GetAllDenomMetaData.
With IterateAllDenomMetaData now returning an error, GetAllDenomMetaData should handle this error to ensure proper error propagation.
Update GetAllDenomMetaData as follows:
-func (k Keeper) GetAllDenomMetaData(ctx context.Context) []types.Metadata {
+func (k Keeper) GetAllDenomMetaData(ctx context.Context) ([]types.Metadata, error) {
denomMetaData := make([]types.Metadata, 0)
- k.IterateAllDenomMetaData(ctx, func(metadata types.Metadata) bool {
+ err := k.IterateAllDenomMetaData(ctx, func(metadata types.Metadata) bool {
denomMetaData = append(denomMetaData, metadata)
return false
})
+ if err != nil {
+ // Handle or log the error as needed
+ return nil, err
+ }
return denomMetaData
}Ensure that any callers of GetAllDenomMetaData are updated to handle the returned error.
247-254: ****
Avoid using panic in IterateAllDenomMetaData; return errors instead.
Similar to the previous comment, using panic in IterateAllDenomMetaData is not recommended. Errors should be propagated up to the caller to handle them gracefully.
Modify the method to return the error:
-func (k Keeper) IterateAllDenomMetaData(ctx context.Context, cb func(types.Metadata) bool) {
+func (k Keeper) IterateAllDenomMetaData(ctx context.Context, cb func(types.Metadata) bool) error {
err := k.denomMetadata.Walk(ctx, nil, func(_ string, metadata types.Metadata) (stop bool, err error) {
return cb(metadata), nil
})
if err != nil {
- panic(err)
+ return err
}
+ return nil
}Again, ensure that any code calling this method is updated to handle the returned error.
256-259: LGTM!
The SetDenomMetaData function correctly sets the denomination metadata using the denomMetadata.Set method. The use of denomMetaData.Base as the key ensures that the metadata is stored with the correct denomination.
109-113:
Prevent negative total supply when burning coins.
When burning coins, the current implementation reduces the supply by the burn amount without checking if the supply will become negative. This could lead to inconsistencies if the burn amount exceeds the current supply.
Add a check to ensure that the new supply does not become negative:
for _, amount := range amounts {
supply := k.GetSupply(ctx, amount.GetDenom())
+ if supply.IsLT(amount) {
+ return errorsmod.Wrapf(sdkerrors.ErrInvalidCoins, "burn amount exceeds current supply for denom %s", amount.Denom)
+ }
- supply = supply.Sub(amount)
+ newSupply := supply.Sub(amount)
k.setSupply(ctx, newSupply)
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (9)
x/bank/proto/cosmos/bank/v2/query.proto (1)
40-50: Consider adding field validation and examplesThe new query messages are well-structured and properly documented. However, consider these improvements:
- Add field validation for the
denomfield to ensure it matches the expected format- Include example values in the comments to guide API users
Example validation:
string denom = 1 [ (gogoproto.moretags) = "yaml:\"denom\"", (cosmos_proto.scalar) = "cosmos.Denom" // Add denom validation ];tests/systemtests/genesis_io.go (1)
46-53: LGTM with minor suggestionsThe implementation follows the established pattern for genesis mutators and correctly integrates with the bank/v2 module. Consider adding:
- Input validation for the amount parameter
- Documentation explaining the purpose and usage of this function
+// SetDenomCreationFee sets the denomination creation fee in genesis state. +// Parameters: +// - denom: the denomination of the fee token +// - amount: the amount of tokens required for denomination creation func SetDenomCreationFee(t *testing.T, denom string, amount math.Int) GenesisMutator { + if amount.IsNegative() { + t.Fatal("amount cannot be negative") + } t.Helper()x/bank/v2/client/cli/tx.go (1)
71-76: Enhance command documentation with examples.Consider adding examples and subdenom format requirements to the Long description to improve user experience.
Long: `Create new tokenfactory denom. Note, the '--from' flag is ignored as it is implied from [from_key_or_address]. When using '--dry-run' a key name cannot be used, only a bech32 address. + +Example: + $ %s tx bank create-denom mytoken + +The subdenom must be a valid denomination name consisting of alphanumeric characters, +and may include slashes, colons, dots, and hyphens. `,tests/systemtests/bankv2_test.go (2)
89-89: Remove debug print statements.Debug print statements should be removed from the test as they add noise to the test output.
- fmt.Println("valBalance", valBalanceBefore) - fmt.Println("txResult", txResult) - fmt.Println("valBalanceAfter", valBalanceAfter)Also applies to: 93-93, 99-99
61-102: Enhance test coverage with error cases.The test only covers the happy path. Consider adding test cases for:
- Invalid subdenom format
- Insufficient fee balance
- Duplicate denom creation
Add test function documentation.
Add a function comment describing the test's purpose and test cases.
// TestCreateDenom verifies that: // - A new denomination can be created with the correct fee deduction // - The creation fee is correctly deducted from the creator's account // - The newly created denomination is properly registered in the systemx/bank/proto/cosmos/bank/v2/tx.proto (1)
47-54: Add field documentation for MsgCreateDenomPlease add documentation comments for the fields to improve clarity:
sender: Document who can create denoms (authority, restrictions)subdenom: Document the expected format, constraints, and examplesx/bank/v2/keeper/keeper_test.go (2)
308-310: Consider using test suite setup instead of per-test setup
s.SetupTest()is called for each test case, which might be inefficient. Consider moving common setup to aBeforeTestmethod.+func (s *KeeperTestSuite) BeforeTest(suiteName, testName string) { + s.SetupTest() + // Common setup code here +}
411-414: Enhance gas consumption validationThe current validation only checks if gas consumption falls within a range. Consider adding assertions for specific gas usage patterns.
Add these assertions:
s.Require().Greater(gasConsumed, tc.gasConsume) s.Require().Less(gasConsumed, tc.gasConsume+offset) +// Verify gas consumption increases with parameter value +if tc.gasConsume > 0 { + s.Require().Greater(gasConsumed, previousGasConsumed) +} +previousGasConsumed = gasConsumedx/bank/v2/types/params.go (1)
36-47: Refactor validation functions to accept explicit typesCurrently,
validateDenomCreationFeeaccepts aninterface{}and performs a type assertion. Similarly,validateDenomCreationGasConsumedoes the same. For improved type safety and readability, consider changing these functions to accept explicit types.Apply the following changes to
validateDenomCreationFee:-func validateDenomCreationFee(i interface{}) error { - v, ok := i.(sdk.Coins) - if !ok { - return fmt.Errorf("invalid parameter type: %T", i) - } +func validateDenomCreationFee(v sdk.Coins) error { if v.Validate() != nil { return fmt.Errorf("invalid denom creation fee: %+v", v) } return nil }And modify
validateDenomCreationGasConsumeaccordingly:-func validateDenomCreationGasConsume(i interface{}) error { - v, ok := i.(uint64) - if !ok { - return fmt.Errorf("invalid parameter type: %T", i) - } +func validateDenomCreationGasConsume(v uint64) error { return nil }Update the calls in
Params.Validate():func (p Params) Validate() error { - if err := validateDenomCreationFee(p.DenomCreationFee); err != nil { + if err := validateDenomCreationFee(p.DenomCreationFee); err != nil { return err } - if err := validateDenomCreationGasConsume(p.DenomCreationGasConsume); err != nil { + if err := validateDenomCreationGasConsume(p.DenomCreationGasConsume); err != nil { return err } return nil }This refactoring enhances type safety by eliminating unnecessary type assertions and makes the code more idiomatic.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/bank/v2/types/query.pb.gois excluded by!**/*.pb.gox/bank/v2/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
tests/systemtests/bankv2_test.go(2 hunks)tests/systemtests/genesis_io.go(2 hunks)x/bank/proto/cosmos/bank/v2/query.proto(2 hunks)x/bank/proto/cosmos/bank/v2/tx.proto(2 hunks)x/bank/v2/client/cli/tx.go(2 hunks)x/bank/v2/keeper/handlers.go(4 hunks)x/bank/v2/keeper/keeper.go(5 hunks)x/bank/v2/keeper/keeper_test.go(2 hunks)x/bank/v2/types/codec.go(1 hunks)x/bank/v2/types/params.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
tests/systemtests/bankv2_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/systemtests/genesis_io.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
x/bank/v2/client/cli/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/keeper/handlers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/keeper/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/keeper/keeper_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/bank/v2/types/codec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/types/params.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (23)
x/bank/v2/types/codec.go (1)
12-12: LGTM! Verify message type definition.
The registration of MsgCreateDenom follows the established pattern for message type implementations.
Let's verify the message type is properly defined:
✅ Verification successful
Message type is properly defined and follows SDK conventions
The MsgCreateDenom message type is well-defined in x/bank/proto/cosmos/bank/v2/tx.proto with:
- Correct signer option and amino name annotations
- Required
senderfield with proper cosmos address scalar subdenomfield for denomination creation- Corresponding response message with
new_token_denomfield
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MsgCreateDenom type definition exists
# Expected: Find the message type definition in proto files
# Search for MsgCreateDenom definition
rg -A 5 "message MsgCreateDenom"
Length of output: 875
x/bank/proto/cosmos/bank/v2/query.proto (1)
9-9: LGTM: Import statement is correctly placed and necessary
The new import for authorityMetadata.proto is required for the DenomAuthorityMetadata type and follows the package's conventions.
tests/systemtests/genesis_io.go (1)
46-53: Verify test coverage for denomination creation
Let's ensure this utility function is being used in relevant test cases for the new denomination creation features.
✅ Verification successful
Test coverage for denomination creation is adequate
The utility function SetDenomCreationFee is properly used in TestCreateDenom which tests the denomination creation functionality. The test sets up the genesis state with a specific denom creation fee, which is essential for testing the new denomination management features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of SetDenomCreationFee in test files
echo "Searching for test cases using SetDenomCreationFee..."
rg -l "SetDenomCreationFee" --type go --type-add 'go:*.go' -g '*_test.go'
Length of output: 223
Script:
#!/bin/bash
# Check the test cases in bankv2_test.go that use SetDenomCreationFee
echo "Checking test cases in bankv2_test.go..."
rg "SetDenomCreationFee" tests/systemtests/bankv2_test.go -B 2 -A 5
Length of output: 363
Script:
#!/bin/bash
# Get more context around the test case to understand the test scenario
echo "Getting full test context..."
rg "func.*Test.*Denom" tests/systemtests/bankv2_test.go -B 5 -A 20
Length of output: 716
x/bank/v2/client/cli/tx.go (1)
27-27: Consider AutoCLI migration impact.
The command is correctly registered, but note the TODO comment above suggesting migration to AutoCLI (issue #21682). Ensure this new command is included in the AutoCLI migration plan.
✅ Verification successful
AutoCLI migration is already tracked and implementation is pending
The AutoCLI implementation for bank/v2 is intentionally disabled pending resolution of issue #21682. The new command is correctly registered in the traditional CLI structure, and its migration to AutoCLI will be handled as part of the broader module migration tracked in the mentioned issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any related AutoCLI implementations or discussions
rg -l "AutoCLI.*bank.*v2"
Length of output: 27
Script:
#!/bin/bash
# Let's try a broader search for AutoCLI references in bank module
rg -l "AutoCLI.*bank"
# Also search for any TODO comments related to AutoCLI
rg "TODO.*AutoCLI"
# Check if there are any existing AutoCLI implementations
fd -t f autocli
Length of output: 1215
Script:
#!/bin/bash
# Let's check the content of bank v2 AutoCLI implementation
cat x/bank/v2/autocli.go
# And check the TODO comment context in the file under review
rg -A 5 "TODO: Use AutoCLI commands" x/bank/v2/client/cli/tx.go
Length of output: 1647
x/bank/proto/cosmos/bank/v2/tx.proto (1)
61-69: Add access control validation
Consider adding validation to ensure that only the current admin can initiate the admin change. This could be done through a combination of:
- Keeper-level checks
- Documentation of access control requirements in the message definition
x/bank/v2/keeper/handlers.go (3)
55-64: LGTM! Clean and concise implementation.
The handler correctly delegates to the Keeper's CreateDenom method and properly handles errors.
168-168: Clarify the minting strategy
The TODO comment raises an important architectural question about the minting process. Consider documenting the decision and rationale about whether to mint directly or use an intermediate module.
#!/bin/bash
# Check how minting is handled in other modules
rg "MintCoins.*module" -A 566-66: Address the TODO comment about governance
The comment "TODO: should be gov?" suggests uncertainty about whether this functionality should be governed by the governance module. This architectural decision should be clarified before proceeding.
x/bank/v2/types/params.go (3)
11-15: Function NewParams implementation is correct
The NewParams function appropriately initializes the Params struct with the provided denomCreationFee and gasConsume parameters.
20-20: DefaultParams uses sensible default values
The DefaultParams function correctly calls NewParams with default values for DenomCreationFee and DenomCreationGasConsume. This ensures that the module has valid default parameters.
25-34: Validation logic in Params.Validate() is sound
The Validate method properly checks both DenomCreationFee and DenomCreationGasConsume using their respective validation functions, ensuring that parameter values are valid.
x/bank/v2/keeper/keeper.go (12)
25-32: LGTM!
The new fields authority, addressCodec, denomMetadata, and denomAuthority are added to the Keeper struct to support the management of denomination metadata and authorities. The types and naming conventions are consistent with the existing fields.
47-48: LGTM!
The denomMetadata and denomAuthority fields are initialized using collections.NewMap with appropriate prefixes, keys, and value codecs. This is consistent with the initialization of other fields in the NewKeeper function.
197-206: LGTM!
The GetAllBalances function retrieves all account balances for a given address by iterating over the account's balances using IterateAccountBalances. The balances are accumulated in a sdk.Coins object and sorted before being returned. The implementation is straightforward and follows the expected behavior.
233-242: LGTM!
The GetAllDenomMetaData function retrieves all denomination metadata by iterating over the metadata using IterateAllDenomMetaData. The metadata is collected in a slice and returned. The implementation is straightforward and follows the expected behavior.
256-259: LGTM!
The SetDenomMetaData function sets the denomination metadata by calling k.denomMetadata.Set with the appropriate key and value. The implementation is straightforward and follows the expected behavior.
94-128:
Implement burn restrictions and permission checks.
The BurnCoins function includes a TODO comment for implementing burn restrictions and permissions. Currently, it lacks checks to prevent unauthorized burning of coins, which poses a significant security risk. According to the Uber Go Style Guide, TODO comments should be minimized and, if used, should include a reference to an issue tracker.
Please implement the necessary permission checks to ensure that only authorized entities can burn coins. If this functionality is to be added later, consider linking to an issue or removing the function until it is properly secured.
Do you want me to help implement the burn restrictions and permission checks or open a GitHub issue to track this task?
109-113:
Prevent negative total supply when burning coins.
When burning coins, the supply is reduced by the burn amount without checking if the supply will become negative. This could lead to inconsistencies if the burn amount exceeds the current supply.
Add a check to ensure that the new supply does not become negative:
for _, amount := range amounts {
supply := k.GetSupply(ctx, amount.GetDenom())
+ if supply.IsLT(amount) {
+ return errorsmod.Wrapf(sdkerrors.ErrInvalidCoins, "burn amount exceeds current supply for denom %s", amount.Denom)
+ }
- supply = supply.Sub(amount)
+ newSupply := supply.Sub(amount)
k.setSupply(ctx, newSupply)
}181-185: 🛠️ Refactor suggestion
Handle errors explicitly in HasSupply.
In the HasSupply function, errors from k.supply.Has are implicitly handled by returning has && err == nil. This might suppress underlying errors that should be addressed.
Consider modifying the function to handle the error explicitly and return it:
func (k Keeper) HasSupply(ctx context.Context, denom string) (bool, error) {
has, err := k.supply.Has(ctx, denom)
if err != nil {
return false, err
}
return has, nil
}This change allows callers to handle errors appropriately.
211-218:
Avoid panicking in IterateAccountBalances; return errors instead.
In IterateAccountBalances, the function panics if an error occurs during iteration. According to best practices and the Uber Go Style Guide, panicking in library code is discouraged.
Consider changing the function signature to return an error:
func (k Keeper) IterateAccountBalances(ctx context.Context, addr sdk.AccAddress, cb func(sdk.Coin) bool) error {
err := k.balances.Walk(ctx, collections.NewPrefixedPairRange[[]byte, string](addr), func(key collections.Pair[[]byte, string], value math.Int) (stop bool, err error) {
return cb(sdk.NewCoin(key.K2(), value)), nil
})
if err != nil {
return err
}
return nil
}Callers can then handle the error appropriately without unexpected panics.
220-225: 🛠️ Refactor suggestion
Return errors explicitly in GetDenomMetaData.
In GetDenomMetaData, the error from k.denomMetadata.Get(ctx, denom) is converted to a boolean. For clearer error handling, consider returning the error directly.
Modify the method:
-func (k Keeper) GetDenomMetaData(ctx context.Context, denom string) (types.Metadata, bool) {
+func (k Keeper) GetDenomMetaData(ctx context.Context, denom string) (types.Metadata, error) {
m, err := k.denomMetadata.Get(ctx, denom)
- return m, err == nil
+ if err != nil {
+ return types.Metadata{}, err
+ }
+ return m, nil
}Update any code that calls GetDenomMetaData to handle the returned error.
227-231: 🛠️ Refactor suggestion
Handle errors explicitly in HasDenomMetaData.
Similar to HasSupply, the HasDenomMetaData function implicitly handles errors, which could suppress important error information.
Modify the function to handle and return errors explicitly:
func (k Keeper) HasDenomMetaData(ctx context.Context, denom string) (bool, error) {
has, err := k.denomMetadata.Has(ctx, denom)
if err != nil {
return false, err
}
return has, nil
}This change ensures that any errors are properly propagated to the caller.
247-254:
Avoid panicking in IterateAllDenomMetaData; return errors instead.
Similar to the previous comment, using panic in IterateAllDenomMetaData is not recommended. Errors should be propagated up to the caller to handle them gracefully.
Modify the method to return the error:
-func (k Keeper) IterateAllDenomMetaData(ctx context.Context, cb func(types.Metadata) bool) {
+func (k Keeper) IterateAllDenomMetaData(ctx context.Context, cb func(types.Metadata) bool) error {
err := k.denomMetadata.Walk(ctx, nil, func(_ string, metadata types.Metadata) (stop bool, err error) {
return cb(metadata), nil
})
if err != nil {
- panic(err)
+ return err
}
+ return nil
}Again, ensure that any code calling this method is updated to handle the returned error.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
tests/systemtests/bankv2_test.go (1)
89-89: Remove debug print statements.Debug print statements should be removed from the test as they add noise to the test output.
- fmt.Println("valBalance", valBalanceBefore) - fmt.Println("txResult", txResult) - fmt.Println("valBalanceAfter", valBalanceAfter) - fmt.Println("denoms", raw) - fmt.Println("authority", raw)Also applies to: 93-93, 94-94, 99-99, 106-106, 111-111
x/bank/v2/client/cli/query.go (1)
83-118: Enhance error message for empty denomWhile the implementation is solid, consider making the error message more descriptive to help users understand the issue better.
- return errors.New("empty denom") + return errors.New("denom cannot be empty")x/bank/v2/module.go (1)
Line range hint
119-127: Track TODOs for CLI command generationThere are TODO comments indicating a plan to migrate to autocli. It would be helpful to track these improvements.
Would you like me to create GitHub issues to track the migration of GetTxCmd and GetQueryCmd to autocli?
x/bank/v2/keeper/handlers.go (1)
66-66: Address the TODO comment regarding governance.The comment suggests uncertainty about whether this functionality should be controlled by governance. This architectural decision should be clarified and documented.
Should this operation be restricted to governance proposals? This would provide better security and community oversight for admin changes.
x/bank/proto/cosmos/bank/v2/query.proto (3)
43-43: Consider adding address validation annotation todenomfieldFor improved validation and consistency, consider adding the
(cosmos_proto.scalar) = "cosmos.DenomString"annotation to thedenomfield inQueryDenomAuthorityMetadataRequest.Apply this diff:
message QueryDenomAuthorityMetadataRequest { - string denom = 1; + string denom = 1 [(cosmos_proto.scalar) = "cosmos.DenomString"]; }
42-44: Addoptionstatements for gogoproto settings for consistencyFor consistency with other message definitions in this file, such as
QueryBalanceRequest, consider adding the followingoptionstatements to the new messages to disable equality checking and getter generation:Apply this diff:
message QueryDenomAuthorityMetadataRequest { + option (gogoproto.equal) = false; + option (gogoproto.goproto_getters) = false; string denom = 1; }Repeat the same
optionstatements for the other new messages:message QueryDenomAuthorityMetadataResponse { + option (gogoproto.equal) = false; + option (gogoproto.goproto_getters) = false; DenomAuthorityMetadata authority_metadata = 1 [(gogoproto.nullable) = false]; } message QueryDenomsFromCreatorRequest { + option (gogoproto.equal) = false; + option (gogoproto.goproto_getters) = false; string creator = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; } message QueryDenomsFromCreatorResponse { + option (gogoproto.equal) = false; + option (gogoproto.goproto_getters) = false; repeated string denoms = 1; }
49-49: Add(amino.dont_omitempty) = truetoauthority_metadatafieldTo ensure that the
authority_metadatafield is always serialized and not omitted when empty, add the(amino.dont_omitempty) = trueannotation, similar to other responses in this file.Apply this diff:
message QueryDenomAuthorityMetadataResponse { DenomAuthorityMetadata authority_metadata = 1 [ (gogoproto.nullable) = false, + (amino.dont_omitempty) = true ]; }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/bank/v2/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
tests/systemtests/bankv2_test.go(2 hunks)x/bank/proto/cosmos/bank/v2/query.proto(2 hunks)x/bank/v2/client/cli/query.go(2 hunks)x/bank/v2/keeper/handlers.go(4 hunks)x/bank/v2/module.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tests/systemtests/bankv2_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/bank/v2/client/cli/query.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/keeper/handlers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/v2/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
x/bank/v2/client/cli/query.go (1)
34-35: LGTM: Commands properly registered
The new query commands are correctly registered with the parent command.
x/bank/v2/module.go (1)
101-102: LGTM: Message handlers properly registered
The new message handlers for denomination management (MsgCreateDenom and MsgChangeAdmin) are correctly registered following the module's established patterns.
x/bank/v2/keeper/handlers.go (3)
55-64: LGTM! Clean and concise implementation.
The implementation correctly delegates to the Keeper's CreateDenom method and handles errors appropriately.
75-77: Address comparison needs to be updated.
This issue has been previously identified in the past reviews. Please refer to the existing comments about using proper address comparison with byte representation.
264-297: LGTM! Well-implemented query handlers.
Both query handlers are implemented cleanly with proper error handling. The use of the Walk function for iterating over denoms is efficient and appropriate.
hieuvubk
changed the title
| // authority is the address that controls the module (defaults to x/gov unless overwritten). | ||
| string authority = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | ||
|
|
||
| string burn_from_address = 2 [(cosmos_proto.scalar) = "cosmos.AddressString"]; |
There was a problem hiding this comment.
I have some concerns about this feature. The original token factory was built for a permissioned chain which is always able to do this via migration code.
When a contract can use the TF to mint and burn tokens from any address, this opens the door to fraud.
The burn use-case is not new in other chains but what they do instead is providing transparency via metadata that the users know this is possible. See https://developers.stellar.org/docs/learn/encyclopedia/transactions-specialized/clawbacks for example
Description
Ref: #22251
denomMetadata&authorityMetadatafuncsMsgMint,MsgBurninto bank/v2mint,burnAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
MsgCreateDenom,MsgChangeAdmin,MsgBurn, and their corresponding response types.Bug Fixes
Tests
Documentation