feat: implement begin blocker logic #3
Conversation
WalkthroughThe changes introduce a series of enhancements across various components of the application. A new test file validates the module's begin blocker logic, while configuration updates include an additional blocker in the SimApp. New methods are added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant FeeCollector
participant Owner
participant BankKeeper
Test->>FeeCollector: Check balance
FeeCollector->>Test: Return balance
alt Balance > 0
FeeCollector->>Owner: Transfer fees
Owner->>BankKeeper: Update balance
else Balance == 0
Test->>FeeCollector: No action taken
end
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
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (15)
utils/mocks/account.go (1)
19-21: LGTM: GetModuleAddress method is correctly implemented.The
GetModuleAddressmethod is a good mock implementation that correctly usesauthtypes.NewModuleAddressto generate the module address. This allows for effective testing of module address generation without a real AccountKeeper.Consider adding error handling for invalid module names, if applicable. For example:
func (k AccountKeeper) GetModuleAddress(moduleName string) sdk.AccAddress { if moduleName == "" { return nil // or handle error as appropriate } return authtypes.NewModuleAddress(moduleName) }This would make the mock implementation more robust and potentially catch issues in tests where an invalid module name is provided.
utils/mocks/bank.go (3)
11-11: Consider using a more descriptive variable name.While
cdcis a common abbreviation for "codec", it might not be immediately clear to all developers. Consider using a more descriptive name likebech32CodecoraddressCodecto improve code readability.-var cdc = codec.NewBech32Codec("noble") +var bech32Codec = codec.NewBech32Codec("noble")
15-17: LGTM: Struct definition is appropriate for a mock.The
BankKeeperstruct with aBalancesmap is well-suited for a mock implementation. However, consider adding a comment to explain the purpose of theBalancesfield and the format of the string keys (e.g., Bech32-encoded addresses).type BankKeeper struct { + // Balances maps Bech32-encoded addresses to their corresponding coin balances Balances map[string]sdk.Coins }
24-26: LGTM: SendCoins stub implementation is acceptable for a mock.The current implementation of
SendCoinsas a stub that always returnsnilis acceptable for a basic mock. However, consider the following suggestions to enhance its usefulness:
- Add a comment explaining that this is a stub implementation.
- If more complex testing scenarios are needed, consider implementing basic logic to update balances or track calls to this method.
Here's an example of how you might enhance this method:
+// SendCoins is a stub implementation that doesn't perform any actual coin transfer. +// For more complex testing scenarios, consider implementing balance updates here. func (k BankKeeper) SendCoins(_ context.Context, from, to sdk.AccAddress, amount sdk.Coins) error { + // Optionally, implement basic logic or tracking here + // For example: + // k.sentCoins = append(k.sentCoins, SendCoinCall{From: from, To: to, Amount: amount}) return nil }x/authority/keeper/abci.go (1)
18-25: Consider combining error checks for owner retrieval and decoding.To simplify the error handling, you could combine the owner retrieval and decoding into a single operation. This would reduce the number of error checks and make the code more concise.
Here's a suggested improvement:
- owner, err := k.Owner.Get(ctx) - if err != nil { - return errors.Wrap(err, "failed to get owner from state") - } - address, err := k.accountKeeper.AddressCodec().StringToBytes(owner) - if err != nil { - return errors.Wrap(err, "failed to decode owner address") - } + address, err := k.GetOwnerAddress(ctx) + if err != nil { + return errors.Wrap(err, "failed to get and decode owner address") + }This assumes you create a new method
GetOwnerAddressin theKeeperstruct that combines these operations.e2e/fee_test.go (2)
13-13: Consider using a constant instead of a variable for COLLECTOR.While using a global variable for a constant address is acceptable, it's generally better to use constants for such values in Go. This improves code readability and prevents accidental modifications.
Consider changing the declaration to:
-var COLLECTOR = "noble17xpfvakm2amg962yls6f84z3kell8c5lc6kgnn" +const COLLECTOR = "noble17xpfvakm2amg962yls6f84z3kell8c5lc6kgnn"
15-21: LGTM: Test setup looks good, with a minor suggestion.The test function is well-named and set up to run in parallel, which is good for efficiency. The use of a wrapper for the testing environment is a good practice.
Consider adding a comment explaining why the first validator is selected, or if it matters which validator is used. This could prevent potential issues if the order of validators becomes significant in future scenarios.
x/authority/keeper/keeper.go (2)
25-25: LGTM! Consider adding documentation for the new field.The addition of the
bankKeeperfield to theKeeperstruct is appropriate and aligns with the expected changes. This suggests that theKeepernow interacts with banking functionalities, which is a logical extension of its responsibilities.Consider adding a brief comment above the
bankKeeperfield to describe its purpose and any important details about its usage within theKeeperstruct.
35-35: LGTM! Consider grouping related keeper parameters.The update to the
NewKeeperfunction correctly incorporates the newbankKeeperparameter and assigns it to theKeeperstruct. This change is consistent with the addition of thebankKeeperfield and follows the existing pattern for dependency injection.Consider grouping related keeper parameters together in the function signature for better readability. For example, you could place
accountKeeperandbankKeepernext to each other:func NewKeeper( cdc codec.Codec, logger log.Logger, storeService store.KVStoreService, eventService event.Service, router baseapp.MessageRouter, accountKeeper types.AccountKeeper, bankKeeper types.BankKeeper, ) *Keeper { // ... (rest of the function remains the same) }This grouping can make it easier to see related dependencies at a glance.
Also applies to: 50-50
x/authority/keeper/abci_test.go (4)
12-17: LGTM: Test function declaration and initial setup are well-structured.The test function name
TestBeginBlockclearly indicates its purpose, and the setup creates necessary mocks and keepers for testing.Consider adding a brief comment explaining the purpose of the
AuthorityKeeperWithBankfunction for better clarity. For example:// Create a new AuthorityKeeper with a mocked BankKeeper for testing keeper, ctx := mocks.AuthorityKeeperWithBank(t, bank)
18-21: LGTM: First test case correctly checks BeginBlock with empty fee collector.The test case appropriately covers the scenario where there's no balance in the fee collector and correctly asserts that the operation should succeed.
Consider adding a check for the state after
BeginBlockis called to ensure no unexpected changes occurred. For example:// Check that no unexpected state changes occurred require.Empty(t, bank.Balances)
23-39: LGTM: Test cases cover important scenarios and error conditions.The test cases appropriately cover scenarios with a fee collector balance, no owner set, and an invalid owner set. The use of
ErrorContainsfor error checking is a good practice for partial string matching.Consider extracting the error messages into constants at the package level. This would make the error messages easier to maintain and reuse. For example:
const ( ErrNoOwnerSet = "failed to get owner from state" ErrInvalidOwner = "failed to decode owner address" ) // Then in the test: require.ErrorContains(t, err, ErrNoOwnerSet) require.ErrorContains(t, err, ErrInvalidOwner)
41-49: LGTM: Final test case correctly checks BeginBlock with a valid owner set.The test case appropriately covers the happy path scenario with a valid owner set and correctly asserts that the operation should succeed.
Consider adding the following improvements to make the test more robust:
- Check the balance transfer:
// Check that the balance was transferred to the owner ownerBalance := bank.Balances[owner.Address] require.Equal(t, sdk.NewInt64Coin("uusdc", 20_000), ownerBalance.AmountOf("uusdc"))
- Verify that the fee collector balance is now empty:
// Verify that the fee collector balance is now empty feeCollectorBalance := bank.Balances["noble17xpfvakm2amg962yls6f84z3kell8c5lc6kgnn"] require.True(t, feeCollectorBalance.IsZero())These additional checks will ensure that the
BeginBlockfunction is correctly transferring the funds as expected.utils/mocks/authority.go (1)
Line range hint
28-48: LGTM! Consider adding a brief comment explaining the purpose.The new
AuthorityKeeperWithBankfunction is well-implemented and enhances the testability of the module by allowing customBankKeeperimplementations. This is a valuable addition for more flexible and comprehensive testing scenarios.Consider adding a brief comment above the function to explain its purpose and how it differs from
AuthorityKeeper. This would improve the code's self-documentation. For example:// AuthorityKeeperWithBank creates a Keeper instance with a custom BankKeeper for testing purposes. // This allows for more flexible test setups compared to the standard AuthorityKeeper function. func AuthorityKeeperWithBank(t testing.TB, bank types.BankKeeper) (*keeper.Keeper, sdk.Context) { // ... (existing implementation) }x/authority/module.go (1)
100-102: LGTM: BeginBlock method implementationThe
BeginBlockmethod is correctly implemented, delegating the logic to the keeper. This approach maintains a good separation of concerns.Consider adding a comment to explain the purpose of this method and any significant actions performed in the keeper's
BeginBlock. This would improve code readability and maintainability.func (m AppModule) BeginBlock(ctx context.Context) error { + // Execute begin blocker logic in the keeper return m.keeper.BeginBlock(ctx) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- e2e/fee_test.go (1 hunks)
- simapp/app.yaml (1 hunks)
- utils/mocks/account.go (2 hunks)
- utils/mocks/authority.go (2 hunks)
- utils/mocks/bank.go (1 hunks)
- x/authority/keeper/abci.go (1 hunks)
- x/authority/keeper/abci_test.go (1 hunks)
- x/authority/keeper/keeper.go (3 hunks)
- x/authority/keeper/msg_server_test.go (1 hunks)
- x/authority/module.go (4 hunks)
- x/authority/types/expected_keepers.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/authority/keeper/msg_server_test.go
🔇 Additional comments (21)
x/authority/types/expected_keepers.go (4)
3-8: LGTM: New imports are correctly added and necessary.The new imports are correctly formatted and are required for the new methods added to the interfaces. The use of the
sdkalias for the cosmos-sdk types package is a common and accepted practice.
10-13: LGTM: New method enhances AccountKeeper functionality.The addition of
GetModuleAddressmethod to theAccountKeeperinterface is a good enhancement. It allows retrieving a module's address based on its name, which can be useful in various scenarios. The method signature is clear and follows Go naming conventions.
15-18: LGTM: New BankKeeper interface provides essential banking operations.The new
BankKeeperinterface is well-designed and provides crucial banking operations:
GetAllBalances: Retrieves all balances for a given address.SendCoins: Facilitates the transfer of coins between addresses.Both methods use
context.Contextas their first parameter, which is a good practice. The method signatures are clear, follow Go naming conventions, and use appropriate Cosmos SDK types (sdk.AccAddressandsdk.Coins).
1-18: Summary: Excellent additions to support begin blocker logic.The changes in this file provide essential interfaces (
AccountKeeperandBankKeeper) that will likely be used in the implementation of the begin blocker logic. These interfaces offer methods for retrieving module addresses, getting account balances, and transferring coins, which are fundamental operations for many blockchain transactions.The additions are well-structured, follow Go best practices, and use appropriate Cosmos SDK types. These changes lay a solid foundation for implementing the begin blocker logic mentioned in the PR objectives.
utils/mocks/account.go (2)
5-7: LGTM: New imports are correctly added and used.The new imports for
sdkandauthtypesare necessary for the implementation of theGetModuleAddressmethod. They are correctly aliased and utilized in the new code.
Line range hint
1-21: Summary: Good implementation of GetModuleAddress in mock AccountKeeper.The changes to this file effectively extend the mock
AccountKeeperimplementation with theGetModuleAddressmethod. This addition:
- Aligns with the PR objective of implementing begin blocker logic.
- Enhances the mock's capabilities for testing scenarios involving module addresses.
- Maintains consistency with the expected
AccountKeeperinterface.The implementation is clean, correct, and will facilitate testing of components that interact with module addresses.
utils/mocks/bank.go (2)
1-9: LGTM: Package declaration and imports are appropriate.The package name
mocksis suitable for a mock implementation, and the imports are relevant to the functionality being implemented. There are no unused imports.
13-13: LGTM: Proper interface assertion.The interface assertion ensures at compile-time that
BankKeeperimplements thetypes.BankKeeperinterface. This is a good practice to catch any discrepancies early.x/authority/keeper/abci.go (3)
1-8: LGTM: Package declaration and imports are appropriate.The package declaration and imports are correct and relevant for the implemented functionality.
10-11: LGTM: Method signature and comment are well-defined.The
BeginBlockmethod signature is appropriate, and the comment clearly describes its purpose.
12-28: LGTM: BeginBlock implementation is logically sound.The implementation follows a clear and logical flow, with appropriate error handling and early return for optimization.
e2e/fee_test.go (2)
1-11: LGTM: Package declaration and imports are appropriate.The package name
e2eis suitable for end-to-end tests, and the imports are relevant to the test's functionality. There are no unused imports.
23-39: LGTM: Test logic is comprehensive, but consider adding an explicit wait.The test covers the main functionality well, checking both the transfer and fee collection aspects. The balance checks are thorough, and error handling is appropriate.
However, it's not clear if the test explicitly waits for the begin blocker to run. Consider adding a wait or a method to trigger the begin blocker explicitly to ensure the fee transfer has occurred before checking balances.
To verify if there's a method to trigger the begin blocker, let's search for it:
If no such method is found, consider adding one to the test suite for more explicit control over when the begin blocker runs.
✅ Verification successful
Verified: Test explicitly triggers the begin blocker. No additional wait necessary.
The test function
TestBeginBlockerine2e/fee_test.goensures that the begin blocker is invoked during the test execution, addressing the initial concern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for methods related to triggering the begin blocker rg --type go -i 'func.*begin.*block'Length of output: 325
x/authority/keeper/keeper.go (1)
25-25: Summary: Good implementation of bank keeper integrationThe changes to integrate the
bankKeeperinto theKeeperstruct and its constructor are well-implemented. This addition suggests that theauthoritymodule now interacts with banking functionalities, which could be part of implementing the begin blocker logic mentioned in the PR objectives.A few points to consider:
- Ensure that the
types.BankKeeperinterface is properly defined with all necessary methods for the intended functionality.- Update any existing unit tests or add new ones to cover the interactions with the
bankKeeper.- If there are any specific requirements or constraints for using the
bankKeeper, consider documenting them in comments or in the module's documentation.Overall, these changes lay the groundwork for implementing the begin blocker logic. The next steps would likely involve using this
bankKeeperin the actual implementation of the blocker logic.Also applies to: 35-35, 50-50
x/authority/keeper/abci_test.go (1)
1-11: LGTM: Package declaration and imports are appropriate.The package name
keeper_testfollows the convention for test files, and the imports include necessary testing utilities, SDK types, and custom packages required for the test implementation.utils/mocks/authority.go (2)
25-26: LGTM! Good refactoring for improved flexibility.The changes to
AuthorityKeepermaintain backward compatibility while allowing for more flexible testing scenarios. By delegating toAuthorityKeeperWithBankwith a defaultBankKeeper{}, it preserves existing functionality while opening up possibilities for more specific test setups.
Line range hint
25-48: Overall, these changes enhance testability and maintain backward compatibility.The introduction of
AuthorityKeeperWithBankand the modification ofAuthorityKeeperare well-implemented changes that improve the flexibility of testing scenarios. By allowing customBankKeeperimplementations, these changes enable more comprehensive and specific tests without breaking existing functionality.The refactoring maintains backward compatibility while opening up new possibilities for testing. This is a positive step towards more robust and adaptable test infrastructure.
x/authority/module.go (4)
36-36: LGTM: Implementing HasBeginBlocker interfaceThe addition of
appmodule.HasBeginBlockerto the list of implemented interfaces is correct and consistent with the implementation of theBeginBlockmethod later in the file. This enhancement allows the module to perform actions at the beginning of each block, which is a crucial part of the Cosmos SDK module lifecycle.
Line range hint
1-209: Summary: Implementation of begin blocker logic and integration of BankKeeperThe changes in this file successfully implement the begin blocker logic and integrate the
BankKeeperinto the authority module. The modifications are well-structured and follow good practices for Cosmos SDK module development.Key points:
- The
AppModulenow implements theHasBeginBlockerinterface.- A
BeginBlockmethod has been added, delegating to the keeper.- The
ModuleInputsandNewKeeperfunction have been updated to include theBankKeeper.These changes suggest that the module now performs some financial operations at the beginning of each block.
As a final step, please ensure that:
- The
BeginBlocklogic in the keeper is implemented correctly and handles any potential errors.- Any new functionality using the
BankKeeperis properly tested.- The module's documentation is updated to reflect these new capabilities.
Run the following script to verify the implementation of the keeper's
BeginBlockmethod:#!/bin/bash # Description: Verify the keeper's BeginBlock method implementation # Test: Search for the BeginBlock method in the keeper rg --type go -A 10 'func \(k .*Keeper\) BeginBlock\('
204-204: LGTM: Passing BankKeeper to NewKeeperThe addition of
in.BankKeeperas an argument toNewKeeperis consistent with the changes made toModuleInputs. This ensures that theBankKeeperis properly passed to the module's keeper during initialization.To ensure the
NewKeeperfunction is correctly updated, please verify the following:
- The
NewKeeperfunction signature in the keeper package has been updated to accept theBankKeeperparameter.- The
BankKeeperis properly stored and utilized within the keeper struct and its methods.Run the following script to verify the
NewKeeperfunction signature:#!/bin/bash # Description: Verify the NewKeeper function signature # Test: Search for the NewKeeper function definition rg --type go -A 5 'func NewKeeper\('
186-186: LGTM: Addition of BankKeeper to ModuleInputsThe inclusion of
BankKeeperinModuleInputsis appropriate, indicating that the module now requires access to banking functionality. This is consistent with the module's enhanced capabilities, possibly related to fee transfers or other financial operations.To ensure the
BankKeeperis properly integrated, please verify the following:
- The
types.BankKeeperinterface is correctly defined with all necessary methods.- The
BankKeeperis properly initialized and passed to the module in the app's setup.Run the following script to verify the
BankKeeperinterface definition:✅ Verification successful
Verified: BankKeeper Interface Integration Confirmed
The
BankKeeperinterface is properly defined inx/authority/types/expected_keepers.gowith the necessary methodsGetAllBalancesandSendCoins. Its integration intoModuleInputsis appropriate and aligns with the module's banking functionalities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the BankKeeper interface definition # Test: Search for the BankKeeper interface definition rg --type go -A 10 'type BankKeeper interface'Length of output: 456
Script:
#!/bin/bash # Description: Corrected script to verify the BankKeeper interface definition # Search for the BankKeeper interface definition in Go files rg --type go 'type BankKeeper interface'Length of output: 108
Script:
#!/bin/bash # Description: Display the BankKeeper interface definition rg --type go 'type BankKeeper interface' -A 10 x/authority/types/expected_keepers.goLength of output: 270
| func (k BankKeeper) GetAllBalances(_ context.Context, bz sdk.AccAddress) sdk.Coins { | ||
| address, _ := cdc.BytesToString(bz) | ||
| return k.Balances[address] | ||
| } |
There was a problem hiding this comment.
Improve error handling in GetAllBalances method.
The method correctly converts the sdk.AccAddress to a string for map lookup. However, it doesn't handle the case where the address doesn't exist in the Balances map. Consider returning an empty sdk.Coins or handling this case explicitly.
func (k BankKeeper) GetAllBalances(_ context.Context, bz sdk.AccAddress) sdk.Coins {
address, _ := cdc.BytesToString(bz)
- return k.Balances[address]
+ if balance, exists := k.Balances[address]; exists {
+ return balance
+ }
+ return sdk.Coins{}
}📝 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 BankKeeper) GetAllBalances(_ context.Context, bz sdk.AccAddress) sdk.Coins { | |
| address, _ := cdc.BytesToString(bz) | |
| return k.Balances[address] | |
| } | |
| func (k BankKeeper) GetAllBalances(_ context.Context, bz sdk.AccAddress) sdk.Coins { | |
| address, _ := cdc.BytesToString(bz) | |
| if balance, exists := k.Balances[address]; exists { | |
| return balance | |
| } | |
| return sdk.Coins{} | |
| } |
| collector := k.accountKeeper.GetModuleAddress(authtypes.FeeCollectorName) | ||
| balance := k.bankKeeper.GetAllBalances(ctx, collector) |
There was a problem hiding this comment.
💡 Codebase verification
Missing Required Methods in AccountKeeper Interface
The AccountKeeper interface does not implement the required methods GetModuleAddress and AddressCodec as used in the BeginBlock method. Please ensure these methods are correctly defined and implemented within the AccountKeeper.
🔗 Analysis chain
Verify the existence and correctness of external dependencies.
The BeginBlock method relies on external dependencies such as accountKeeper, bankKeeper, and Owner. Ensure these are correctly initialized and implemented in the Keeper struct.
Run the following script to verify the existence and implementation of these dependencies:
Also applies to: 27-27
To thoroughly verify the dependencies, please run the following scripts:
To gather more information about the methods implemented by accountKeeper and bankKeeper, please run the following scripts:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of Keeper dependencies
# Test: Search for the Keeper struct definition and its fields
echo "Searching for Keeper struct definition:"
ast-grep --lang go --pattern $'type Keeper struct {
$$$
accountKeeper $_
bankKeeper $_
Owner $_
$$$
}'
# Test: Search for the implementation of required methods in accountKeeper
echo "Searching for accountKeeper methods:"
ast-grep --lang go --pattern $'type $_ interface {
$$$
GetModuleAddress($_) $_
AddressCodec() $_
$$$
}'
# Test: Search for the implementation of required methods in bankKeeper
echo "Searching for bankKeeper methods:"
ast-grep --lang go --pattern $'type $_ interface {
$$$
GetAllBalances($_, $_) $_
SendCoins($_, $_, $_, $_) $_
$$$
}'
# Test: Search for the implementation of the Owner field
echo "Searching for Owner implementation:"
rg --type go "type Owner"
Length of output: 1000
Script:
#!/bin/bash
# Description: Further verify Keeper struct and dependencies
# Test: Search for the Keeper struct and list its fields
echo "Listing Keeper struct fields:"
ast-grep --lang go --pattern $'type Keeper struct {
$$$
$_: $_
$$$
}' | grep Keeper
# Test: List all methods of accountKeeper
echo "Listing accountKeeper methods:"
ast-grep --lang go --pattern $'func ($_ *$_) GetModuleAddress($_) $_'
# Test: List all methods of bankKeeper
echo "Listing bankKeeper methods:"
ast-grep --lang go --pattern $'func ($_ *$_) GetAllBalances($_, $_) $_'
ast-grep --lang go --pattern $'func ($_ *$_) SendCoins($_, $_, $_, $_) $_'
# Test: Search for the Owner field in Keeper struct
echo "Searching for Owner field in Keeper struct:"
ast-grep --lang go --pattern $'type Keeper struct {
$$$
Owner $_
$$$
}'
Length of output: 1592
Script:
#!/bin/bash
# Description: List all methods of accountKeeper and bankKeeper interfaces
# List all methods of accountKeeper
echo "Listing all methods of accountKeeper interface:"
ast-grep --lang go --pattern $'type AccountKeeper interface {
$$$
$_($_, $_) $_
$$$
}'
# List all methods of bankKeeper
echo "Listing all methods of bankKeeper interface:"
ast-grep --lang go --pattern $'type BankKeeper interface {
$$$
$_($_, $_) $_
$$$
}'
Length of output: 835
| // TestBeginBlocker tests the module's begin blocker logic. | ||
| func TestBeginBlocker(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var wrapper Wrapper | ||
| ctx, _, _ := Suite(t, &wrapper, false) | ||
| validator := wrapper.chain.Validators[0] | ||
|
|
||
| oldBalance, err := wrapper.chain.BankQueryAllBalances(ctx, wrapper.owner.FormattedAddress()) | ||
| require.NoError(t, err) | ||
|
|
||
| err = validator.BankSend(ctx, wrapper.owner.KeyName(), ibc.WalletAmount{ | ||
| Address: wrapper.pendingOwner.FormattedAddress(), | ||
| Denom: "uusdc", | ||
| Amount: math.NewInt(1_000_000), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| balance, err := wrapper.chain.BankQueryAllBalances(ctx, COLLECTOR) | ||
| require.NoError(t, err) | ||
| require.True(t, balance.IsZero()) | ||
|
|
||
| newBalance, err := wrapper.chain.BankQueryAllBalances(ctx, wrapper.owner.FormattedAddress()) | ||
| require.NoError(t, err) | ||
| require.Equal(t, oldBalance.Sub(sdk.NewInt64Coin("uusdc", 1_000_000)), newBalance) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more test cases for comprehensive coverage.
While the current test provides good basic coverage of the begin blocker functionality, consider expanding it to cover more scenarios:
- Test with different transfer amounts to verify if the fee is correctly calculated as a percentage.
- Add edge cases, such as transfers that would result in zero fees.
- Test with multiple transfers in a single block to ensure fees are collected correctly.
- Verify the behavior when the owner has insufficient balance for the fee.
These additional tests would provide more comprehensive coverage of the begin blocker logic.
| package keeper_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| "github.com/noble-assets/authority/utils" | ||
| "github.com/noble-assets/authority/utils/mocks" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestBeginBlock(t *testing.T) { | ||
| bank := mocks.BankKeeper{ | ||
| Balances: make(map[string]sdk.Coins), | ||
| } | ||
| keeper, ctx := mocks.AuthorityKeeperWithBank(t, bank) | ||
|
|
||
| // ACT: Attempt to run begin blocker with empty fee collector. | ||
| err := keeper.BeginBlock(ctx) | ||
| // ASSERT: The action should've succeeded due to empty account. | ||
| require.NoError(t, err) | ||
|
|
||
| // ARRANGE: Give the fee collector some balance. | ||
| bank.Balances["noble17xpfvakm2amg962yls6f84z3kell8c5lc6kgnn"] = sdk.NewCoins( | ||
| sdk.NewInt64Coin("uusdc", 20_000), | ||
| ) | ||
|
|
||
| // ACT: Attempt to run begin blocker with no owner set. | ||
| err = keeper.BeginBlock(ctx) | ||
| // ASSERT: The action should've failed due to no owner set. | ||
| require.ErrorContains(t, err, "failed to get owner from state") | ||
|
|
||
| // ARRANGE: Set an invalid owner in state. | ||
| require.NoError(t, keeper.Owner.Set(ctx, "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn")) | ||
|
|
||
| // ACT: Attempt to run begin blocker with invalid owner set. | ||
| err = keeper.BeginBlock(ctx) | ||
| // ASSERT: The action should've failed due to invalid owner set. | ||
| require.ErrorContains(t, err, "failed to decode owner address") | ||
|
|
||
| // ARRANGE: Generate an owner account and set in state. | ||
| owner := utils.TestAccount() | ||
| require.NoError(t, keeper.Owner.Set(ctx, owner.Address)) | ||
|
|
||
| // ACT: Attempt to run begin blocker. | ||
| err = keeper.BeginBlock(ctx) | ||
| // ASSERT: The action should've succeeded. | ||
| require.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Overall, excellent test coverage with room for minor enhancements.
This test file provides comprehensive coverage for the BeginBlock function, including various scenarios such as empty fee collector, missing owner, invalid owner, and successful execution. The test structure is clear and follows good testing practices.
To further improve the test file:
-
Consider adding subtests using
t.Run()for each scenario. This would improve test organization and allow for more granular test execution. -
Add a test case for when the fee collector has multiple coin types to ensure correct handling of all balances.
-
Consider testing edge cases such as very large balances or zero balances for specific coin types.
Here's an example of how you could structure the tests using subtests:
func TestBeginBlock(t *testing.T) {
t.Run("EmptyFeeCollector", func(t *testing.T) {
// Test with empty fee collector
})
t.Run("NoOwnerSet", func(t *testing.T) {
// Test with no owner set
})
t.Run("InvalidOwner", func(t *testing.T) {
// Test with invalid owner
})
t.Run("ValidOwner", func(t *testing.T) {
// Test with valid owner
})
t.Run("MultipleCoinTypes", func(t *testing.T) {
// New test with multiple coin types
})
}This structure would make the tests more organized and easier to maintain as the complexity grows.
| @@ -4,7 +4,7 @@ modules: | |||
| "@type": cosmos.app.runtime.v1alpha1.Module | |||
| app_name: SimApp | |||
| pre_blockers: [ upgrade ] | |||
| begin_blockers: [ capability, staking, ibc ] | |||
| begin_blockers: [ authority, capability, staking, ibc ] | |||
There was a problem hiding this comment.
💡 Codebase verification
Authority module lacks a BeginBlocker implementation.
The authority module has been added to the begin_blockers list, but no BeginBlocker function was found within the module. This may lead to issues during block processing as the expected blocker logic is missing.
- File:
x/authority/authority.go(expected location forBeginBlocker)
Please implement the BeginBlocker function in the authority module to ensure proper block initialization.
🔗 Analysis chain
LGTM! Verify the authority module's begin blocker implementation.
The addition of the authority module to the begin_blockers list is consistent with the PR objective of implementing begin blocker logic. This change ensures that the authority module's begin blocker runs before other modules, which is likely the intended behavior.
To ensure the correctness of this change, please verify the following:
- The
authoritymodule has a properly implemented begin blocker. - The begin blocker of the
authoritymodule doesn't introduce any breaking changes or unexpected side effects.
Run the following script to check the implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the authority module's begin blocker implementation
# Test: Search for BeginBlocker function in the authority module
rg --type go -A 10 'func.*BeginBlocker' x/authority
Length of output: 53
Script:
#!/bin/bash
# Description: Search for any BeginBlocker-related functions in the authority module using ast-grep
ast-grep --lang go --pattern $'func $_BeginBlocker($_) {$$$}' 'x/authority'
Length of output: 74
Script:
#!/bin/bash
# Description: Search for any functions containing "Blocker" in their names across the entire codebase
rg --type go 'func.*Blocker' .
Length of output: 88
Summary by CodeRabbit
New Features
Bug Fixes
Documentation