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): Send function #21606

Merged
merged 23 commits into from
Sep 12, 2024
Merged

feat(bank/v2): Send function #21606

merged 23 commits into from
Sep 12, 2024

Conversation

hieuvubk
Copy link
Contributor

@hieuvubk hieuvubk commented Sep 9, 2024

Description

Closes: #21596

  • Add account keeper
  • Bring SendCoins & MintCoins to v2
  • Add mock & some test helper
  • Test basic Send funcs

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

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

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

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Introduced authentication capabilities for coin minting and transfers.
    • Added event types for tracking coin transactions, including transfers and minting.
    • Implemented a vesting account interface for managing locked and delegated coins.
    • Enhanced the bank module with robust methods for minting and transferring coins.
  • Bug Fixes

    • Enhanced error handling and validation checks for financial operations.
  • Tests

    • Added a comprehensive test suite for the bank module's keeper functionalities, focusing on coin transfer scenarios.
  • Documentation

    • Updated documentation to reflect new features and functionalities in the bank module.

Copy link
Contributor

coderabbitai bot commented Sep 9, 2024

Walkthrough

Walkthrough

The pull request enhances the Cosmos SDK's bank module by integrating an AuthKeeper for improved authentication and authorization. It introduces new methods for minting and transferring coins, establishes a comprehensive testing suite, and defines event types for various coin transactions. The modifications streamline the bank module's functionality, ensuring robust error handling and event logging for coin-related operations.

Changes

File Path Change Summary
x/bank/v2/depinject.go Added AuthKeeper to ModuleInputs struct; modified ProvideModule to include AuthKeeper in keeper instantiation.
x/bank/v2/keeper/keeper.go Enhanced Keeper struct with AuthKeeper; added methods for minting and transferring coins (MintCoins, SendCoins); introduced utility methods for managing balances and supplies; modified NewKeeper to accept AuthKeeper.
x/bank/v2/keeper/keeper_test.go Introduced a test suite for the bank keeper, validating functionality of coin transfers between accounts and modules with multiple test cases.
x/bank/v2/testutil/helpers.go Introduced FundAccount utility function to simplify funding accounts during tests.
x/bank/v2/types/events.go Defined event types and attributes for bank module transactions, including transfer and minting events.
x/bank/v2/types/expected_keepers.go Added AuthKeeper interface outlining methods for account management within the bank module.
x/bank/v2/types/keys.go Introduced constants and variables for data storage optimization, including StoreKey and BalanceValueCodec.
x/bank/v2/types/vesting.go Defined VestingAccount interface for managing vesting accounts, including methods for locked and delegated coins.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant BankKeeper
    participant AuthKeeper

    User->>BankKeeper: MintCoins(amounts)
    BankKeeper->>AuthKeeper: GetModuleAccount(moduleName)
    AuthKeeper-->>BankKeeper: Return module account
    BankKeeper->>BankKeeper: Validate and Mint Coins
    BankKeeper-->>User: Return success
Loading
sequenceDiagram
    participant User
    participant BankKeeper
    participant AuthKeeper

    User->>BankKeeper: SendCoins(from, to, amounts)
    BankKeeper->>AuthKeeper: GetAccount(from)
    AuthKeeper-->>BankKeeper: Return sender account
    BankKeeper->>AuthKeeper: GetAccount(to)
    AuthKeeper-->>BankKeeper: Return receiver account
    BankKeeper->>BankKeeper: Validate and Transfer Coins
    BankKeeper-->>User: Return success
Loading

Possibly related PRs

  • fix(x/bank): Better handling of negative spendable balances #21407: The changes in this PR enhance the handling of spendable balances, which is directly related to the modifications made in the Keeper struct and its methods in the main PR, particularly regarding balance management and the introduction of the AuthKeeper.
  • refactor(x/bank): Simplify the return #21602: This PR simplifies return statements in the DelegateCoins and UndelegateCoins methods, which are part of the Keeper struct in the bank module. The changes in the main PR also involve modifications to the Keeper struct, indicating a focus on improving the code structure and readability in related areas.
  • fix(x/accounts/lockup): prevent double withdraw #21619: This PR enhances the withdrawal process in the lockup functionality of the accounts module, which is related to the bank module's handling of coin transactions. The changes in the main PR regarding the Keeper struct and its methods for managing balances are relevant to the overall transaction handling in the ecosystem.

Suggested labels

C:x/accounts, C:x/accounts/multisig, backport/v0.52.x


Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c4c284b and fc75c03.

Files selected for processing (1)
  • x/bank/v2/keeper/keeper_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • x/bank/v2/keeper/keeper_test.go

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

x/bank/v2/keeper/keeper.go Show resolved Hide resolved
x/bank/v2/depinject.go Outdated Show resolved Hide resolved
x/bank/v2/keeper/keeper.go Outdated Show resolved Hide resolved
@hieuvubk hieuvubk marked this pull request as ready for review September 9, 2024 13:44
Copy link
Contributor

github-actions bot commented Sep 9, 2024

@hieuvubk your pull request is missing a changelog!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Outside diff range, codebase verification and nitpick comments (1)
x/bank/v2/keeper/keeper.go (1)

36-46: Constructor modifications are appropriate for the new functionality.

The addition of AuthKeeper is essential for integrating authorization checks. Consider addressing the TODO comment about whether to add addressCodec to the environment for clarity.

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ac53e33 and 660e601.

Files selected for processing (9)
  • x/bank/v2/depinject.go (2 hunks)
  • x/bank/v2/keeper/keeper.go (2 hunks)
  • x/bank/v2/keeper/keeper_test.go (1 hunks)
  • x/bank/v2/testutil/expected_keepers_mocks.go (1 hunks)
  • x/bank/v2/testutil/helpers.go (1 hunks)
  • x/bank/v2/types/events.go (1 hunks)
  • x/bank/v2/types/expected_keepers.go (1 hunks)
  • x/bank/v2/types/keys.go (1 hunks)
  • x/bank/v2/types/vesting.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • x/bank/v2/testutil/expected_keepers_mocks.go
Additional context used
Path-based instructions (8)
x/bank/v2/types/expected_keepers.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/bank/v2/testutil/helpers.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/bank/v2/types/events.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/bank/v2/types/vesting.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/bank/v2/depinject.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/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/keeper.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (10)
x/bank/v2/types/expected_keepers.go (1)

9-15: Interface AuthKeeper is well-defined and adheres to Go conventions.

The AuthKeeper interface is clearly defined, with method signatures that are appropriate for the operations they are intended to perform. The use of context.Context and specific types from the Cosmos SDK (sdk.AccAddress, sdk.AccountI, sdk.ModuleAccountI) enhances the clarity and effectiveness of the interface.

x/bank/v2/types/events.go (1)

7-24: Event type and attribute constants are well-defined and appropriately documented.

The constants defined in this file are clear and follow Go naming conventions. The note regarding the choice of EventTypeCoinMint to avoid clashes with the mint module is particularly insightful, demonstrating careful consideration of the broader module ecosystem.

x/bank/v2/types/vesting.go (1)

9-32: Well-defined interface for vesting accounts.

The VestingAccount interface is clearly defined, with methods that cover essential functionalities required for managing vesting accounts. The use of sdk.Coins and time.Time is appropriate, ensuring that the vesting logic can be accurately implemented based on the blockchain's time.

x/bank/v2/depinject.go (2)

35-35: Appropriate addition of AuthKeeper to ModuleInputs.

The inclusion of AuthKeeper in the ModuleInputs struct is a well-considered enhancement that aligns with the module's need to handle authentication-related functionalities. The field is correctly placed and follows good practices for dependency injection.


57-57: Correct integration of AuthKeeper in ProvideModule.

The modification to include in.AuthKeeper in the keeper.NewKeeper function call within ProvideModule is correctly implemented. This change effectively integrates the authentication keeper into the module's operations, enhancing its capabilities to handle authentication-related functionalities.

x/bank/v2/types/keys.go (1)

15-49: Well-implemented constants and variable for enhanced data handling.

The introduction of constants such as StoreKey, MintModuleName, and various prefixes, along with the variable BalanceValueCodec, is well-executed. These additions significantly enhance the module's capability to manage data efficiently and maintain compatibility with previous implementations. The use of collections.NewPrefix and collcodec.NewAltValueCodec is appropriate and aligns with the module's objectives.

x/bank/v2/keeper/keeper_test.go (2)

84-122: Test setup is well-structured and appropriate for the scope of the tests.

The initialization of components and the use of mocking are well-implemented, ensuring that the tests are isolated and focused on the functionality being tested.


184-213: Module-to-module transfer tests are correctly implemented and comprehensive.

The test case effectively checks both error conditions and successful scenarios, ensuring that the functionality behaves as expected.

x/bank/v2/keeper/keeper.go (2)

58-100: MintCoins method implementation is robust and meets functional requirements.

The method includes comprehensive checks and validations, ensuring that coins are minted securely and appropriately. The error handling and event emission are well-implemented.


102-160: SendCoins method is correctly implemented and aligns with module requirements.

The method effectively handles different scenarios for coin transfers, including validation checks and error handling. The event logging is a crucial aspect for tracking transactions.

Comment on lines 12 to 21
// FundAccount is a utility function that funds an account by minting and
// sending the coins to the address. This should be used for testing purposes
// only!
func FundAccount(ctx context.Context, bankKeeper bankkeeper.Keeper, addr string, amounts sdk.Coins) error {
if err := bankKeeper.MintCoins(ctx, types.MintModuleName, amounts); err != nil {
return err
}

return bankKeeper.SendCoins(ctx, types.MintModuleName, addr, amounts)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Function FundAccount is well-implemented for testing purposes.

The FundAccount function is appropriately documented and implemented for testing purposes. It handles errors effectively and uses the context.Context properly. Consider adding more detailed error messages or logging within the function to aid in debugging during test failures.

Consider enhancing error handling and logging:

-	if err := bankKeeper.MintCoins(ctx, types.MintModuleName, amounts); err != nil {
-		return err
+	if err := bankKeeper.MintCoins(ctx, types.MintModuleName, amounts); err != nil {
+		return fmt.Errorf("failed to mint coins: %w", err)
+	}
+
+	if err := bankKeeper.SendCoins(ctx, types.MintModuleName, addr, amounts); err != nil {
+		return fmt.Errorf("failed to send coins: %w", 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
// FundAccount is a utility function that funds an account by minting and
// sending the coins to the address. This should be used for testing purposes
// only!
func FundAccount(ctx context.Context, bankKeeper bankkeeper.Keeper, addr string, amounts sdk.Coins) error {
if err := bankKeeper.MintCoins(ctx, types.MintModuleName, amounts); err != nil {
return err
}
return bankKeeper.SendCoins(ctx, types.MintModuleName, addr, amounts)
}
// FundAccount is a utility function that funds an account by minting and
// sending the coins to the address. This should be used for testing purposes
// only!
func FundAccount(ctx context.Context, bankKeeper bankkeeper.Keeper, addr string, amounts sdk.Coins) error {
if err := bankKeeper.MintCoins(ctx, types.MintModuleName, amounts); err != nil {
return fmt.Errorf("failed to mint coins: %w", err)
}
if err := bankKeeper.SendCoins(ctx, types.MintModuleName, addr, amounts); err != nil {
return fmt.Errorf("failed to send coins: %w", err)
}
return nil
}

Comment on lines 124 to 151
func (suite *KeeperTestSuite) TestSendCoins_Acount_To_Account() {
ctx := suite.ctx
require := suite.Require()
balances := sdk.NewCoins(newFooCoin(100), newBarCoin(50))
sendAmt := sdk.NewCoins(newFooCoin(10), newBarCoin(10))

acc0Str, _ := suite.addressCodec.BytesToString(accAddrs[0])
acc1Str, _ := suite.addressCodec.BytesToString(accAddrs[1])

// Try send with empty balances
err := suite.bankKeeper.SendCoins(ctx, acc0Str, acc1Str, sendAmt)
require.Error(err)

// Set balances for acc0 and then try send to acc1
suite.mockFundAccount(accAddrs[0])
require.NoError(banktestutil.FundAccount(ctx, suite.bankKeeper, acc0Str, balances))
require.NoError(suite.bankKeeper.SendCoins(ctx, acc0Str, acc1Str, sendAmt))

// Check balances
acc0FooBalance := suite.bankKeeper.GetBalance(ctx, accAddrs[0], fooDenom)
require.Equal(acc0FooBalance.Amount, math.NewInt(90))
acc0BarBalance := suite.bankKeeper.GetBalance(ctx, accAddrs[0], barDenom)
require.Equal(acc0BarBalance.Amount, math.NewInt(40))
acc1FooBalance := suite.bankKeeper.GetBalance(ctx, accAddrs[1], fooDenom)
require.Equal(acc1FooBalance.Amount, math.NewInt(10))
acc1BarBalance := suite.bankKeeper.GetBalance(ctx, accAddrs[1], barDenom)
require.Equal(acc1BarBalance.Amount, math.NewInt(10))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider expanding test coverage for edge cases in coin transfers.

While the existing tests cover basic scenarios, adding tests for edge cases such as sending coins to the same account or sending zero amounts would enhance the robustness of the test suite.

// Function take sender & receipient as string.
// They can be sdk address or module name.
// An error is returned upon failure.
func (k Keeper) SendCoins(ctx context.Context, from, to string, amt sdk.Coins) error {
Copy link
Member

Choose a reason for hiding this comment

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

can we use []byte?

var err error

// Detect from & to is address format or module name
fromAddr, err = sdk.AccAddressFromBech32(from)
Copy link
Member

Choose a reason for hiding this comment

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

let's use the address codec

}
}

toAddr, err = sdk.AccAddressFromBech32(to)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (2)
x/bank/v2/keeper/keeper.go (2)

59-99: LGTM with a minor suggestion!

The MintCoins function is well-implemented with proper checks and error handling. It ensures that only authorized module accounts can mint coins and that the amounts are valid. The function correctly updates the supply and emits an event for tracking the minting operation.

Consider adding a log statement at the debug level to log the module account address along with the amount minted. This can be helpful for debugging purposes.

+	k.Logger.Debug("minted coins from module account", "amount", amounts.String(), "from", moduleName, "address", addrStr)

105-151: LGTM with a minor suggestion!

The SendCoins function is well-implemented with proper checks and error handling. It correctly handles both regular addresses and module account addresses. The function ensures that the transfer amounts are valid and that the sender has sufficient funds. It emits an event for tracking the transfer operation.

Consider adding a log statement at the debug level to log the sender and recipient addresses along with the transferred amount. This can be helpful for debugging purposes.

+	k.Logger.Debug("transferred coins", "amount", amt.String(), "from", fromAddrString, "to", toAddrString)
Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 660e601 and 392207f.

Files selected for processing (3)
  • x/bank/v2/keeper/keeper.go (2 hunks)
  • x/bank/v2/keeper/keeper_test.go (1 hunks)
  • x/bank/v2/testutil/helpers.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • x/bank/v2/keeper/keeper_test.go
  • x/bank/v2/testutil/helpers.go
Additional context used
Path-based instructions (1)
x/bank/v2/keeper/keeper.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (6)
x/bank/v2/keeper/keeper.go (6)

35-35: LGTM!

The changes to the NewKeeper function are approved. The addition of the ak parameter and its initialization in the Keeper struct are necessary for the keeper to interact with module accounts for authorization checks.


154-160: LGTM!

The GetSupply function is straightforward and correctly retrieves the supply for a given denomination from the store. It handles the case when the supply is not found by returning a zero coin.


164-170: LGTM!

The GetBalance function is straightforward and correctly retrieves the balance of a specific denomination for a given account address. It handles the case when the balance is not found by returning a zero coin.


178-212: LGTM!

The subUnlockedCoins function is well-implemented with proper checks and error handling. It correctly retrieves the balance, checks for sufficient funds, and updates the balance. It emits an event for tracking the coin spent operation.


219-240: LGTM!

The addCoins function is straightforward and correctly retrieves the balance, adds the specified amount, and updates the balance. It emits an event for tracking the coin received operation.


243-250: LGTM!

The setSupply function is straightforward and correctly sets the supply for the given coin. It handles the case of zero coins by removing them from the store to maintain bank invariants and IBC requirements.

Copy link
Contributor

@akhilkumarpilli akhilkumarpilli left a comment

Choose a reason for hiding this comment

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

lgtm, please fix failing lint check

}

// SendCoins transfers amt coins from a sending account to a receiving account.
// Function take sender & receipient as string.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Function take sender & receipient as string.
// Function take sender & receipient as []byte.

func (k Keeper) MintCoins(ctx context.Context, moduleName string, amounts sdk.Coins) error {

// TODO: Mint restriction
acc := k.ak.GetModuleAccount(ctx, moduleName)
Copy link
Member

Choose a reason for hiding this comment

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

we are trying to remove auth from being a dependency. Im not sure we want to introduce it again here. The module account system is a legacy design we should not bring into0 bank v2 imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then Mint will take input as []byte. We should check if this address have Minter permission. Marked as TODO.

if !acc.HasPermission(authtypes.Minter) {

x/bank/v2/keeper/keeper.go Outdated Show resolved Hide resolved
x/bank/v2/types/keys.go Outdated Show resolved Hide resolved
sdk "github.com/cosmos/cosmos-sdk/types"
)

// AuthKeeper defines the account contract that must be fulfilled when
Copy link
Member

Choose a reason for hiding this comment

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

Let's indeed not require auth at all

x/bank/v2/keeper/keeper.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Outside diff range and nitpick comments (2)
x/bank/v2/keeper/keeper_test.go (2)

87-110: Add more test cases for edge scenarios.

The current test covers the basic scenarios of sending coins between accounts, which looks good! However, as mentioned in the previous review, consider adding more test cases to cover edge scenarios such as:

  • Sending coins to the same account
  • Sending zero amounts

This will further improve the robustness of the test suite.


1-187: Code follows Uber Golang style guide, but test coverage can be improved.

I've reviewed the entire test file and can confirm that the code follows the Uber Golang style guide. No deviations were found.

The existing tests provide good coverage for the SendCoins function, testing various scenarios of sending coins between regular accounts and module accounts. However, I noticed that there are no tests for the MintCoins function, which is also part of the changes in this pull request.

To improve the test coverage, please consider adding tests for the MintCoins function, covering scenarios such as:

  • Minting coins to a regular account
  • Minting coins to a module account
  • Minting zero amount of coins
  • Minting invalid denominations of coins

This will ensure that all the changes in the pull request have adequate test coverage.

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4f5dece and c4c284b.

Files selected for processing (4)
  • x/bank/v2/keeper/keeper.go (4 hunks)
  • x/bank/v2/keeper/keeper_test.go (1 hunks)
  • x/bank/v2/testutil/helpers.go (1 hunks)
  • x/bank/v2/types/expected_keepers.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • x/bank/v2/types/expected_keepers.go
Additional context used
Path-based instructions (3)
x/bank/v2/testutil/helpers.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/keeper.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (11)
x/bank/v2/testutil/helpers.go (1)

14-16: The past review comment is still valid:

Function FundAccount is well-implemented for testing purposes.

The FundAccount function is appropriately documented and implemented for testing purposes. It handles errors effectively and uses the context.Context properly.

Consider enhancing error handling and logging as suggested in the past review comment:

-	return bankKeeper.MintCoins(ctx, addr, amounts)
+	if err := bankKeeper.MintCoins(ctx, types.MintModuleName, amounts); err != nil {
+		return fmt.Errorf("failed to mint coins: %w", err)
+	}
+
+	if err := bankKeeper.SendCoins(ctx, types.MintModuleName, addr, amounts); err != nil {
+		return fmt.Errorf("failed to send coins: %w", err)
+	}
+
+	return nil
x/bank/v2/keeper/keeper_test.go (3)

112-135: LGTM!

The test covers the necessary scenarios for sending coins from an account to a module account. The assertions look good.


137-161: LGTM!

The test covers the necessary scenarios for sending coins from a module account to a regular account. It correctly checks the failure case when sending from an account with insufficient balance and the success case when sending from an account with sufficient balance. The assertions look good.


163-187: LGTM!

The test covers the necessary scenarios for sending coins from one module account to another. It correctly checks the failure case when sending from a module account with insufficient balance and the success case when sending from a module account with sufficient balance. The assertions look good.

x/bank/v2/keeper/keeper.go (7)

Line range hint 33-52: LGTM!

The NewKeeper function is correctly initializing the new fields balances and supply in the Keeper struct. The usage of []byte for the authority parameter aligns with the past review comment.


126-133: LGTM!

The GetSupply function is correctly retrieving the supply from the store and returning a zero coin if the supply doesn't exist.


135-143: LGTM!

The GetBalance function is correctly retrieving the balance from the store and returning a zero coin if the balance doesn't exist.


145-185: LGTM!

The subUnlockedCoins function is correctly retrieving the balance, checking if the resulting balance is negative, updating the balance, emitting a coin spent event, and returning an error if the resulting balance is negative.


187-213: LGTM!

The addCoins function is correctly retrieving the balance, adding the specified amount to it, updating the balance, and emitting a coin received event.


215-223: LGTM!

The setSupply function is correctly removing the supply if the coin is zero and setting the supply if the coin is non-zero.


225-240: LGTM!

The setBalance function is correctly validating the balance parameter, removing the balance if the coin is zero, setting the balance if the coin is non-zero, and returning an error if the balance parameter is invalid.

Comment on lines +54 to +85
// MintCoins creates new coins from thin air and adds it to the module account.
// An error is returned if the module account does not exist or is unauthorized.
func (k Keeper) MintCoins(ctx context.Context, addr []byte, amounts sdk.Coins) error {
// TODO: Mint restriction & permission

if !amounts.IsValid() {
return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amounts.String())
}

err := k.addCoins(ctx, addr, amounts)
if err != nil {
return err
}

for _, amount := range amounts {
supply := k.GetSupply(ctx, amount.GetDenom())
supply = supply.Add(amount)
k.setSupply(ctx, supply)
}

addrStr, err := k.addressCodec.BytesToString(addr)
if err != nil {
return err
}

// emit mint event
return k.EventService.EventManager(ctx).EmitKV(
types.EventTypeCoinMint,
event.NewAttribute(types.AttributeKeyMinter, addrStr),
event.NewAttribute(sdk.AttributeKeyAmount, amounts.String()),
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Address the TODO comment.

The MintCoins function looks good overall. It correctly validates the amounts parameter, updates the supply, and emits a minting event. However, I wanted to remind you about the TODO comment regarding the missing mint restriction and permission logic.

Do you want me to implement the mint restriction and permission logic or open a GitHub issue to track this task?

Comment on lines +87 to +124
// SendCoins transfers amt coins from a sending account to a receiving account.
// Function take sender & receipient as []byte.
// They can be sdk address or module name.
// An error is returned upon failure.
func (k Keeper) SendCoins(ctx context.Context, from, to []byte, amt sdk.Coins) error {
if !amt.IsValid() {
return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, amt.String())
}

var err error
// TODO: Send restriction

err = k.subUnlockedCoins(ctx, from, amt)
if err != nil {
return err
}

err = k.addCoins(ctx, to, amt)
if err != nil {
return err
}

fromAddrString, err := k.addressCodec.BytesToString(from)
if err != nil {
return err
}
toAddrString, err := k.addressCodec.BytesToString(to)
if err != nil {
return err
}

return k.EventService.EventManager(ctx).EmitKV(
types.EventTypeTransfer,
event.NewAttribute(types.AttributeKeyRecipient, toAddrString),
event.NewAttribute(types.AttributeKeySender, fromAddrString),
event.NewAttribute(sdk.AttributeKeyAmount, amt.String()),
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Address the TODO comment.

The SendCoins function looks good overall. It correctly validates the amt parameter, updates the balances of the sender and recipient accounts, and emits a transfer event. However, I wanted to remind you about the TODO comment regarding the missing send restriction logic.

Do you want me to implement the send restriction logic or open a GitHub issue to track this task?

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create bank/v2 Send functions
4 participants