Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(bank/v2): Basic CreateDenom #22264

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions x/bank/proto/cosmos/bank/v2/authorityMetadata.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
syntax = "proto3";
package cosmos.bank.v2;

import "gogoproto/gogo.proto";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 gogoproto is 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)

import "cosmos/base/v1beta1/coin.proto";
Comment on lines +1 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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)


option go_package = "cosmossdk.io/x/bank/v2/types";

// DenomAuthorityMetadata specifies metadata for addresses that have specific
// capabilities over a token factory denom. Right now there is only one Admin
// permission, but is planned to be extended to the future.
message DenomAuthorityMetadata {
option (gogoproto.equal) = true;

// Can be empty for no admin, or a valid osmosis address
string admin = 1 [(gogoproto.moretags) = "yaml:\"admin\""];
}
Comment on lines +15 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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?

60 changes: 59 additions & 1 deletion x/bank/proto/cosmos/bank/v2/bank.proto
Original file line number Diff line number Diff line change
@@ -1,7 +1,65 @@
syntax = "proto3";
package cosmos.bank.v2;

import "gogoproto/gogo.proto";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix missing import: "gogoproto/gogo.proto"

The imported file "gogoproto/gogo.proto" does not exist, which will lead to a compilation error. Please ensure the file path is correct and that the necessary dependency is included in your project.

🧰 Tools
🪛 buf

4-4: import "gogoproto/gogo.proto": file does not exist

(COMPILE)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix the import path for 'gogoproto/gogo.proto'

The import statement at line 4 refers to a file that does not exist, as indicated by the static analysis tool. Please verify that the import path is correct and that all necessary dependencies are included.

🧰 Tools
🪛 buf

4-4: import "gogoproto/gogo.proto": file does not exist

(COMPILE)

import "cosmos_proto/cosmos.proto";
import "cosmos/base/v1beta1/coin.proto";

option go_package = "cosmossdk.io/x/bank/v2/types";

// Params defines the parameters for the bank/v2 module.
message Params {}
message Params {
// DenomCreationFee defines the fee to be charged on the creation of a new
// denom. The fee is drawn from the MsgCreateDenom's sender account, and
// transferred to the community pool.
repeated cosmos.base.v1beta1.Coin denom_creation_fee = 1 [
(gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins",
(gogoproto.moretags) = "yaml:\"denom_creation_fee\"",
(gogoproto.nullable) = false
Comment on lines +15 to +19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Review the use of 'nullable' option with repeated fields

In the denom_creation_fee field, the option (gogoproto.nullable) = false is set for a repeated field. The nullable option is generally not applicable to repeated fields. Please confirm if this option is necessary or remove it to avoid potential confusion.

Apply the following diff to remove the unnecessary nullable option:

 repeated cosmos.base.v1beta1.Coin denom_creation_fee = 1 [
     (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins",
     (gogoproto.moretags)     = "yaml:\"denom_creation_fee\"",
-    (gogoproto.nullable)     = false
 ];
📝 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.

Suggested change
repeated cosmos.base.v1beta1.Coin denom_creation_fee = 1 [
(gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins",
(gogoproto.moretags) = "yaml:\"denom_creation_fee\"",
(gogoproto.nullable) = false
];
repeated cosmos.base.v1beta1.Coin denom_creation_fee = 1 [
(gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins",
(gogoproto.moretags) = "yaml:\"denom_creation_fee\""
];

];

// DenomCreationGasConsume defines the gas cost for creating a new denom.
// This is intended as a spam deterrence mechanism.
//
// See: https://github.com/CosmWasm/token-factory/issues/11
uint64 denom_creation_gas_consume = 2
[(gogoproto.moretags) = "yaml:\"denom_creation_gas_consume\"", (gogoproto.nullable) = true];
}

// DenomUnit represents a struct that describes a given
// denomination unit of the basic token.
message DenomUnit {
// denom represents the string name of the given denom unit (e.g uatom).
string denom = 1;
// exponent represents power of 10 exponent that one must
// raise the base_denom to in order to equal the given DenomUnit's denom
// 1 denom = 10^exponent base_denom
// (e.g. with a base_denom of uatom, one can create a DenomUnit of 'atom' with
// exponent = 6, thus: 1 atom = 10^6 uatom).
uint32 exponent = 2;
// aliases is a list of string aliases for the given denom
repeated string aliases = 3;
}

// Metadata represents a struct that describes
// a basic token.
message Metadata {
string description = 1;
// denom_units represents the list of DenomUnit's for a given coin
repeated DenomUnit denom_units = 2;
// base represents the base denom (should be the DenomUnit with exponent = 0).
string base = 3;
// display indicates the suggested denom that should be
// displayed in clients.
string display = 4;
// name defines the name of the token (eg: Cosmos Atom)
string name = 5 [(cosmos_proto.field_added_in) = "cosmos-sdk 0.43"];
// symbol is the token symbol usually shown on exchanges (eg: ATOM). This can
// be the same as the display.
string symbol = 6 [(cosmos_proto.field_added_in) = "cosmos-sdk 0.43"];
// URI to a document (on or off-chain) that contains additional information. Optional.
string uri = 7 [(gogoproto.customname) = "URI", (cosmos_proto.field_added_in) = "cosmos-sdk 0.46"];
// URIHash is a sha256 hash of a document pointed by URI. It's used to verify that
// the document didn't change. Optional.
string uri_hash = 8 [(gogoproto.customname) = "URIHash", (cosmos_proto.field_added_in) = "cosmos-sdk 0.46"];
}
16 changes: 16 additions & 0 deletions x/bank/proto/cosmos/bank/v2/genesis.proto
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cosmos.bank.v2;

import "gogoproto/gogo.proto";
import "cosmos/bank/v2/bank.proto";
import "cosmos/bank/v2/authorityMetadata.proto";
import "amino/amino.proto";
import "cosmos/base/v1beta1/coin.proto";
import "cosmos_proto/cosmos.proto";
Expand All @@ -25,6 +26,11 @@ message GenesisState {
(gogoproto.nullable) = false,
(amino.dont_omitempty) = true
];

// denom_metadata defines the metadata of the different coins.
repeated Metadata denom_metadata = 4 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true];

repeated GenesisDenom factory_denoms = 5 [(gogoproto.nullable) = false];
}

// Balance defines an account address and balance pair used in the bank module's
Expand All @@ -43,4 +49,14 @@ message Balance {
(gogoproto.nullable) = false,
(amino.dont_omitempty) = true
];
}

// GenesisDenom defines a tokenfactory denom that is defined within genesis
// state. The structure contains DenomAuthorityMetadata which defines the
// denom's admin.
message GenesisDenom {
option (gogoproto.equal) = true;

string denom = 1;
DenomAuthorityMetadata authority_metadata = 2 [(gogoproto.nullable) = false];
}
27 changes: 27 additions & 0 deletions x/bank/proto/cosmos/bank/v2/tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,33 @@ message MsgSend {
// MsgSendResponse defines the response structure for executing a MsgSend message.
message MsgSendResponse {}

// 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;
}
Comment on lines +47 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation annotations to the 'subdenom' field.

To ensure proper validation and serialization, consider adding the (cosmos_proto.scalar) = "cosmos.DenomString" annotation to the subdenom field. This enforces expected formats and constraints for sub-denominations.

Comment on lines +47 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add documentation comments for new message fields

To enhance clarity and maintain consistency with existing messages, please add comments describing the sender and subdenom fields in the MsgCreateDenom message.


// MsgCreateDenomResponse defines the response structure for executing a MsgCreateDenom message.
message MsgCreateDenomResponse {
string new_token_denom = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add annotation to 'new_token_denom' field.

Consider adding the (cosmos_proto.scalar) = "cosmos.DenomString" annotation to the new_token_denom field in MsgCreateDenomResponse to ensure consistent serialization and type handling.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Provide a comment for the response field

Please add a documentation comment for the new_token_denom field in MsgCreateDenomResponse to improve readability.

}

// 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;
Comment on lines +66 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation annotation to the 'denom' field.

To ensure proper validation and serialization of the denom field, consider adding the (cosmos_proto.scalar) = "cosmos.DenomString" annotation. This will help enforce expected formats and constraints.

string new_admin = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"];
}
Comment on lines +66 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Document the fields in MsgChangeAdmin message

For better code documentation and consistency, consider adding comments explaining the sender, denom, and new_admin fields in the MsgChangeAdmin message.


// MsgChangeAdminResponse defines the response structure for executing a MsgChangeAdmin message.
message MsgChangeAdminResponse {}

// MsgMint is the Msg/Mint request type.
message MsgMint {
option (cosmos.msg.v1.signer) = "authority";
Expand Down
34 changes: 34 additions & 0 deletions x/bank/v2/keeper/admins.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package keeper

import (
"context"

"cosmossdk.io/x/bank/v2/types"
)

// GetAuthorityMetadata returns the authority metadata for a specific denom
func (k Keeper) GetAuthorityMetadata(ctx context.Context, denom string) (types.DenomAuthorityMetadata, error) {
authority, err := k.denomAuthority.Get(ctx, denom)
return authority, err
}
Comment on lines +10 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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
}
Comment on lines +17 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Wrap validation error with contextual information

To enhance error traceability, consider wrapping the error returned from metadata.Validate() with additional context.

Modify the error handling as follows:

err := metadata.Validate()
if err != nil {
-    return err
+    return fmt.Errorf("validation failed for DenomAuthorityMetadata: %w", err)
}

Remember to import 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.

Suggested change
err := metadata.Validate()
if err != nil {
return err
}
err := metadata.Validate()
if err != nil {
return fmt.Errorf("validation failed for DenomAuthorityMetadata: %w", err)
}


return k.denomAuthority.Set(ctx, denom, metadata)
}
Comment on lines +15 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
// 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)
}


func (k Keeper) setAdmin(ctx context.Context, denom, admin string) error {
metadata, err := k.GetAuthorityMetadata(ctx, denom)
if err != nil {
return err
}
Comment on lines +26 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Provide context when returning error from GetAuthorityMetadata

Adding context to the error can help with debugging when failures occur.

Update the error handling:

if err != nil {
-    return err
+    return fmt.Errorf("failed to get authority metadata for denom %s: %w", denom, err)
}

Ensure that "fmt" is imported.

📝 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.

Suggested change
metadata, err := k.GetAuthorityMetadata(ctx, denom)
if err != nil {
return err
}
metadata, err := k.GetAuthorityMetadata(ctx, denom)
if err != nil {
return fmt.Errorf("failed to get authority metadata for denom %s: %w", denom, err)
}


metadata.Admin = admin

return k.setAuthorityMetadata(ctx, denom, metadata)
}
111 changes: 111 additions & 0 deletions x/bank/v2/keeper/createdenom.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package keeper

import (
"context"
"fmt"

"cosmossdk.io/x/bank/v2/types"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/address"
)

// ConvertToBaseToken converts a fee amount in a whitelisted fee token to the base fee token amount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Incorrect function comment for CreateDenom

The comment on line 13 describes ConvertToBaseToken, but the function below is CreateDenom. This appears to be a copy-paste error. Please update the comment to accurately reflect the purpose of the CreateDenom function.

Apply this diff to correct the comment:

-// ConvertToBaseToken converts a fee amount in a whitelisted fee token to the base fee token amount
+// CreateDenom handles the creation of a new denomination, including validation and fee charging.
📝 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.

Suggested change
// ConvertToBaseToken converts a fee amount in a whitelisted fee token to the base fee token amount
// CreateDenom handles the creation of a new denomination, including validation and fee charging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Update the function comment to match CreateDenom

The comment at line 13 refers to ConvertToBaseToken, but the function below is CreateDenom. Please update the comment to accurately describe the CreateDenom function.

Apply this diff to fix the comment:

-// ConvertToBaseToken converts a fee amount in a whitelisted fee token to the base fee token amount
+// CreateDenom creates a new token denomination after validation and fee charges.
📝 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.

Suggested change
// ConvertToBaseToken converts a fee amount in a whitelisted fee token to the base fee token amount
// CreateDenom creates a new token denomination after validation and fee charges.

func (k Keeper) CreateDenom(ctx context.Context, creatorAddr, subdenom string) (newTokenDenom string, err error) {
denom, err := k.validateCreateDenom(ctx, creatorAddr, subdenom)
if err != nil {
return "", err
}

err = k.chargeForCreateDenom(ctx, creatorAddr)
if err != nil {
return "", err
}

err = k.createDenomAfterValidation(ctx, creatorAddr, denom)
return denom, err
}

// Runs CreateDenom logic after the charge and all denom validation has been handled.
// Made into a second function for genesis initialization.
func (k Keeper) createDenomAfterValidation(ctx context.Context, creatorAddr, denom string) (err error) {
_, exists := k.GetDenomMetaData(ctx, denom)
if !exists {
denomMetaData := types.Metadata{
DenomUnits: []*types.DenomUnit{{
Denom: denom,
Exponent: 0,
}},
Base: denom,
Name: denom,
Symbol: denom,
Display: denom,
}

err := k.SetDenomMetaData(ctx, denomMetaData)
if err != nil {
return err
Comment on lines +45 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid shadowing the err variable

The declaration err := at line 45 shadows the named return variable err. This can lead to confusion and potential bugs. Use err = instead of err := to assign to the existing err variable.

Apply this diff to fix the variable shadowing:

-		err := k.SetDenomMetaData(ctx, denomMetaData)
+		err = k.SetDenomMetaData(ctx, denomMetaData)
📝 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.

Suggested change
err := k.SetDenomMetaData(ctx, denomMetaData)
if err != nil {
return err
err = k.SetDenomMetaData(ctx, denomMetaData)
if err != nil {
return err

}
}

authorityMetadata := types.DenomAuthorityMetadata{
Admin: creatorAddr,
}
err = k.setAuthorityMetadata(ctx, denom, authorityMetadata)
if err != nil {
return err
}

// TODO: do we need map creator => denom
// k.addDenomFromCreator(ctx, creatorAddr, denom)
return nil
}
Comment on lines +31 to +62
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid unnecessary named return parameters

In the createDenomAfterValidation function, the named return parameter err is used but isn't necessary, as it doesn't enhance clarity. According to the Uber Go Style Guide, named return parameters should be avoided when they don't provide additional insight.

Apply this diff to remove the named return parameter:

-func (k Keeper) createDenomAfterValidation(ctx context.Context, creatorAddr, denom string) (err error) {
+func (k Keeper) createDenomAfterValidation(ctx context.Context, creatorAddr, denom string) error {

Update the return statements accordingly:

-			return err
+			return err

Note: Ensure all return statements remain consistent after the change.

📝 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.

Suggested change
func (k Keeper) createDenomAfterValidation(ctx context.Context, creatorAddr, denom string) (err error) {
_, exists := k.GetDenomMetaData(ctx, denom)
if !exists {
denomMetaData := types.Metadata{
DenomUnits: []*types.DenomUnit{{
Denom: denom,
Exponent: 0,
}},
Base: denom,
Name: denom,
Symbol: denom,
Display: denom,
}
err := k.SetDenomMetaData(ctx, denomMetaData)
if err != nil {
return err
}
}
authorityMetadata := types.DenomAuthorityMetadata{
Admin: creatorAddr,
}
err = k.setAuthorityMetadata(ctx, denom, authorityMetadata)
if err != nil {
return err
}
// TODO: do we need map creator => denom
// k.addDenomFromCreator(ctx, creatorAddr, denom)
return nil
}
func (k Keeper) createDenomAfterValidation(ctx context.Context, creatorAddr, denom string) error {
_, exists := k.GetDenomMetaData(ctx, denom)
if !exists {
denomMetaData := types.Metadata{
DenomUnits: []*types.DenomUnit{{
Denom: denom,
Exponent: 0,
}},
Base: denom,
Name: denom,
Symbol: denom,
Display: denom,
}
err := k.SetDenomMetaData(ctx, denomMetaData)
if err != nil {
return err
}
}
authorityMetadata := types.DenomAuthorityMetadata{
Admin: creatorAddr,
}
err := k.setAuthorityMetadata(ctx, denom, authorityMetadata)
if err != nil {
return err
}
// TODO: do we need map creator => denom
// k.addDenomFromCreator(ctx, creatorAddr, denom)
return nil
}


func (k Keeper) validateCreateDenom(ctx context.Context, creatorAddr, subdenom string) (newTokenDenom string, err error) {
// Temporary check until IBC bug is sorted out
if k.HasSupply(ctx, subdenom) {
return "", fmt.Errorf("temporary error until IBC bug is sorted out, " +
"can't create subdenoms that are the same as a native denom")
}
Comment on lines +67 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Simplify and clarify the error message

The error message includes phrases like "temporary error until IBC bug is sorted out," which may not be appropriate for end-users. Consider rephrasing the error message to be concise and user-friendly.

Apply this diff to improve the error message:

-return "", fmt.Errorf("temporary error until IBC bug is sorted out, " +
-	"can't create subdenoms that are the same as a native denom")
+return "", fmt.Errorf("cannot create a subdenom that matches an existing native denomination")
📝 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.

Suggested change
return "", fmt.Errorf("temporary error until IBC bug is sorted out, " +
"can't create subdenoms that are the same as a native denom")
}
return "", fmt.Errorf("cannot create a subdenom that matches an existing native denomination")
}


denom, err := types.GetTokenDenom(creatorAddr, subdenom)
if err != nil {
return "", err
}

_, found := k.GetDenomMetaData(ctx, denom)
if found {
return "", types.ErrDenomExists
}

return denom, nil
}

Comment on lines +64 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid unnecessary named return parameters

In the validateCreateDenom function, the named return parameters newTokenDenom and err are used but may not add clarity. It's advisable to omit them when they're not required for understanding.

Apply this diff to simplify the function signature:

-func (k Keeper) validateCreateDenom(ctx context.Context, creatorAddr, subdenom string) (newTokenDenom string, err error) {
+func (k Keeper) validateCreateDenom(ctx context.Context, creatorAddr, subdenom string) (string, error) {

Ensure all return statements are updated accordingly to match the new signature.

📝 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.

Suggested change
func (k Keeper) validateCreateDenom(ctx context.Context, creatorAddr, subdenom string) (newTokenDenom string, err error) {
// Temporary check until IBC bug is sorted out
if k.HasSupply(ctx, subdenom) {
return "", fmt.Errorf("temporary error until IBC bug is sorted out, " +
"can't create subdenoms that are the same as a native denom")
}
denom, err := types.GetTokenDenom(creatorAddr, subdenom)
if err != nil {
return "", err
}
_, found := k.GetDenomMetaData(ctx, denom)
if found {
return "", types.ErrDenomExists
}
return denom, nil
}
func (k Keeper) validateCreateDenom(ctx context.Context, creatorAddr, subdenom string) (string, error) {
// Temporary check until IBC bug is sorted out
if k.HasSupply(ctx, subdenom) {
return "", fmt.Errorf("temporary error until IBC bug is sorted out, " +
"can't create subdenoms that are the same as a native denom")
}
denom, err := types.GetTokenDenom(creatorAddr, subdenom)
if err != nil {
return "", err
}
_, found := k.GetDenomMetaData(ctx, denom)
if found {
return "", types.ErrDenomExists
}
return denom, nil
}

func (k Keeper) chargeForCreateDenom(ctx context.Context, creatorAddr string) (err error) {
params := k.GetParams(ctx)

// if DenomCreationFee is non-zero, transfer the tokens from the creator
// account to community pool
if params.DenomCreationFee != nil {
accAddr, err := sdk.AccAddressFromBech32(creatorAddr)
if err != nil {
return err
}

communityPoolAddr := address.Module("protocolpool")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Incorrect Module Name for Community Pool Address

The module name "protocolpool" used at line 95 in x/bank/v2/keeper/createdenom.go does not match the commonly used module name "distribution". Please verify and update the module name accordingly.

  • File: x/bank/v2/keeper/createdenom.go
  • Line: 95
🔗 Analysis chain

Verify the module name for the community pool address

At line 95, the module name "protocolpool" is used to derive the community pool address. Please verify that "protocolpool" is the correct module name. Commonly, the community pool module is "distribution".

Run the following script to check the module names in the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: List all module account names used in the codebase.

rg --type go 'Module\("(\w+)"\)' -o -r '$1' | sort | uniq

Length of output: 417


if err := k.SendCoins(ctx, accAddr, communityPoolAddr, params.DenomCreationFee); err != nil {
return err
}
}

// if DenomCreationGasConsume is non-zero, consume the gas
if params.DenomCreationGasConsume != 0 {
err = k.Environment.GasService.GasMeter(ctx).Consume(params.DenomCreationGasConsume, "consume denom creation gas")
if err != nil {
return err
}
}

return nil
}
Comment on lines +85 to +111
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify function signature by removing named return parameter

The chargeForCreateDenom function uses a named return parameter err which isn't necessary for comprehension. Removing it can enhance readability.

Apply this diff to modify the function:

-func (k Keeper) chargeForCreateDenom(ctx context.Context, creatorAddr string) (err error) {
+func (k Keeper) chargeForCreateDenom(ctx context.Context, creatorAddr string) error {

Adjust any assignments or return statements as needed to reflect this change.

📝 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.

Suggested change
params := k.GetParams(ctx)
// if DenomCreationFee is non-zero, transfer the tokens from the creator
// account to community pool
if params.DenomCreationFee != nil {
accAddr, err := sdk.AccAddressFromBech32(creatorAddr)
if err != nil {
return err
}
communityPoolAddr := address.Module("protocolpool")
if err := k.SendCoins(ctx, accAddr, communityPoolAddr, params.DenomCreationFee); err != nil {
return err
}
}
// if DenomCreationGasConsume is non-zero, consume the gas
if params.DenomCreationGasConsume != 0 {
err = k.Environment.GasService.GasMeter(ctx).Consume(params.DenomCreationGasConsume, "consume denom creation gas")
if err != nil {
return err
}
}
return nil
}
func (k Keeper) chargeForCreateDenom(ctx context.Context, creatorAddr string) error {
params := k.GetParams(ctx)
// if DenomCreationFee is non-zero, transfer the tokens from the creator
// account to community pool
if params.DenomCreationFee != nil {
accAddr, err := sdk.AccAddressFromBech32(creatorAddr)
if err != nil {
return err
}
communityPoolAddr := address.Module("protocolpool")
if err := k.SendCoins(ctx, accAddr, communityPoolAddr, params.DenomCreationFee); err != nil {
return err
}
}
// if DenomCreationGasConsume is non-zero, consume the gas
if params.DenomCreationGasConsume != 0 {
err := k.Environment.GasService.GasMeter(ctx).Consume(params.DenomCreationGasConsume, "consume denom creation gas")
if err != nil {
return err
}
}
return nil
}

28 changes: 28 additions & 0 deletions x/bank/v2/keeper/creators.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package keeper

// import (
// storetypes "cosmossdk.io/store/types"
// sdk "github.com/cosmos/cosmos-sdk/types"
// )

// func (k Keeper) addDenomFromCreator(ctx sdk.Context, creator, denom string) {
// store := k.GetCreatorPrefixStore(ctx, creator)
// store.Set([]byte(denom), []byte(denom))
// }

// func (k Keeper) getDenomsFromCreator(ctx sdk.Context, creator string) []string {
// store := k.GetCreatorPrefixStore(ctx, creator)

// iterator := store.Iterator(nil, nil)
// defer iterator.Close()

// denoms := []string{}
// for ; iterator.Valid(); iterator.Next() {
// denoms = append(denoms, string(iterator.Key()))
// }
// return denoms
// }

// func (k Keeper) GetAllDenomsIterator(ctx sdk.Context) storetypes.Iterator {
// return k.GetCreatorsPrefixStore(ctx).Iterator(nil, nil)
// }
22 changes: 22 additions & 0 deletions x/bank/v2/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,28 @@ func (k *Keeper) InitGenesis(ctx context.Context, state *types.GenesisState) err
k.setSupply(ctx, supply)
}

for _, meta := range state.DenomMetadata {
err := k.SetDenomMetaData(ctx, meta)
if err != nil {
return err
}
}
Comment on lines +47 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add context to error handling when setting denomination metadata

Currently, if k.SetDenomMetaData(ctx, meta) returns an error, it's returned directly without additional context. Providing more detailed error messages improves debuggability.

Consider modifying the error handling as follows:

 for _, meta := range state.DenomMetadata {
     err := k.SetDenomMetaData(ctx, meta)
     if err != nil {
-        return err
+        return fmt.Errorf("failed to set metadata for denom %s: %w", meta.Base, 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.

Suggested change
for _, meta := range state.DenomMetadata {
err := k.SetDenomMetaData(ctx, meta)
if err != nil {
return err
}
}
for _, meta := range state.DenomMetadata {
err := k.SetDenomMetaData(ctx, meta)
if err != nil {
return fmt.Errorf("failed to set metadata for denom %s: %w", meta.Base, err)
}
}


for _, genDenom := range state.GetFactoryDenoms() {
creator, _, err := types.DeconstructDenom(genDenom.GetDenom())
if err != nil {
return err
}
err = k.createDenomAfterValidation(ctx, creator, genDenom.GetDenom())
if err != nil {
return err
}
err = k.setAuthorityMetadata(ctx, genDenom.GetDenom(), genDenom.GetAuthorityMetadata())
if err != nil {
return err
}
}
Comment on lines +54 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Error handling and validation in factory denom initialization

The code introduces initialization for factory denoms by iterating over state.GetFactoryDenoms(). While it correctly deconstructs the denom and calls createDenomAfterValidation, consider the following improvements:

  1. Consolidate Error Messages: The error messages returned may lack context about which denom caused the error. Enhance the error messages to include the denom information for better debugging.

  2. Validate Authority Metadata: Before setting the authority metadata with k.setAuthorityMetadata, ensure that the genDenom.GetAuthorityMetadata() is valid and not nil to prevent potential nil pointer dereferences.

Apply this diff to improve error messages:

 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)
     }
 }
📝 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.

Suggested change
for _, genDenom := range state.GetFactoryDenoms() {
creator, _, err := types.DeconstructDenom(genDenom.GetDenom())
if err != nil {
return err
}
err = k.createDenomAfterValidation(ctx, creator, genDenom.GetDenom())
if err != nil {
return err
}
err = k.setAuthorityMetadata(ctx, genDenom.GetDenom(), genDenom.GetAuthorityMetadata())
if err != nil {
return err
}
}
for _, genDenom := range state.GetFactoryDenoms() {
creator, _, err := types.DeconstructDenom(genDenom.GetDenom())
if err != nil {
return fmt.Errorf("failed to deconstruct denom %s: %w", genDenom.GetDenom(), err)
}
err = k.createDenomAfterValidation(ctx, creator, genDenom.GetDenom())
if err != nil {
return fmt.Errorf("failed to create denom %s: %w", genDenom.GetDenom(), err)
}
err = k.setAuthorityMetadata(ctx, genDenom.GetDenom(), genDenom.GetAuthorityMetadata())
if err != nil {
return fmt.Errorf("failed to set authority metadata for denom %s: %w", genDenom.GetDenom(), err)
}
}

💡 Codebase verification

Issues Found: Instances of createDenomAfterValidation are present outside of InitGenesis, indicating inconsistent denom creation across the codebase.

🔗 Analysis chain

Verify consistency of denom creation across the codebase

Ensure that all usages and initializations of factory denoms are consistent with the new initialization logic introduced here. There might be other places in the codebase where denoms are created or initialized that need to be updated.

Run the following script to search for other instances of denom creation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find instances where denoms are created or initialized outside InitGenesis

# Search for calls to createDenomAfterValidation excluding InitGenesis method
rg --type go 'createDenomAfterValidation\(' -g '!x/bank/v2/keeper/genesis.go' -A 5 -B 5

Length of output: 1203


return nil
}

Expand Down
Loading
Loading