Add x/rollup unit tests #161
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MsgServer
participant Keeper
participant MockBankKeeper
User->>MsgServer: Submit L1 Transaction
MsgServer->>Keeper: ApplyL1Txs
Keeper->>MockBankKeeper: Process Transactions
MockBankKeeper-->>Keeper: Return Results
Keeper-->>MsgServer: Transaction Status
MsgServer-->>User: Notify User
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
x/rollup/keeper/deposits.go
Outdated
| coin := sdk.NewCoin(types.ETH, amount) | ||
| if err := k.bankkeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(coin)); err != nil { | ||
| return fmt.Errorf("failed to mint deposit coins to the rollup module: %v", err) | ||
| } | ||
| if err := k.bankkeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, addr, sdk.NewCoins(coin)); err != nil { | ||
| return fmt.Errorf("failed to send deposit coins from rollup module to user account %v: %v", addr, err) | ||
| } | ||
|
|
||
| // TODO: move event emission to msg_server.go |
There was a problem hiding this comment.
Created issue #162
| return types.WrapError(err, "set") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // TODO: include the logic to also store the L1 block info by blockhash in setL1BlockInfo and remove setL1BlockHistory |
There was a problem hiding this comment.
Created issue #163
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
x/rollup/keeper/deposits.go (1)
33-40: Update References to Exported Method NameThe method
setL1BlockHistoryis still referenced with its old name in the following locations:
x/rollup/keeper/msg_server.go: Update the method call to use the new exported name.x/rollup/keeper/deposits.go: Revise comments to reflect the new method name for clarity.Please ensure these references are updated to maintain consistency with the exported method naming convention.
Analysis chain
Ensure Consistency with Exported Method Names.
The method
setL1BlockHistoryis now exported. Verify that all references to this method across the codebase are updated to use the new name.Run the following script to check for references to the old method name:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `setL1BlockHistory` are updated. # Test: Search for the old method name. Expect: No occurrences. rg --type go 'setL1BlockHistory'Length of output: 549
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- Makefile (2 hunks)
- testutils/utils.go (2 hunks)
- x/rollup/keeper/deposits.go (4 hunks)
- x/rollup/keeper/deposits_test.go (1 hunks)
- x/rollup/keeper/keeper.go (1 hunks)
- x/rollup/keeper/keeper_test.go (1 hunks)
- x/rollup/keeper/msg_server.go (3 hunks)
- x/rollup/keeper/msg_server_test.go (1 hunks)
- x/rollup/keeper/withdrawals.go (1 hunks)
- x/rollup/keeper/withdrawals_test.go (1 hunks)
- x/rollup/testutil/expected_keepers_mocks.go (1 hunks)
- x/rollup/types/expected_keepers.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/rollup/keeper/withdrawals.go
Additional comments not posted (20)
x/rollup/keeper/keeper.go (1)
14-14: Verify the impact of changingbankkeepertype.The
bankkeeperfield in theKeeperstruct and thebankKeeperparameter in theNewKeeperfunction have been updated to usetypes.BankKeeper. Ensure that all interactions withbankkeeperin the codebase are compatible with this new interface.Run the following script to verify the usage of
bankkeeperin the codebase:Also applies to: 21-21
Verification successful
Change to
bankkeeperType is CompatibleThe
bankkeeperfield's change totypes.BankKeeperis compatible with the existing codebase. The required methods (BurnCoins,MintCoins,SendCoinsFromAccountToModule, andSendCoinsFromModuleToAccount) are defined in theBankKeeperinterface inx/rollup/types/expected_keepers.go, ensuring compatibility.
x/rollup/types/expected_keepers.go: Defines theBankKeeperinterface with necessary methods.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `bankkeeper` in the codebase to ensure compatibility with `types.BankKeeper`. # Test: Search for `bankkeeper` usage. Expect: Compatibility with `types.BankKeeper`. rg --type go -A 5 'bankkeeper'Length of output: 3441
Script:
#!/bin/bash # Search for the definition of `types.BankKeeper` and its methods to ensure compatibility. ast-grep --lang go --pattern 'interface BankKeeper { $$$ }' --jsonLength of output: 416
Script:
#!/bin/bash # Use ripgrep to search for the definition or references of `BankKeeper` in the codebase. rg 'BankKeeper'Length of output: 3852
x/rollup/types/expected_keepers.go (1)
9-16: LGTM! TheBankKeeperinterface is well-defined.The interface encapsulates necessary methods for bank operations, promoting modularity and ease of testing.
x/rollup/keeper/withdrawals_test.go (1)
11-52: LGTM! Comprehensive test coverage forBurnETH.The tests effectively cover both successful and error scenarios using mocks.
However, ensure that all edge cases are considered in the test scenarios.
Run the following script to verify the coverage of edge cases in the test scenarios:
x/rollup/keeper/keeper_test.go (1)
18-52: LGTM! Well-structured test suite forKeeper.The use of
testify/suiteand mocks provides a robust framework for testing.Ensure that all necessary mock setups are included for comprehensive test coverage.
Run the following script to verify the completeness of mock setups:
Verification successful
Mocks are comprehensively set up across multiple test files. The
mockBurnETHandmockMintETHmethods are utilized in various test scenarios, ensuring robust test coverage for theBankKeeperinterface.
keeper_test.go: UsesmockBurnETHandmockMintETH.withdrawals_test.go: Includes additional mock setups for error scenarios.msg_server_test.go: UtilizesmockBurnETHfor testing message server interactions.deposits_test.go: EmploysmockMintETHfor deposit-related tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify completeness of mock setups in `KeeperTestSuite`. # Test: Search for additional mock setups. Expect: Comprehensive coverage. rg --type go -A 5 'mock'Length of output: 18514
Makefile (1)
54-57: LGTM! New Makefile targets for mock management.The
install-mockgenandgen-mockstargets enhance the build process by supporting mock generation.Ensure these targets are integrated and functioning correctly within the build process.
Run the following script to verify the integration of new Makefile targets:
Also applies to: 66-69
x/rollup/keeper/msg_server.go (3)
32-32: Ensure Consistency with Exported Method Names.The method
ProcessL1SystemDepositTxis now exported. Verify that all references to this method across the codebase are updated to use the new name.Run the following script to check for references to the old method name:
Verification successful
All references to
processL1SystemDepositTxhave been updated.The search did not find any occurrences of the old method name, indicating that all references have been correctly updated to use the new exported name
ProcessL1SystemDepositTx. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `processL1SystemDepositTx` are updated. # Test: Search for the old method name. Expect: No occurrences. rg --type go 'processL1SystemDepositTx'Length of output: 39
76-76: Ensure Consistency with Exported Method Names.The method
BurnETHis now exported. Verify that all references to this method across the codebase are updated to use the new name.Run the following script to check for references to the old method name:
Verification successful
All references to
burnETHhave been updated. The search did not find any occurrences of the old method name, confirming that the codebase is consistent with the exported method nameBurnETH.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `burnETH` are updated. # Test: Search for the old method name. Expect: No occurrences. rg --type go 'burnETH'Length of output: 22
Script:
#!/bin/bash # Re-run the search for the old method name `burnETH` to ensure no occurrences exist. rg 'burnETH' --type go -A 2Length of output: 27
55-55: Ensure Consistency with Exported Method Names.The method
ProcessL1UserDepositTxsis now exported. Verify that all references to this method across the codebase are updated to use the new name.Run the following script to check for references to the old method name:
Verification successful
Method Renaming Verified Successfully
The method
ProcessL1UserDepositTxsis consistently used across the codebase, and no references to the old method nameprocessL1UserDepositTxswere found. The renaming has been correctly applied.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `processL1UserDepositTxs` are updated. # Test: Search for the old method name. Expect: No occurrences. rg --type go 'processL1UserDepositTxs'Length of output: 38
Script:
#!/bin/bash # Description: Verify all references to `ProcessL1UserDepositTxs` are correctly updated. # Search for the new method name to ensure it is used consistently across the codebase. rg --type go 'ProcessL1UserDepositTxs' -A 2Length of output: 1565
x/rollup/testutil/expected_keepers_mocks.go (2)
1-5: Auto-generated Mock File.This file is auto-generated by MockGen and should not be manually edited. Ensure that the mock generation process is correctly integrated into the build or test setup.
15-31: Verify Mock Interface Alignment.Ensure that
MockBankKeepercorrectly implements theBankKeeperinterface. This is crucial for the accuracy of tests relying on these mocks.If there are changes to the
BankKeeperinterface, regenerate the mocks to ensure alignment.testutils/utils.go (2)
68-72: Refactor Improves Modularity.The refactoring of
GenerateEthTxsto useGenerateL1Blockenhances modularity and readability. Ensure thatGenerateL1Blockis correctly implemented and returns the expected block structure.
129-135: Ensure Correctness of Block Generation.The
GenerateL1Blockfunction creates a new block with fixed parameters. Verify that these parameters are appropriate for the tests and adjust if necessary.Check the usage of
GenerateL1Blockin tests to ensure it meets the test requirements.x/rollup/keeper/msg_server_test.go (2)
16-89: Comprehensive Test Coverage forApplyL1Txs.The tests for
ApplyL1Txscover multiple scenarios, including successful and error cases. Ensure that the mock setup and assertions accurately reflect the expected behavior.
91-157: Comprehensive Test Coverage forInitiateWithdrawal.The tests for
InitiateWithdrawalcover various scenarios, including invalid inputs and expected errors. Ensure that the mock expectations are correctly set up and that the assertions validate the expected outcomes.x/rollup/keeper/deposits.go (3)
Line range hint
46-64: Ensure Consistency with Exported Method Names.The method
ProcessL1SystemDepositTxis now exported. Verify that all references to this method across the codebase are updated to use the new name.
Line range hint
65-100: Ensure Consistency with Exported Method Names.The method
ProcessL1UserDepositTxsis now exported. Verify that all references to this method across the codebase are updated to use the new name.
Line range hint
101-120: Ensure Consistency with Exported Method Names.The method
MintETHis now exported. Verify that all references to this method across the codebase are updated to use the new name.Run the following script to check for references to the old method name:
Verification successful
All references to
MintETHare correctly updated.The search results show that the new method name
MintETHis consistently used across the codebase. There are no occurrences of the old method namemintETH, confirming that all references have been updated appropriately.
MintETHis used inx/rollup/keeper/keeper_test.go,x/rollup/keeper/deposits_test.go,x/rollup/keeper/deposits.go, and other relevant files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `mintETH` are updated. # Test: Search for the old method name. Expect: No occurrences. rg --type go 'mintETH'Length of output: 22
Script:
#!/bin/bash # Description: Verify all references to `MintETH` are correctly used. # Test: Search for the new method name. Expect: Occurrences where it is used. rg --type go 'MintETH'Length of output: 1175
x/rollup/keeper/deposits_test.go (3)
15-67: LGTM! VerifyMintETHmethod implementation.The
TestMintETHfunction is well-structured and covers multiple scenarios effectively.Ensure that the
MintETHmethod in therollupKeeperis implemented correctly and handles errors as expected.Run the following script to verify the
MintETHmethod implementation:Verification successful
The
MintETHmethod is implemented correctly and handles errors as expected.The method in
x/rollup/keeper/deposits.goproperly checks for errors during the minting and sending of coins, aligning with the test scenarios inTestMintETH.
- File:
x/rollup/keeper/deposits.go- Method:
MintETHScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `MintETH` method in the `rollupKeeper`. # Test: Search for the `MintETH` method implementation. Expect: Proper error handling and logic. ast-grep --lang go --pattern $'func (k *RollupKeeper) MintETH($_, $_, $_) error { $$$ }'Length of output: 91
Script:
#!/bin/bash # Search for the `MintETH` method in the codebase using ripgrep. rg 'func \(.*\) MintETH\(' --context 5Length of output: 883
69-121: LGTM! VerifyProcessL1SystemDepositTxmethod implementation.The
TestProcessL1SystemDepositTxfunction is comprehensive and covers various scenarios effectively.Ensure that the
ProcessL1SystemDepositTxmethod in therollupKeeperis implemented correctly and handles errors as expected.Run the following script to verify the
ProcessL1SystemDepositTxmethod implementation:Verification successful
Verification Successful:
ProcessL1SystemDepositTxMethod ImplementationThe
ProcessL1SystemDepositTxmethod is implemented inx/rollup/keeper/deposits.goand includes appropriate error handling for transaction unmarshalling, which aligns with the test cases. The implementation appears correct and consistent with the test expectations.
- Location:
x/rollup/keeper/deposits.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `ProcessL1SystemDepositTx` method in the `rollupKeeper`. # Test: Search for the `ProcessL1SystemDepositTx` method implementation. Expect: Proper error handling and logic. ast-grep --lang go --pattern $'func (k *RollupKeeper) ProcessL1SystemDepositTx($_, $_) (*derive.L1BlockInfo, error) { $$$ }'Length of output: 127
Script:
#!/bin/bash # Use ripgrep to search for the `ProcessL1SystemDepositTx` method in the codebase. rg 'ProcessL1SystemDepositTx' --context 5Length of output: 4128
123-194: LGTM! VerifyProcessL1UserDepositTxsmethod implementation.The
TestProcessL1UserDepositTxsfunction is well-structured and covers multiple scenarios effectively.Ensure that the
ProcessL1UserDepositTxsmethod in therollupKeeperis implemented correctly and handles errors as expected.Run the following script to verify the
ProcessL1UserDepositTxsmethod implementation:Verification successful
ProcessL1UserDepositTxsMethod VerifiedThe
ProcessL1UserDepositTxsmethod is implemented inx/rollup/keeper/deposits.goand is referenced appropriately in the codebase. The test cases indeposits_test.goeffectively cover its functionality, including error scenarios.
- Location:
x/rollup/keeper/deposits.go- References:
x/rollup/keeper/msg_server.go,x/rollup/types/errors.goThe method processes L1 user deposit transactions and handles errors as expected. No further issues were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `ProcessL1UserDepositTxs` method in the `rollupKeeper`. # Test: Search for the `ProcessL1UserDepositTxs` method implementation. Expect: Proper error handling and logic. ast-grep --lang go --pattern $'func (k *RollupKeeper) ProcessL1UserDepositTxs($_, $_) error { $$$ }'Length of output: 103
Script:
#!/bin/bash # Description: Search for the `ProcessL1UserDepositTxs` method across the codebase to verify its implementation or references. # Use ripgrep to search for the method name in the codebase. rg 'ProcessL1UserDepositTxs' --context 5Length of output: 3925
|
Instead of using our own mock, what do you think about these options:
I tend to prefer just testing the actual behavior rather than mocking (I guess the SDK calls that an "integration test" and not a unit test, oh well). But I can be persuaded otherwise if the integration test utils are too cumbersome. My thinking is that we might as well just go straight to the integration test if it isn't too much effort, rather than having an extra unit test that involves a lot of mocking. |
Bank module mock from the SDK Integration test utils After re-reviewing the added unit tests in this PR and re-reading through the Testing docs and ADR 059, I don't think that any of them would qualify to split out into integration tests. I'll start working on a separate PR to add a rollup module integration test to the |
|
@natebeauregard Sounds good, appreciate the descriptions. Let's go with what you suggested. |
To close the loop, I brought up #165 to add the integration test highlighting the rollup keeper/bank keeper interaction. |
| @@ -108,7 +108,7 @@ func (k *Keeper) MintETH(ctx sdk.Context, addr sdk.AccAddress, amount sdkmath.In | |||
| return fmt.Errorf("failed to send deposit coins from rollup module to user account %v: %v", addr, err) | |||
| } | |||
|
|
|||
| // TODO: move event emission to msg_server.go | |||
| // TODO: only emit sdk.EventTypeMessage once per message | |||
There was a problem hiding this comment.
Noticed a bug with the message emission for ApplyL1Txs, I updated #162 to reflect this
x/rollup/keeper/msg_server_test.go
Outdated
| var resp *types.MsgApplyL1TxsResponse | ||
| resp, err = keeper.NewMsgServerImpl(s.rollupKeeper).ApplyL1Txs(s.ctx, test.msg) |
There was a problem hiding this comment.
Why the extra var and not :=?
There was a problem hiding this comment.
I was getting the following IDE warning and figured I'd remove the shadowed declaration and just reassign the previously initialized err field: Declaration of 'err' shadows declaration at msg_server_test.go
It's kind of unfortunate you can't conditionally initialize vars in go when assigning multiple vars. Lmk if you feel strongly about changing this, otherwise I'll leave it in
There was a problem hiding this comment.
It'll be removed in the integration test PR too since now err isn't declared above it :)
x/rollup/keeper/msg_server_test.go
Outdated
|
|
||
| // Verify that the l1 block info and l1 block history are saved to the store | ||
| expectedBlockInfo := eth.BlockToInfo(testutils.GenerateL1Block()) | ||
| store := sdk.UnwrapSDKContext(s.ctx).KVStore(s.storeKey) |
There was a problem hiding this comment.
Is this how the store is retrieved in other Cosmos SDK unit tests? Is the store ever kept on the keeper suite struct?
There was a problem hiding this comment.
Most cosmos sdk unit tests just use pre-existing query endpoints instead of accessing the store directly, however since we don't have any for asserting the l1 block info I opted to just check the store directly. I like your idea of keeping the store in the keeper suite so we can access it directly from the suite, I'll update that.
There was a problem hiding this comment.
Once we figure out #160, we'll probably add query endpoints. At that point, it would be better to use the query endpoints in tests than directly accessing the store.
This works for now.
- Adds new makefile commands to install mockgen and to generate expected keeper mocks for use in x/rollup unit tests. - Generates expected keeper mocks for the x/rollup module
Adds unit test coverage to assert behavior for the x/rollup messages and the related helper functions involved in the deposit and withdrawal flow.
natebeauregard
force-pushed
the
nkb.add-rollup-module-tests
branch
from
0b2e473
to
fa8d701
Compare
Closes: #134
This PR is heavily influenced by the Cosmos SDK testing docs and existing Cosmos SDK/Osmosis unit tests.
x/rollupbankkeeperx/rollupunit tests.x/rollupmessages and the related helper functions involved in the deposit and withdrawal flow.Summary by CodeRabbit
New Features
install-mockgenandgen-mockstargets in the build process to facilitate mock generation for testing.BankKeeperinterface to define methods for managing coin transfers, minting, and burning within the rollup module.Keepercomponent and its methods, enhancing the reliability of transaction processing and withdrawals.GenerateEthTxsfunction to encapsulate block generation logic, improving modularity and readability.BankKeeperinterface to streamline testing processes.Bug Fixes
Documentation