test(evm): Add tests for feemarket wrapper #66
Conversation
WalkthroughThe pull request introduces comprehensive test coverage for the FeeMarketWrapper in the EVM module. A new test file Changes
Sequence DiagramsequenceDiagram
participant Wrapper as FeeMarketWrapper
participant Keeper as FeeMarketKeeper
participant Context as SDK Context
Wrapper->>Keeper: Request Parameters
Keeper-->>Wrapper: Return Parameters
Wrapper->>Wrapper: Check BaseFee Nil
alt BaseFee Not Nil
Wrapper->>Wrapper: Convert to 18 Decimals
else BaseFee Nil
Wrapper->>Wrapper: Handle Nil Case
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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 (
|
0xstepit
requested review from
ramacarlucho and
MalteHerrmann
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
x/evm/wrappers/feemarket_test.go (1)
17-105: Encourage checks for negative/invalid base fees.
The table-driven tests cover many scenarios, such as zero and nil fee amounts, ensuring thorough testing. For completeness, consider adding a test case that explicitly checks how negative fees are handled, if those scenarios are expected or disallowed in your domain.x/evm/types/scaling.go (1)
17-19: Improve clarity in the added comment.The newly added comment clarifies the function contract but slightly repeats the same idea across lines 18 and 19. You can unify them into a single succinct sentence for better readability:
-// CONTRACT: The function should only be called when the coin denom is the EVM. This means that -// should be called only when the code forces the denom to be the expected one. +// CONTRACT: Only call this function when the coin denom is confirmed to be the EVM denom.x/evm/wrappers/bank_test.go (4)
23-56: TestMintAmountToAccount: Confirm thorough negative coverage.This test suite covers different decimal conversions and mint operations well. Consider adding a test case for negative amounts in the future to ensure that any invalid input is handled gracefully (even if your flow logically prevents negative amounts).
82-95: TestMintAmountToAccount: Provide additional error context.When returning an error such as “failed to mint coins to account in bank wrapper,” it might be helpful to append contextual information (e.g., which address or token is involved) for easier debugging if the logs surface this error.
149-205: TestBurnAmountFromAccount: Verify partial burn scenarios.The test covers integer-based full burns effectively. Consider verifying partial burn scenarios (e.g., turning 1.5 tokens into 0.5 tokens) to ensure correct decimal conversion in these intermediate conditions.
600-741: TestGetBalance: Potential large-amount boundary checks.The test rightfully checks max int64. However, ensure that extremely large amounts formatted beyond int64 range (if ever possible in your domain) are either disallowed or handled properly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
CHANGELOG.md(2 hunks)go.mod(2 hunks)x/erc20/keeper/msg_server_test.go(1 hunks)x/erc20/types/mocks/BankKeeper.go(1 hunks)x/evm/types/scaling.go(1 hunks)x/evm/wrappers/bank_test.go(1 hunks)x/evm/wrappers/feemarket.go(1 hunks)x/evm/wrappers/feemarket_test.go(1 hunks)x/evm/wrappers/testutil/mock.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- x/erc20/types/mocks/BankKeeper.go
- x/evm/wrappers/testutil/mock.go
🔇 Additional comments (5)
x/evm/wrappers/feemarket.go (1)
55-57: Add a safeguard if negative base fees are invalid.
Currently, the code converts the base fee only if it is non-nil. If negative values are not expected, consider an additional defensive check or validation to prevent potential misconfigurations.
x/evm/wrappers/feemarket_test.go (1)
106-195: Great use of mocks and table-driven tests for CalculateBaseFee.
The structure is clean, uses a mock keeper, and covers various fee scenarios. This is a good approach to isolate and verify the wrapper's behavior.
x/erc20/keeper/msg_server_test.go (1)
19-19: Approve migration to go.uber.org/mock/gomock.
Switching from github.com/golang/mock to go.uber.org/mock is a recommended move as it aligns with best practices and ensures consistent usage with the rest of the codebase. No further changes needed.
x/evm/wrappers/bank_test.go (1)
272-306: TestSendCoinsFromModuleToAccount: Check coverage for zero or negative coins.
The suite already checks for zero conversions in one scenario (line 388). Consider verifying explicit negative amounts remain unreachable or cause errors. Properly documenting or testing such edge cases helps ensure clarity around what is and isn’t allowed.
go.mod (1)
50-50: LGTM! Appropriate migration to go.uber.org/mock
The change from github.com/golang/mock to go.uber.org/mock is correct as it's the new official home for gomock. The old package is correctly marked as indirect since it might still be required by other dependencies.
Let's verify that all gomock imports have been updated across the codebase:
Also applies to: 138-138
✅ Verification successful
Migration to go.uber.org/mock is complete and consistent
The verification shows:
- No remaining imports of the old
github.com/golang/mock/gomockpackage - All mock usages consistently use the new
go.uber.org/mock/gomockpackage across:- x/evm/wrappers tests
- x/erc20 tests and mocks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct usage of the old mock package
# This helps ensure the migration is complete
echo "Checking for any remaining direct imports of github.com/golang/mock..."
rg --type go "github.com/golang/mock/gomock"
echo "Verifying adoption of new mock package..."
rg --type go "go.uber.org/mock/gomock"
Length of output: 644
Description
This PR add tests for the feemarket wrapper used in the x/evm module to perform custom decimals conversions.
THIS BRANCH HAS BEEN CREATED OUT OF #65
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
GetParamsmethod by adding nil check for BaseFee parameterTests
GetBaseFee,CalculateBaseFee, andGetParamsmethods