-
Notifications
You must be signed in to change notification settings - Fork 634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: untracked tokenfactory ibc-rate-limit inflow #8922
fix: untracked tokenfactory ibc-rate-limit inflow #8922
Conversation
WalkthroughThe pull request introduces a new test function Changes
Sequence DiagramsequenceDiagram
participant Test as Test Function
participant Packet as Packet Struct
participant LocalDenom as local_denom Method
Test->>Packet: Create Packet with factory token denom
Packet->>LocalDenom: Call local_denom method
LocalDenom-->>Packet: Return processed denom
Test->>Test: Assert expected denom matches result
The sequence diagram illustrates the test flow, showing how the test function creates a packet, calls 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 (
|
This reverts commit f92d330.
…hub.com:osmosis-labs/osmosis into fix/untracked-tokenfactor-ibc-rate-limit-inflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/ibc-rate-limit/contracts/rate-limiter/src/contract_tests.rs (2)
411-411
: Consider improving test function name and documentation.While the function name indicates the version, it could be more descriptive about what it's testing. Consider adding documentation to explain:
- The purpose of the migration test
- The steps being tested
- The expected outcomes
-fn proper_migrate_for_v0_1_0() { +/// Tests the migration process from version 0.1.0, specifically verifying: +/// - Proper contract version setting +/// - Role revocation and reassignment for governance contract +/// - Maintenance of contract state during migration +fn test_migration_maintains_roles_from_v0_1_0() {
430-431
: Consider adding version verification after migration.The test sets the contract version but doesn't verify if it's updated after migration. Consider adding an assertion to ensure the version is properly updated.
// force set contract version to 0.1.0 set_contract_version(deps.as_mut().storage, "crates.io:rate-limiter", "0.1.0").unwrap(); + +// verify initial version +let version = cw2::get_contract_version(deps.as_ref().storage).unwrap(); +assert_eq!(version.version, "0.1.0");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
x/ibc-rate-limit/Cargo.lock
is excluded by!**/*.lock
,!**/*.lock
x/ibc-rate-limit/bytecode/rate_limiter.wasm
is excluded by!**/*.wasm
,!**/*.wasm
📒 Files selected for processing (1)
x/ibc-rate-limit/contracts/rate-limiter/src/contract_tests.rs
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: e2e
- GitHub Check: go-split-test-files
- GitHub Check: Test Cosmwasm Contracts (./cosmwasm/, tests/ibc-hooks/bytecode/outpost.wasm, artifacts/outpost.wa...
- GitHub Check: Test Cosmwasm Contracts (./cosmwasm/, tests/ibc-hooks/bytecode/swaprouter.wasm, artifacts/swaprou...
- GitHub Check: Test Cosmwasm Contracts (./cosmwasm/, tests/ibc-hooks/bytecode/crosschain_swaps.wasm, artifacts/c...
- GitHub Check: Optimize Cosmwasm Contracts (./cosmwasm/, tests/ibc-hooks/bytecode/outpost.wasm, artifacts/outpos...
- GitHub Check: Optimize Cosmwasm Contracts (./cosmwasm/, tests/ibc-hooks/bytecode/swaprouter.wasm, artifacts/swa...
- GitHub Check: Optimize Cosmwasm Contracts (./cosmwasm/, tests/ibc-hooks/bytecode/crosschain_swaps.wasm, artifac...
- GitHub Check: Optimize Cosmwasm Contracts (./x/ibc-rate-limit/, x/ibc-rate-limit/bytecode/rate_limiter.wasm, ar...
- GitHub Check: Summary
🔇 Additional comments (2)
x/ibc-rate-limit/contracts/rate-limiter/src/contract_tests.rs (2)
8-8
: LGTM! Good practice using cw2 for contract versioning.Using
cw2
for contract versioning is a standard practice in CosmWasm and helps with proper migration management.
442-442
: LGTM! Good test coverage for role reassignment.The test properly verifies that roles are reassigned during migration by:
- Explicitly revoking all roles
- Performing migration
- Verifying all roles are reassigned
What is the purpose of the change
Recv denom doesn't detect tokenfactory token as native and hashed it as ibc token so inflow is untracked. This PR fixes it.