test: many to one test #705
Conversation
WalkthroughThe changes enhance the token minting and balance management functionalities within the Cosmos SDK. Several methods in the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Warning Review ran into problems🔥 ProblemsError running Gitleaks: Cloning the repository previously failed. Error running golangci-lint: Command failed with exit code 4: jk_cp -o -f -k -j /cache/54/6300ea40-43e2-43c0-8800-d997902910bb /dev/null /dev/urandom /usr/bin/ast-grep /usr/bin/curl /usr/bin/diff /usr/bin/dirname /usr/bin/env /usr/bin/fd /usr/bin/gawk /usr/bin/gh /usr/bin/jq /usr/bin/patch /usr/bin/rg /usr/bin/shellcheck /usr/bin/tokei /usr/bin/tree /usr/bin/uniq /usr/bin/xargs /etc/ssl/certs /etc/passwd /etc/group /home/jailuser/ast-grep-rules ERROR: /usr/bin/ast-grep does not exist Usage: /usr/sbin/jk_cp -j [OPTIONS] -h --help : this help screen Note: if no jail is specified, the first argument is Error running semgrep: Cloning the repository previously failed. Git: Failed to clone repository. Please run the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
x/evm/keeper/erc20.go (1)
40-40: Add a comment explaining the method's purpose.To improve code readability and maintainability, consider adding a comment that explains the purpose of the
ERC20BalanceOfmethod and its parameters.Here's a suggested comment:
// ERC20BalanceOf queries the balance of the specified address for the given ERC20 contract. // It returns the balance as a *big.Int and an error if the query fails.x/crosschain/keeper/keeper_v1_test.go (3)
168-174: LGTM:SetToken()method is well-implemented, with a minor suggestion.The
SetToken()method is a good wrapper for testing purposes. It correctly handles the special case for the default denomination and checks for errors.Consider adding a comment explaining the purpose of this method and why the default denomination is treated differently. This would improve the code's readability and maintainability.
func (suite *KeeperTestSuite) SetToken(symbol string, bridgeDenoms ...string) { + // SetToken is a test helper that sets up a token with the given symbol and bridge denominations. + // The default denomination (fxtypes.DefaultDenom) is treated as a special case with no bridge denominations. if symbol == fxtypes.DefaultDenom { bridgeDenoms = []string{} } err := suite.Keeper().SetToken(suite.Ctx, "Test Token", symbol, 18, bridgeDenoms...) suite.NoError(err) }
176-182: LGTM with suggestions:AddTokenPair()method could be improved.The
AddTokenPair()method is a useful helper for testing. However, there are a few areas where it could be improved:
- The use of a hardcoded hex address (
helpers.GenHexAddress()) might not be suitable for all test scenarios. Consider parameterizing this.- The method doesn't check for errors when adding the token pair. It's generally good practice to handle potential errors, even in test code.
Consider refactoring the method as follows:
-func (suite *KeeperTestSuite) AddTokenPair(denom string, isNative bool) { +func (suite *KeeperTestSuite) AddTokenPair(denom string, isNative bool, erc20Address string) error { contractOwner := erc20types.OWNER_EXTERNAL if isNative { contractOwner = erc20types.OWNER_MODULE } - suite.App.Erc20Keeper.AddTokenPair(suite.Ctx, erc20types.TokenPair{Erc20Address: helpers.GenHexAddress().String(), Denom: denom, Enabled: true, ContractOwner: contractOwner}) + if erc20Address == "" { + erc20Address = helpers.GenHexAddress().String() + } + return suite.App.Erc20Keeper.AddTokenPair(suite.Ctx, erc20types.TokenPair{ + Erc20Address: erc20Address, + Denom: denom, + Enabled: true, + ContractOwner: contractOwner, + }) }This refactoring allows for more flexible testing scenarios and proper error handling.
184-188: LGTM with suggestions:AddBridgeToken()method could be more flexible.The
AddBridgeToken()method is a useful helper for testing bridge token functionality. However, it could be made more flexible to accommodate various test scenarios:Consider refactoring the method to allow for more customization:
-func (suite *KeeperTestSuite) AddBridgeToken(contractAddr string, symbol string) { +func (suite *KeeperTestSuite) AddBridgeToken(contractAddr, name, symbol string, decimals uint32) { bridgeTokenClaim := &types.MsgBridgeTokenClaim{ TokenContract: contractAddr, - Name: "Test Token", + Name: name, Symbol: symbol, - Decimals: 18, + Decimals: decimals, ChainName: suite.chainName, } err := suite.Keeper().AddBridgeTokenExecuted(suite.Ctx, bridgeTokenClaim) suite.Require().NoError(err) }This refactoring allows for more flexible testing scenarios by parameterizing the token name and decimals. You might also consider adding a comment explaining the purpose of this method and its relationship to the
AddBridgeTokenExecutedfunction.x/crosschain/keeper/many_to_one_test.go (1)
64-91: Enhance test coverage with additional edge cases inTestManyToOneCurrently,
TestManyToOnecovers basic success scenarios. Consider adding test cases for:
- Invalid
ConvertDenomvalues that are neither base nor bridge denoms.- Unsupported
Targetvalues.- Empty strings for
ConvertDenomorTarget.This will ensure that the function behaves correctly under all possible inputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- testutil/helpers/suite.go (3 hunks)
- x/crosschain/keeper/bridge_call_out_v1_test.go (1 hunks)
- x/crosschain/keeper/keeper_v1_test.go (4 hunks)
- x/crosschain/keeper/many_to_one.go (7 hunks)
- x/crosschain/keeper/many_to_one_test.go (1 hunks)
- x/evm/keeper/erc20.go (1 hunks)
🔇 Additional comments (18)
x/crosschain/keeper/bridge_call_out_v1_test.go (1)
84-84: LGTM! Verify theCheckBalanceOfmethod implementation.The change from
suite.checkBalanceOftosuite.CheckBalanceOfappears to be part of a refactoring effort. The functionality seems to remain the same, which is good for maintaining test integrity.To ensure the refactoring was done correctly, please run the following script to verify the
CheckBalanceOfmethod implementation:x/crosschain/keeper/keeper_v1_test.go (2)
13-13: LGTM: Import statements are appropriate.The new import statements are relevant to the added functionality and existing code. They are correctly formatted and placed.
Also applies to: 25-30
120-123: LGTM:ModuleAddress()method is correctly implemented.The
ModuleAddress()method correctly usesauthtypes.NewModuleAddress()to generate the module address based on thechainName. This is a standard approach in Cosmos SDK.testutil/helpers/suite.go (7)
131-134: UpdatedMintTokenfunction allows minting multiple coinsThe modification correctly updates the
MintTokenfunction to accept a variadic parameter, enabling minting of multiple coins to a single address.
145-147:Balancefunction correctly retrieves all balancesThe
Balancefunction properly retrieves all balances for the given account usingGetAllBalances.
149-154:CheckBalancefunction accurately verifies individual balancesThe function iterates over the expected balances and verifies that each denomination matches the actual balance.
156-159:CheckAllBalancefunction effectively compares total balancesThe function compares the expected and actual total balances, ensuring they match exactly.
161-165:CheckBalanceOffunction properly checks ERC20 token balancesThe function correctly retrieves the ERC20 token balance and asserts its equality with the expected balance.
167-174:NewCoinfunction generates coins with specified or random amountsThe function creates a new coin with a random denomination and amount, or uses the provided amount if specified.
176-184:NewBridgeCoinfunction creates bridge coins associated with a moduleThe function generates a new bridge coin and corresponding token address, facilitating cross-chain interactions.
x/crosschain/keeper/many_to_one.go (8)
56-58: Handle default denomination inDepositBridgeToken.The added condition checks if
bridgeToken.Denomis the default denomination (fxtypes.DefaultDenom). If so, it directly sends the coins from the module to the account holder without additional processing. This ensures that the default token is handled appropriately.
76-77: IntroduceWithdrawBridgeTokenfunction.The
WithdrawBridgeTokenfunction has been added or renamed fromClaimBridgeToken. This function manages the withdrawal of bridge tokens to the crosschain module. Ensure that its implementation correctly handles all necessary logic and that any renaming does not affect the functionality.
81-83: Ensure correct handling of default denomination inWithdrawBridgeToken.An early return has been added for cases where
bridgeToken.Denomequalsfxtypes.DefaultDenom. This implies no further processing is needed for the default denomination. Verify that this logic is correct and that skipping additional steps is appropriate in this context.
118-120: Simplify conversion logic inManyToOne.The condition checks if
baseDenomis the default denomination or if no target is specified. If either is true, it returnsbaseDenomimmediately. This optimization avoids unnecessary conversions and simplifies the function's logic when conversion isn't needed.
153-155: OptimizeConversionCoinfor default denomination.An early return has been added when
coin.Denomis the default denomination inConversionCoin. This prevents unnecessary processing and reflects that no conversion is required for the default token.
92-93: Verify skipping coin burning for native ERC20 tokens.In
WithdrawBridgeToken, the function now returns early iftokenPair.IsNativeERC20()istrue, bypassing the coin burning step. Confirm that this behavior is intentional and appropriate, as it affects the total supply of tokens. Skipping the burning process for native ERC20 tokens might be necessary, but it's important to ensure this aligns with the overall token management strategy.To review where
IsNativeERC20influences logic, run:#!/bin/bash # Description: Find all instances where `IsNativeERC20` affects control flow. rg --type go 'IsNativeERC20\(' -A 5
36-44: Confirm the use ofWithdrawBridgeTokeninBaseCoinToBridgeToken.The function
BaseCoinToBridgeTokennow callsWithdrawBridgeTokeninstead ofClaimBridgeToken. Ensure thatWithdrawBridgeTokencorrectly replacesClaimBridgeTokenand that it handles all necessary withdrawal logic for bridge tokens. Verify that any changes in behavior are intentional and that all dependent code operates as expected.To check for correct usage, you can search for all references to
WithdrawBridgeToken:
17-20: UpdatetokenAddrparameter type and ensure consistent handling.The
tokenAddrparameter inBridgeTokenToBaseCoinhas been changed fromcommon.Addresstostring. This simplifies address handling but requires that all usages oftokenAddrwithin this function and any downstream functions correctly handle thestringtype. Also, verify that all callers ofBridgeTokenToBaseCoinhave been updated to pass astringinstead of acommon.Address.To ensure consistency, run the following script to find all calls to
BridgeTokenToBaseCoinand check that they pass astringastokenAddr:✅ Verification successful
✅ Verification Successful:
tokenAddris consistently handled as astringacross all usages.
- All calls to
BridgeTokenToBaseCoinpass astringfor thetokenAddrparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `BridgeTokenToBaseCoin` to verify parameter types. rg --type go 'BridgeTokenToBaseCoin\(' -A 1Length of output: 717
Script:
#!/bin/bash # Description: Find the definition of `TokenAddr` in `many_to_one_test.go` to verify its type. rg --type go 'TokenAddr\s*:' x/crosschain/keeper/many_to_one_test.goLength of output: 189
Co-authored-by: nulnut <151493716+nulnut@users.noreply.github.com>
Summary by CodeRabbit
New Features
Tests