Fix ibchooks tests #1980
Fix ibchooks tests #1980
Conversation
WalkthroughThe recent updates primarily focus on refining the Changes
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 Configration 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: 1
Out of diff range and nitpick comments (4)
x/ibcratelimit/module/ibc_middleware_test.go (1)
Line range hint
258-277: RefactoredinitializeEscrowto set up initial conditions for testing rate limiting with escrow accounts.Consider breaking down this function into smaller, more focused functions to improve clarity and maintainability.
- // Move some funds from chainA to chainB so that there is something in escrow - // Each user has 10% of the supply, so we send most of the funds from one user to chainA + suite.setupInitialEscrowConditions()Define
setupInitialEscrowConditions()to handle the specifics of setting up escrow conditions.CHANGELOG.md (3)
Line range hint
222-222: Use descriptive text for URLs to improve readability and accessibility.It's a good practice to use descriptive text for URLs instead of bare URLs. This improves readability and accessibility. For example, change:
https://github.com/provenance-io/provenance/issues/123to
[Issue #123](https://github.com/provenance-io/provenance/issues/123)Also applies to: 253-253, 352-352, 402-402, 414-414, 430-430, 501-501, 512-512, 520-520, 556-556, 581-581, 593-593, 639-643, 690-690, 731-731, 784-784, 897-897
Line range hint
939-939: Remove unnecessary spaces inside emphasis markers.There are unnecessary spaces inside emphasis markers which can lead to incorrect formatting. Consider removing these spaces for correct emphasis formatting.
Line range hint
335-335: Remove unnecessary spaces inside code span elements.There are unnecessary spaces inside code span elements which can lead to incorrect formatting. Consider removing these spaces for correct code formatting.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
testutil/contracts/rate-limiter/Cargo.lockis excluded by!**/*.locktestutil/contracts/rate-limiter/artifacts/rate_limiter-aarch64.wasmis excluded by!**/*.wasmtestutil/contracts/rate-limiter/artifacts/rate_limiter.wasmis excluded by!**/*.wasm
Files selected for processing (18)
- .github/workflows/test.yml (1 hunks)
- CHANGELOG.md (1 hunks)
- app/app.go (5 hunks)
- internal/antewrapper/msg_fees_decorator.go (2 hunks)
- testutil/contracts/rate-limiter/Cargo.toml (1 hunks)
- testutil/contracts/rate-limiter/artifacts/checksums.txt (1 hunks)
- testutil/contracts/rate-limiter/artifacts/checksums_intermediate.txt (1 hunks)
- testutil/contracts/rate-limiter/src/contract_tests.rs (5 hunks)
- testutil/contracts/rate-limiter/src/execute.rs (4 hunks)
- testutil/contracts/rate-limiter/src/helpers.rs (3 hunks)
- testutil/contracts/rate-limiter/src/management.rs (4 hunks)
- testutil/contracts/rate-limiter/src/query.rs (2 hunks)
- testutil/ibc/testchain.go (3 hunks)
- x/ibchooks/ibc_middleware_test.go (3 hunks)
- x/ibchooks/keeper/params.go (1 hunks)
- x/ibchooks/marker_hooks_test.go (1 hunks)
- x/ibcratelimit/module/ibc_middleware.go (2 hunks)
- x/ibcratelimit/module/ibc_middleware_test.go (11 hunks)
Files skipped from review due to trivial changes (5)
- .github/workflows/test.yml
- testutil/contracts/rate-limiter/Cargo.toml
- testutil/contracts/rate-limiter/artifacts/checksums.txt
- testutil/contracts/rate-limiter/artifacts/checksums_intermediate.txt
- x/ibchooks/keeper/params.go
Additional comments not posted (38)
testutil/contracts/rate-limiter/src/query.rs (1)
Line range hint
1-11: Switch toto_json_binaryfor serialization aligns with modern JSON-based interfaces.Verification successful
The integration of
get_quotaswith other components correctly handles the JSON output. Thequeryfunction incontract.rsdirectly returns the result ofget_quotas, ensuring proper handling of the JSON output.
testutil/contracts/rate-limiter/src/contract.rs: Thequeryfunction callsquery::get_quotasand returns its result directly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `get_quotas` with other components. # Test: Search for usage of `get_quotas`. Expect: Proper handling of JSON output. rg --type rust $'get_quotas'Length of output: 236
Script:
#!/bin/bash # Extract the relevant code from `contract.rs` to verify the handling of JSON output from `get_quotas`. rg --type rust --context 10 'query::get_quotas' testutil/contracts/rate-limiter/src/contract.rsLength of output: 661
testutil/contracts/rate-limiter/src/helpers.rs (1)
Line range hint
5-31: Switch toto_json_binaryfor serialization in helper functions enhances consistency.internal/antewrapper/msg_fees_decorator.go (1)
Line range hint
4-84: Enhanced flexibility in test environment detection withstrings.HasPrefix.Verification successful
Verification of
isTestContextIntegrationThe
isTestContextfunction is used in the following files:
internal/antewrapper/msg_fees_decorator.gointernal/antewrapper/tx_gas_limit_decorator_test.gointernal/antewrapper/tx_gas_limit_decorator.gointernal/antewrapper/provenance_fee.goThese usages indicate that the function is integrated to handle different test environments properly. The changes enhance flexibility in detecting test contexts, which is beneficial for maintaining compatibility across various test setups.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `isTestContext` with other components. # Test: Search for usage of `isTestContext`. Expect: Proper handling across various test environments. rg --type go $'isTestContext'Length of output: 624
testutil/ibc/testchain.go (1)
Line range hint
86-154: Enhancements toSendMsgsNoCheckand addition ofcommitBlockimprove test chain handling.testutil/contracts/rate-limiter/src/management.rs (1)
Line range hint
115-237: Switch tofrom_jsonfor deserialization aligns with modern JSON-based interfaces.testutil/contracts/rate-limiter/src/execute.rs (1)
Line range hint
114-236: Switch tofrom_jsonfor deserialization aligns with modern JSON-based interfaces.Verification successful
The switch to
from_jsonfor deserialization in themanagement_add_and_remove_pathfunction aligns with modern JSON-based interfaces and is consistent with the PR's aim. Both instances of this function inexecute.rsandmanagement.rsare test functions and do not seem to be used elsewhere in the codebase.
testutil/contracts/rate-limiter/src/execute.rstestutil/contracts/rate-limiter/src/management.rsScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `management_add_and_remove_path` with other components. # Test: Search for usage of this function. Expect: Proper handling of JSON input. rg --type rust $'management_add_and_remove_path'Length of output: 228
Script:
#!/bin/bash # Description: Extract the function definition and its usage in `management.rs`. # Extract the function definition ast-grep --lang rust --pattern $'fn management_add_and_remove_path() { $$$ }' testutil/contracts/rate-limiter/src/management.rs # Search for the usage of the function within the file rg 'management_add_and_remove_path' testutil/contracts/rate-limiter/src/management.rsLength of output: 10847
x/ibcratelimit/module/ibc_middleware.go (1)
214-220: Handling ofFungibleTokenPacketDatainSendPacketenhances middleware capabilities.Verification successful
The handling of
FungibleTokenPacketDatain theSendPacketfunction is consistent and correct across the relevant files. The code properly checks for validDenomandAmountfields and includes functions to verify and extract ICS20 packet data.
x/ibcratelimit/module/ibc_middleware.go: Properly checks for validDenomandAmountfields.x/ibchooks/wasm_hook.go: Includes functions to verify if the packet is an ICS20 packet and to extract the denomination from the packet.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `SendPacket` with other components. # Test: Search for usage of `SendPacket`. Expect: Proper handling of `FungibleTokenPacketData`. rg --type go $'SendPacket'Length of output: 7699
Script:
#!/bin/bash # Description: Verify handling of `FungibleTokenPacketData` in files where `SendPacket` is used. # Search for `FungibleTokenPacketData` in the relevant files. rg --type go 'FungibleTokenPacketData' x/oracle/keeper/relay.go x/ibchooks/wasm_hook.go x/ibcratelimit/module/ibc_middleware.goLength of output: 493
Script:
#!/bin/bash # Description: Extract handling of `FungibleTokenPacketData` in the relevant files for detailed review. # Extract code snippets handling `FungibleTokenPacketData` in `x/ibcratelimit/module/ibc_middleware.go`. rg --type go --context 10 'FungibleTokenPacketData' x/ibcratelimit/module/ibc_middleware.go # Extract code snippets handling `FungibleTokenPacketData` in `x/ibchooks/wasm_hook.go`. rg --type go --context 10 'FungibleTokenPacketData' x/ibchooks/wasm_hook.goLength of output: 2857
testutil/contracts/rate-limiter/src/contract_tests.rs (5)
6-6: Switch fromfrom_binarytofrom_jsonfor deserialization.This change aligns with the broader serialization updates in the project, ensuring consistency with the new serialization strategy.
32-33: Ensure correct loading of module addresses from storage.The assertions correctly verify that the
IBCMODULEandGOVMODULEaddresses are stored and retrieved as expected. This is crucial for ensuring that the contract's state is consistent and reliable.
244-244: Update deserialization inquery_statetest to usefrom_json.This update is consistent with the changes in serialization strategy across the project. It's important to ensure that all parts of the codebase use the same serialization methods to avoid issues during data handling.
274-274: Verify updated query response handling inquery_statetest.The test adjustments here are aligned with the new serialization methods and ensure that the rate limit tracking is functioning as expected. This is crucial for maintaining the integrity of rate limiting in the system.
314-314: Handle edge cases in quota settings inbad_quotastest.This test ensures that quotas exceeding 100% are capped correctly, which is a good safety check to prevent configuration errors from causing unexpected behavior.
x/ibchooks/marker_hooks_test.go (1)
161-163: Ensure metadata fields are correctly updated to reflect new chain ID format.The changes correctly update the metadata fields to include the new chain ID format (
testchain2-1). This is consistent with the updated expectations for metadata representation in the system.x/ibchooks/ibc_middleware_test.go (3)
208-208: Updateclienttypes.NewHeightto reflect new versioning in IBC packets.The change correctly updates the height to
clienttypes.NewHeight(1, 100), aligning with the new versioning requirements for IBC packets. This ensures compatibility with the updated IBC module.
382-382: Ensure theTimeoutHeightis correctly set for new IBC transactions.The update to
TimeoutHeightusingclienttypes.NewHeight(1, 500)is appropriate, ensuring that the IBC transactions have a correct timeout setting based on the new height system.
432-440: Validate the newFullSendfunction implementation.The implementation of the
FullSendfunction correctly handles the sending of messages without checking, and properly relays packets between chains. This is crucial for testing IBC middleware functionalities.x/ibcratelimit/module/ibc_middleware_test.go (18)
17-17: Added import forabci "github.com/cometbft/cometbft/abci/types".This import is necessary for the new return types used in the test functions, aligning with the changes in the middleware logic.
24-24: Added import forcapabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"andtransfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types".These imports are essential for the updated test functions which now utilize types from these modules, particularly for message creation and capability handling.
130-146: UpdatedFullSendBToAto handle new return types and added error handling.The changes align with the updated
SendMsgsNoCheckmethod signature and improve error handling by checking for errors at each step.
151-177: UpdatedFullSendAToBto handle new return types and added comprehensive error handling.Similar to
FullSendBToA, these changes ensure that the function is robust against failures at any step of the message sending process.
185-194: EnhancedAssertReceiveto handle different scenarios based on thesuccessparameter.This method now properly checks for errors and ensures the acknowledgment message is as expected, which is crucial for testing the rate limiting functionality.
201-207: EnhancedAssertSendto handle different scenarios based on thesuccessparameter.This update is necessary for testing the new error handling logic in the rate limiting middleware.
213-213: AddedBuildChannelQuotato construct a quota JSON string for testing.This utility function is useful for creating consistent and repeatable tests for rate limiting functionality.
221-241: AddedTestNonICS20to test rate limiting logic under specific conditions.This test is crucial for ensuring that the rate limiting logic behaves correctly under non-standard IBC conditions, such as when dealing with non-ICS20 messages.
244-247: AddedTestSendTransferNoContractto ensure that transfers work correctly even when no rate limiting contract is configured.This test is important for ensuring that the system behaves correctly in the absence of specific configurations, which is a common real-world scenario.
251-254: AddedTestReceiveTransferNoContractto test the receiving functionality without a rate limiting contract.Ensuring that the system can handle receives without a rate limiting contract is crucial for robustness and flexibility.
Line range hint
282-332: Added comprehensive tests for rate limiting functionality, including sending and receiving under various conditions.These tests are essential for ensuring that the rate limiting functionality is working as expected across different scenarios.
337-347: Added tests for rate limiting with native and non-native tokens.Testing both native and non-native tokens ensures that the rate limiting functionality is robust and works across different types of assets.
351-372: AddedTestSendTransferResetto test the reset functionality of rate limits.This test is crucial for ensuring that rate limits are properly reset after the specified time period, which is a key feature of the rate limiting functionality.
Line range hint
376-419: Added tests for receiving rate limits with native and non-native tokens.These tests ensure that the receiving side of the rate limiting functionality is also robust and works correctly with different types of tokens.
422-433: Added tests for rate limiting on receives with specific token handling logic.These tests are important for ensuring that the rate limiting logic correctly handles tokens based on their source and type, which is crucial for accurate rate limiting.
437-447: AddedTestSendTransferNoQuotato ensure that transfers are allowed when no specific quotas are configured.This test is important for ensuring that the system behaves as expected in scenarios where no specific rate limiting quotas are set, which can be a common configuration.
451-520: AddedTestFailedSendTransferto test the behavior of the system when a send operation fails under rate limiting conditions.This test is crucial for ensuring that the system correctly handles failures during send operations under rate limiting, which is important for maintaining system integrity and user trust.
523-533: AddedTestUnsetRateLimitingContractto test the behavior when a rate limiting contract is unset.This test ensures that the system correctly handles the scenario where a previously set rate limiting contract is unset, which is important for system flexibility and configuration management.
app/app.go (4)
127-127: Added import foribctmmodule.This import is necessary for the new
RateLimitMiddlewareandibctm.AppModule{}additions.
296-296: AddedRateLimitMiddlewareto theAppstruct.This addition aligns with the PR's objective to enhance middleware handling. Ensure that all middleware interactions are properly tested.
547-547: Integration ofRateLimitMiddlewarewith the IBC transfer module.This change is crucial for applying rate limiting to IBC transfers. It's important to verify through integration tests that the middleware behaves as expected under various network conditions.
780-780: Addition ofibctm.AppModule{}to the module manager.This is a necessary addition to integrate the new IBC client module. Ensure that the module is initialized correctly and interacts seamlessly with other IBC modules.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (5)
CHANGELOG.md (5)
Line range hint
103-217: Consistency in list item markers.- 103: Expected: asterisk; Actual: dash - 104: Expected: asterisk; Actual: dash - 105: Expected: asterisk; Actual: dash - 106: Expected: asterisk; Actual: dash - 107: Expected: asterisk; Actual: dash - 108: Expected: asterisk; Actual: dash - 109: Expected: asterisk; Actual: dash - 110: Expected: asterisk; Actual: dash - 111: Expected: asterisk; Actual: dash - 112: Expected: asterisk; Actual: dash - 113: Expected: asterisk; Actual: dash - 114: Expected: asterisk; Actual: dash - 115: Expected: asterisk; Actual: dash - 116: Expected: asterisk; Actual: dash - 117: Expected: asterisk; Actual: dash - 187: Expected: asterisk; Actual: dash - 188: Expected: asterisk; Actual: dash - 189: Expected: asterisk; Actual: dash - 190: Expected: asterisk; Actual: dash - 191: Expected: asterisk; Actual: dash - 192: Expected: asterisk; Actual: dash - 193: Expected: asterisk; Actual: dash - 194: Expected: asterisk; Actual: dash - 195: Expected: asterisk; Actual: dash - 196: Expected: asterisk; Actual: dash - 197: Expected: asterisk; Actual: dash - 198: Expected: asterisk; Actual: dash - 199: Expected: asterisk; Actual: dash - 200: Expected: asterisk; Actual: dash - 201: Expected: asterisk; Actual: dash - 202: Expected: asterisk; Actual: dash - 203: Expected: asterisk; Actual: dash - 204: Expected: asterisk; Actual: dash - 205: Expected: asterisk; Actual: dash - 206: Expected: asterisk; Actual: dash - 207: Expected: asterisk; Actual: dash - 208: Expected: asterisk; Actual: dash - 209: Expected: asterisk; Actual: dash - 210: Expected: asterisk; Actual: dash - 211: Expected: asterisk; Actual: dash - 212: Expected: asterisk; Actual: dash - 213: Expected: asterisk; Actual: dash - 214: Expected: asterisk; Actual: dash - 215: Expected: asterisk; Actual: dash - 216: Expected: asterisk; Actual: dash - 217: Expected: asterisk; Actual: dash - 247: Expected: asterisk; Actual: dash - 248: Expected: asterisk; Actual: dash - 249: Expected: asterisk; Actual: dash - 326: Expected: asterisk; Actual: dash - 327: Expected: asterisk; Actual: dash - 328: Expected: asterisk; Actual: dash - 329: Expected: asterisk; Actual: dash - 330: Expected: asterisk; Actual: dash - 331: Expected: asterisk; Actual: dash - 332: Expected: asterisk; Actual: dash - 333: Expected: asterisk; Actual: dash - 334: Expected: asterisk; Actual: dash - 335: Expected: asterisk; Actual: dash - 336: Expected: asterisk; Actual: dash - 337: Expected: asterisk; Actual: dash - 338: Expected: asterisk; Actual: dash - 339: Expected: asterisk; Actual: dash - 340: Expected: asterisk; Actual: dash - 341: Expected: asterisk; Actual: dash - 342: Expected: asterisk; Actual: dash - 343: Expected: asterisk; Actual: dash - 344: Expected: asterisk; Actual: dash - 345: Expected: asterisk; Actual: dash - 346: Expected: asterisk; Actual: dash - 347: Expected: asterisk; Actual: dash - 348: Expected: asterisk; Actual: dash - 704: Expected: asterisk; Actual: dash - 712: Expected: asterisk; Actual: dash - 757: Expected: asterisk; Actual: dash - 765: Expected: asterisk; Actual: dashAlso applies to: 247-249, 326-348, 704-704, 712-712, 757-757, 765-765
Line range hint
811-811: Remove extra blank lines for consistency.- 811: Expected: 1; Actual: 2 - 1212: Expected: 1; Actual: 2 - 1229: Expected: 1; Actual: 2 - 1281: Expected: 1; Actual: 2 - 1385: Expected: 1; Actual: 2Also applies to: 1212-1212, 1229-1229, 1281-1281, 1385-1385
Line range hint
222-222: Avoid using bare URLs.Consider using Markdown link syntax to make URLs clickable and more readable.
Also applies to: 253-253, 352-352, 402-402, 414-414, 430-430, 501-501, 512-512, 520-520, 556-556, 581-581, 593-593, 639-639, 640-640, 641-641, 642-642, 643-643, 690-690, 731-731, 784-784, 897-897
Line range hint
939-939: Remove spaces inside emphasis markers.- * * Fix unit tests for ibchooks [#1980](https://github.com/provenance-io/provenance/pull/1980). + **Fix unit tests for ibchooks** [#1980](https://github.com/provenance-io/provenance/pull/1980).
Line range hint
335-335: Remove spaces inside code span elements.- ` MsgAddMarkerRequest` 100 hash (100,000,000,000 nhash) + `MsgAddMarkerRequest` 100 hash (100,000,000,000 nhash)
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (5)
CHANGELOG.md (5)
Line range hint
103-348: Consider using a consistent style for unordered lists throughout the document. The current mix of asterisks and dashes can be unified to improve readability and maintain consistency.- - + *
Line range hint
326-348: Multiple consecutive blank lines found. It's a good practice to limit consecutive blank lines to one to maintain code cleanliness and readability.- +
Line range hint
222-222: Bare URLs are used in the document. It's recommended to use descriptive link text instead of bare URLs to improve accessibility and readability.- https://github.com/provenance-io/provenance/issues/105 + [Issue #105](https://github.com/provenance-io/provenance/issues/105)Also applies to: 253-253, 352-352, 402-402, 414-414, 430-430, 501-501, 512-512, 520-520, 556-556, 581-581, 593-593, 639-643, 690-690, 731-731, 784-784, 897-897
Line range hint
939-939: Spaces found inside emphasis markers. This can lead to rendering issues in some markdown parsers.- * message * + *message*
Line range hint
335-335: Spaces found inside code span elements. This can lead to unintended text formatting.- ` code ` + `code`
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- x/ibchooks/ibc_middleware_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/ibchooks/ibc_middleware_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/ibchooks/ibc_middleware_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/ibchooks/ibc_middleware_test.go
Description
Fixes tests for ibchooks.
Related to: #1760
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passesSummary by CodeRabbit
Bug Fixes
ibcratelimitandibchooks.New Features
RateLimitMiddlewarefor enhanced rate limiting.FungibleTokenPacketDatain IBC Middleware.Improvements
Documentation
CHANGELOG.mdwith recent fixes and improvements.