test: ibc-rate-limit test updates #8759
Conversation
PaddyMc
added
the
V:state/compatible/no_backport
WalkthroughThe changes in this pull request introduce new methods and modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (18)
tests/osmosisibctesting/wasm.go (3)
26-28: Consider making the code ID configurableThe function now uses a hardcoded code ID of 1 when calling
InstantiateRLContractRaw. While this simplifies the function, it might be less flexible for future changes or different environments.Consider making the code ID configurable, either through a parameter or a constant:
-func (chain *TestChain) InstantiateRLContract(suite *suite.Suite, quotas string) sdk.AccAddress { - addr := chain.InstantiateRLContractRaw(1, suite, quotas) +func (chain *TestChain) InstantiateRLContract(suite *suite.Suite, quotas string, codeId uint64) sdk.AccAddress { + addr := chain.InstantiateRLContractRaw(codeId, suite, quotas) return addr }This change would allow for more flexibility in testing different versions of the contract.
Line range hint
30-47: Approve new InstantiateRLContractRaw function with a minor suggestionThe new
InstantiateRLContractRawfunction is well-implemented, providing flexibility with the custom code ID and correctly using the governance module as both the creator and admin of the contract.Consider adding a bit more context to the error returned from
contractKeeper.Instantiate:- addr, _, err := contractKeeper.Instantiate(chain.GetContext(), codeId, creator, creator, initMsgBz, "rate limiting contract", nil) - suite.Require().NoError(err) + addr, _, err := contractKeeper.Instantiate(chain.GetContext(), codeId, creator, creator, initMsgBz, "rate limiting contract", nil) + suite.Require().NoError(err, "failed to instantiate rate limiting contract")This will provide more context if the test fails due to this error.
94-98: Approve new MigrateContract function with a minor suggestionThe new
MigrateContractfunction is well-implemented, providing the necessary functionality for contract migration as outlined in the PR objectives.Consider wrapping the error returned from
contractKeeper.Migratewith some context:- return contractKeeper.Migrate(chain.GetContext(), contract, sender, newCodeId, msg) + result, err := contractKeeper.Migrate(chain.GetContext(), contract, sender, newCodeId, msg) + if err != nil { + return nil, fmt.Errorf("failed to migrate contract: %w", err) + } + return result, nilThis will provide more context about the operation that failed if an error occurs during contract migration.
x/ibc-rate-limit/ibc_middleware_test.go (15)
342-345: Replacefmt.Printfandfmt.Printlnwith proper logging or remove themThe use of
fmt.Printfandfmt.Printlnfor outputting messages can clutter test outputs. Consider using the test suite's logging functions, such assuite.T().Logf(), or remove these statements if they are unnecessary.
352-355: Replacefmt.Printfandfmt.Printlnwith proper logging or remove themThe print statements here are used for debugging purposes. To keep the test outputs clean, consider using
suite.T().Logf()for logging or remove these statements if they're no longer needed.
408-408: Remove unnecessaryfmt.PrintlnstatementThe
fmt.Println(denomTrace)appears to be a leftover from debugging. Removing this statement will help keep test outputs concise.
429-429: Replacefmt.Printfwith proper logging or remove itUsing
fmt.Printffor outputting test information can clutter the output. Consider usingsuite.T().Logf()or remove the print statement if it's not needed.
Line range hint
447-447: Remove unnecessaryfmt.PrintlnstatementThe
fmt.Println(denomTrace)seems to be for debugging purposes. It would be better to remove it to keep the test output clean.
Line range hint
485-485: Replacefmt.Printfwith proper logging or remove itConsider replacing
fmt.Printfwithsuite.T().Logf()or removing the print statement to maintain clean test outputs.
504-504: Remove unnecessaryfmt.PrintlnstatementThe
fmt.Println("trying to send ", sendAmount)is likely used for debugging. Removing it will help keep the test outputs focused.
508-508: Replacefmt.Printfwith proper logging or remove itUsing
suite.T().Logf()instead offmt.Printfwill integrate better with the test framework's output.
519-519: Remove unnecessaryfmt.PrintlnstatementThe
fmt.Println("trying to send ", sendAmount)can be removed to keep the test output concise.
Line range hint
557-557: Remove unnecessaryfmt.PrintlnstatementRemoving this debug statement will help keep test outputs clean.
Line range hint
592-592: Replacefmt.Printfwith proper logging or remove itConsider using
suite.T().Logf()for consistency and better integration with the test suite.
635-635: Remove unnecessaryfmt.PrintlnstatementThis print statement appears to be for debugging and can be removed.
Line range hint
652-652: Remove unnecessaryfmt.PrintlnstatementCleaning up this debug statement will keep the test output focused.
810-810: Remove unnecessaryfmt.PrintlnstatementTo maintain clean and professional test outputs, consider removing this print statement.
857-857: Remove unnecessaryfmt.PrintlnstatementRemoving this will help keep the test outputs clean and avoid clutter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/ibc-rate-limit/bytecode/rate_limiter.wasmis excluded by!**/*.wasm,!**/*.wasmx/ibc-rate-limit/bytecode/rate_limiter_v1.wasmis excluded by!**/*.wasm,!**/*.wasm
📒 Files selected for processing (2)
- tests/osmosisibctesting/wasm.go (3 hunks)
- x/ibc-rate-limit/ibc_middleware_test.go (8 hunks)
🧰 Additional context used
🔇 Additional comments (5)
tests/osmosisibctesting/wasm.go (1)
Line range hint
1-112: Summary of changes and alignment with PR objectivesThe modifications to this file enhance the testing capabilities for rate limiting contracts and align well with the PR objectives. Key improvements include:
- More flexible contract instantiation with custom code IDs.
- Support for contract migration, which is crucial for testing the migration process mentioned in the PR objectives.
- Simplified
InstantiateRLContractfunction, which now delegates to the more flexibleInstantiateRLContractRaw.These changes provide the necessary tools to implement the new tests described in the PR objectives, particularly for testing restricted channels and verifying the migration process from the older contract version to the new one.
x/ibc-rate-limit/ibc_middleware_test.go (4)
828-869: Ensure proper error handling inTestV1MigrateThe migration test seems comprehensive. However, after migration, it's crucial to verify that all functionalities, including new features, work as expected. Consider adding assertions to check the state of the contract and ensure that restrictions are enforced correctly post-migration.
493-511: Confirm that restrictions are correctly enforced inTestSendTransferWithRestrictedChannelNativeThe test verifies that sending funds via a restricted channel fails, which is correct. Ensure that the
RemovePathandSetDenomRestrictionsmethods are functioning as intended and that any changes to channel IDs or denominations are accurately reflected in the test.
515-535: Confirm correctness of non-native token restrictions inTestSendTransferWithRestrictedChannelNonNativeThe test checks restrictions on non-native tokens. Verify that denomination traces and channel restrictions are correctly handled, and that the test accurately reflects the intended behavior.
403-410: Check for potential issues with denomination handlingIn the conditional block, ensure that the denomination parsing and assignment to
denomandrestrictedDenomare correct. Pay special attention to the use ofGetFullDenomPathandIBCDenomto avoid any inconsistencies.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
x/ibc-rate-limit/ibc_middleware_test.go (7)
331-337: Add error handling for ExecuteContract callThe
ExecuteContractcall may fail, but the error is currently being ignored. It's important to handle potential errors to ensure the test behaves correctly in failure scenarios. Consider adding error handling like this:_, err := suite.chainA.ExecuteContract(addr, osmosisApp.AccountKeeper.GetModuleAddress(govtypes.ModuleName), []byte(resetQuota), sdk.Coins{}) suite.Require().NoError(err, "Failed to execute RemovePath contract call")This will cause the test to fail if the contract execution fails, making it easier to identify and debug issues.
339-347: Remove debug print statementsThe method contains debug print statements that should be removed before merging to production. While helpful during development, these statements can clutter logs in a production environment. Consider removing or commenting out these lines:
fmt.Printf("resetting path quota for denom=%s channel=%s quota_id=%s", denom, channel, quota_id) fmt.Println(resetQuota)If you want to keep some form of logging for debugging purposes, consider using a proper logging framework that can be easily enabled/disabled based on the environment.
Line range hint
403-432: LGTM: Denomination restrictions correctly implementedThe changes to
fullSendTestlook good. The method now properly handles setting denomination restrictions for both native and non-native tokens. This enhances the test coverage by ensuring that the rate limiting works correctly with the new restriction feature.One minor suggestion for improved readability:
restrictedDenom := denom if !native { denomTrace := transfertypes.ParseDenomTrace(transfertypes.GetPrefixedDenom("transfer", "channel-0", denom)) restrictedDenom = denomTrace.GetFullDenomPath() denom = denomTrace.IBCDenom() }This slight reorganization makes it clearer that
restrictedDenomis always set, regardless of whether the token is native or not.
493-512: LGTM: Comprehensive test for restricted channel with native tokensThis test case effectively verifies that the rate limiting middleware correctly prevents sending native tokens through a restricted channel. It covers the essential steps:
- Setting up the rate limiting
- Removing the path quota
- Setting channel restrictions
- Attempting to send on the restricted channel
The test correctly expects an error when trying to send on the restricted channel, which is the desired behavior.
One minor suggestion to improve the test's robustness:
_, err = suite.AssertSend(false, suite.MessageFromAToB(sdk.DefaultBondDenom, osmomath.NewInt(1))) suite.Require().Error(err) suite.Require().Contains(err.Error(), "channel is not allowed for this denom", "Error should specifically mention channel restriction")This additional assertion ensures that the error is specifically related to the channel restriction, rather than any other type of error that might occur during the send operation.
514-536: LGTM: Comprehensive test for restricted channel with non-native tokensThis test case effectively verifies that the rate limiting middleware correctly prevents sending non-native (IBC) tokens through a restricted channel. It covers the essential steps:
- Setting up the rate limiting
- Removing the path quota
- Setting channel restrictions for the full IBC denomination path
- Attempting to send on the restricted channel
The test correctly expects an error when trying to send on the restricted channel, which is the desired behavior.
Similar to the native token test, consider adding a more specific error check:
_, err = suite.AssertSend(false, suite.MessageFromAToB(denom, osmomath.NewInt(1))) suite.Require().Error(err) suite.Require().Contains(err.Error(), "channel is not allowed for this denom", "Error should specifically mention channel restriction")This additional assertion ensures that the error is specifically related to the channel restriction for the IBC token.
Line range hint
740-802: LGTM: Comprehensive test for denomination restriction flowThis test case effectively verifies the entire flow of denomination restrictions in the rate limiting middleware. It covers all crucial scenarios:
- Sending without restrictions
- Adding restrictions
- Sending on allowed and disallowed channels
- Removing restrictions
- Verifying unrestricted sending after removal
The test is well-structured and provides good coverage of the denomination restriction functionality.
To improve clarity and maintainability, consider breaking down the test into sub-tests using
t.Run(). This can make it easier to identify which part of the flow failed if an error occurs. For example:func (suite *MiddlewareTestSuite) TestDenomRestrictionFlow() { // Setup code... suite.Run("Send without restrictions", func() { _, err := suite.AssertSend(true, suite.MessageFromAToB(denom, sendAmount)) suite.Require().NoError(err, "Send should succeed without restrictions") }) suite.Run("Add restrictions", func() { // Add restriction code... }) suite.Run("Send with restrictions", func() { // Test allowed and disallowed sends... }) suite.Run("Remove restrictions", func() { // Remove restrictions and verify unrestricted sending... }) }This structure can make the test more readable and easier to maintain.
828-871: LGTM: Comprehensive test for contract migration from v1 to v2This test case effectively verifies the migration process of the ibc-rate-limit contract from v1 to v2. It covers all crucial steps:
- Setting up and testing the v1 contract
- Migrating to v2
- Testing v2 functionality, including new features like denomination restrictions
The test is well-structured and provides good coverage of the migration process and post-migration functionality.
To improve clarity and make the test's steps more explicit, consider adding comments or log statements at key points in the test. For example:
// Test v1 functionality suite.testChannelQuota(sendAmount, 2) fmt.Println("Migrating contract from v1 to v2...") // Migrate to new contract _, err := suite.chainA.MigrateContract(addr, osmosisApp.AccountKeeper.GetModuleAddress(govtypes.ModuleName), newCodeId, []byte("{}")) suite.Require().NoError(err) fmt.Println("Testing v2 functionality...") // Set the restrictions (new v2 feature) suite.SetDenomRestrictions(addr, denom, channel) // Test v2 functionality suite.testChannelQuota(sendAmount, 1)These additions can make it easier to follow the flow of the test and understand what's being tested at each stage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/ibc-rate-limit/ibc_middleware_test.go (8 hunks)
🧰 Additional context used
🔇 Additional comments (4)
x/ibc-rate-limit/ibc_middleware_test.go (4)
Line range hint
546-575: LGTM: Denomination restrictions correctly implemented for receive testThe changes to
fullRecvTestlook good. The method now properly handles setting denomination restrictions for both native and non-native tokens in the context of receiving transfers. This enhances the test coverage by ensuring that the rate limiting works correctly with the new restriction feature for incoming transfers.The implementation correctly differentiates between native and non-native tokens, setting the appropriate restricted denomination in each case. This is crucial for testing the behavior of the rate limiting middleware in various scenarios.
606-622: LGTM: Correct verification of receive behavior with restricted channels for native tokensThis test case effectively verifies that the rate limiting middleware does not prevent receiving native tokens through a channel that is restricted for sending. This is an important distinction in the middleware's behavior. The test covers the key points:
- Setting up the rate limiting
- Removing the path quota
- Setting channel restrictions to only allow channel-1
- Attempting to receive on channel-0 (which is restricted for sending)
The test correctly expects no error when receiving on the restricted channel, which confirms that the restriction only applies to outgoing transfers. This is crucial for maintaining the correct behavior of the IBC protocol while implementing send restrictions.
624-639: LGTM: Correct verification of receive behavior with restricted channels for non-native tokensThis test case effectively verifies that the rate limiting middleware does not prevent receiving non-native (IBC) tokens through a channel that is restricted for sending. This complements the native token test and ensures comprehensive coverage. The test covers the key points:
- Setting up the rate limiting
- Removing the path quota
- Setting channel restrictions to only allow channel-1 for the local denomination
- Attempting to receive on channel-0 (which is restricted for sending)
The test correctly expects no error when receiving on the restricted channel, confirming that the restriction only applies to outgoing transfers, even for IBC tokens. This is crucial for maintaining the correct behavior of the IBC protocol while implementing send restrictions for various token types.
Line range hint
1-871: Overall assessment: Comprehensive and well-structured test suiteThe changes to this file significantly enhance the test coverage for the IBC rate limiting middleware. Key improvements include:
- New methods for managing channel quotas and denomination restrictions.
- Updated existing tests to incorporate new functionality.
- New test cases covering restricted channels for both native and non-native tokens.
- A comprehensive test for the denomination restriction flow.
- A migration test from v1 to v2 of the rate limiting contract.
These additions ensure that the middleware's core functionalities, including the new features, are thoroughly tested. The test cases are well-structured and cover a wide range of scenarios, which is crucial for maintaining the reliability of the IBC rate limiting system.
Minor suggestions for improvement mainly focus on:
- Using a JSON library for message construction instead of string formatting.
- Adding more specific error checks in some test cases.
- Improving clarity in some tests by adding comments or using sub-tests.
- Removing debug print statements from production code.
Overall, these changes represent a significant improvement in the test coverage and quality of the IBC rate limiting middleware.
What is the purpose of the change
These changes introduce some new tests and adjustments to previous tests related to the ibc-rate-limiting module and smart contract. The original bytecode for the rate limiter contract has been renamed to
ibc_rate_limiter_v1.wasmand the newer version has been namedibc_rate_limiter.wasm.Tests include the following:
Testing and Verifying
This change added tests and can be verified as follows:
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)