F/swap routes fix #19
Conversation
maxrobot
had a problem deploying
to
github-pages
GitHub Actions
Failure
WalkthroughThe pull request introduces several updates across multiple files related to a Rust project. Key changes include the upgrade of the Rust toolchain version to 1.78.0 in workflow and configuration files, restructuring of dependencies in Changes
Poem
🪧 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 (
|
maxrobot
had a problem deploying
to
github-pages
GitHub Actions
Failure
maxrobot
had a problem deploying
to
github-pages
GitHub Actions
Failure
maxrobot
had a problem deploying
to
github-pages
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (12)
Changelog.md (1)
23-24: Consider adding the specific version of serde-json-wasm.While the changelog entry about bumping serde-json-wasm is good, it would be more helpful to include the specific version numbers for future reference.
- Added pagination to `get_all_routes` -- Bump version of `serde-json-wasm` +- Bump version of `serde-json-wasm` from X.Y.Z to A.B.Ccontracts/swap/src/msg.rs (1)
66-69: Add documentation for pagination parameters.The pagination implementation follows CosmWasm patterns, but could benefit from documentation explaining:
- The format of
start_aftertuple (presumably source_denom, target_denom)- The default and maximum values for
limitAdd documentation like this:
GetAllRoutes { + /// Optional pagination cursor, where (source_denom, target_denom) tuple + /// represents the last route from the previous query start_after: Option<(String, String)>, + /// Optional limit for the number of routes to return. + /// If not specified, a reasonable default will be used. limit: Option<u32>, },contracts/swap/src/state.rs (1)
43-58: LGTM: Well-implemented pagination with a minor suggestion.The pagination implementation follows best practices and handles edge cases correctly. Consider adding a comment explaining the sorting order of the routes for better maintainability.
pub fn get_all_swap_routes( storage: &dyn Storage, start_after: Option<(String, String)>, limit: Option<u32>, ) -> StdResult<Vec<SwapRoute>> { let limit = limit.unwrap_or(DEFAULT_LIMIT) as usize; let start_bound = start_after .as_ref() .map(|(s, t)| Bound::inclusive((s.clone(), t.clone()))); + // Routes are sorted by (source_denom, target_denom) in ascending order let routes = SWAP_ROUTES .range(storage, start_bound, None, Order::Ascending) .take(limit) .map(|item| item.map(|(_, route)| route)) .collect::<StdResult<Vec<SwapRoute>>>()?;contracts/swap/src/testing/authz_tests.rs (1)
38-38: Consider breaking long lines for better readabilityWhile condensing these initialization calls improves vertical space usage, the resulting lines are quite long. Consider breaking them at logical points if they exceed your project's line length guidelines.
- let contr_addr = init_self_relaying_contract_and_get_address(&wasm, &owner, &[str_coin("1_000", USDT, Decimals::Six)]); + let contr_addr = init_self_relaying_contract_and_get_address( + &wasm, + &owner, + &[str_coin("1_000", USDT, Decimals::Six)] + );Also applies to: 54-55
contracts/swap/src/testing/migration_test.rs (2)
10-10: Consider splitting the long import line for better readability.The import line is quite long and imports multiple types. Consider splitting it into multiple lines for better readability.
-use injective_std::types::cosmwasm::wasm::v1::{MsgMigrateContract, MsgMigrateContractResponse, QueryContractInfoRequest, QueryContractInfoResponse}; +use injective_std::types::cosmwasm::wasm::v1::{ + MsgMigrateContract, + MsgMigrateContractResponse, + QueryContractInfoRequest, + QueryContractInfoResponse, +};
Line range hint
36-48: Improve error handling and add documentation for initial funds.
- The code uses multiple
unwrap()calls which could panic. Consider using proper error handling.- The purpose of initializing with 1000 USDT should be documented.
let swap_v101_address: String = wasm .instantiate( swap_v101_code_id, &V101InstantiateMsg { admin: Addr::unchecked(owner.address()), fee_recipient: FeeRecipient::SwapContract, }, Some(&owner.address()), Some("swap-contract"), + // Initialize with 1000 USDT for <document purpose here> &[str_coin("1_000", USDT, Decimals::Six)], &owner, ) - .unwrap() + .expect("Failed to instantiate swap contract") .data .address;.github/workflows/rust.yaml (1)
Line range hint
64-64: Fix inconsistent toolchain version in test step.The test step is still using toolchain version 1.74.0 while other steps have been updated to 1.78.0. This inconsistency could lead to different behavior between linting and testing environments.
Apply this diff to maintain consistency:
- toolchain: 1.74.0 + toolchain: 1.78.0🧰 Tools
🪛 actionlint
33-33: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
contracts/swap/src/swap.rs (1)
78-78: Enhance insufficient funds error messageConsider adding more context to help users understand the exact shortfall.
- return Err(ContractError::InsufficientFundsProvided(fp_coins, required_input)); + return Err(ContractError::InsufficientFundsProvided( + fp_coins, + required_input, + format!("Required {} {}, but only {} {} provided", + required_input, + source_denom, + fp_coins, + source_denom + ) + ));contracts/swap/src/testing/queries_tests.rs (1)
533-533: Enhance pagination test coverage.While the basic pagination test is good, consider adding more test cases to verify:
- Different page sizes
- Using
start_afterparameter- Edge cases (empty results, last page)
- Consistency of results across multiple paginated requests
Example test cases:
// Test with start_after parameter let second_page = get_all_swap_routes(deps.as_ref().storage, Some("eth_inj".to_string()), Some(1u32)); assert_eq!(second_page.unwrap(), vec![eth_usdt_route]); // Test with larger page size let larger_page = get_all_swap_routes(deps.as_ref().storage, None, Some(2u32)); assert_eq!(larger_page.unwrap(), vec![eth_inj_route, eth_usdt_route]); // Test last page let last_page = get_all_swap_routes(deps.as_ref().storage, Some("eth_usdt".to_string()), Some(2u32)); assert_eq!(last_page.unwrap(), vec![usdt_inj_route]);Also applies to: 610-611
contracts/swap/src/testing/test_utils.rs (1)
984-986: Add documentation for the approximate equality function.Consider adding documentation to explain the purpose and usage of this utility function, including what the
max_diffparameter represents.+/// Compares two FPDecimal values for approximate equality within a maximum difference. +/// +/// # Arguments +/// * `first` - First FPDecimal value to compare +/// * `second` - Second FPDecimal value to compare +/// * `max_diff` - Maximum allowed absolute difference between the values +/// +/// # Returns +/// `true` if the absolute difference between the values is less than or equal to `max_diff` pub fn are_fpdecimals_approximately_equal(first: FPDecimal, second: FPDecimal, max_diff: FPDecimal) -> bool { (first - second).abs() <= max_diff }contracts/swap/src/testing/integration_realistic_tests_exact_quantity.rs (2)
Line range hint
1116-1124: Consider extracting common balance check logic.The balance check logic with max diff assertions is repeated across all test templates. This could be extracted into a helper function for better maintainability and reduced code duplication.
Consider creating a helper function:
fn assert_balance_within_max_diff( actual: FPDecimal, expected: FPDecimal, max_diff: FPDecimal, token: &str, decimals: Decimals, ) { assert!( are_fpdecimals_approximately_equal(actual, expected, max_diff), "Contract balance changed too much. Actual balance: {} {}, previous balance: {} {}. Max diff: {} {}", actual.scaled(decimals.get_decimals().neg()), token, expected.scaled(decimals.get_decimals().neg()), token, max_diff.scaled(decimals.get_decimals().neg()), token ); }Also applies to: 1238-1246, 1359-1367
1115-1115: Document magic numbers in comments.The comments mention specific USDT amounts (0.7, 0.82) allowed for contract earnings, but don't explain how these values were determined. Consider adding more detailed documentation about the calculation of these thresholds.
Add more detailed comments explaining the threshold calculations:
- // contract is allowed to earn extra 0.7 USDT from the swap of ~$8150 worth of ETH + // Contract fee calculation: + // - Swap value: ~$8150 worth of ETH + // - Expected fee: 0.1% of swap value = $8.15 + // - Maximum allowed fee in USDT (considering price impact): 0.7 USDTAlso applies to: 1237-1237, 1358-1358
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockcontracts/swap/src/testing/test_artifacts/swap-contract-v100.wasmis excluded by!**/*.wasmcontracts/swap/src/testing/test_artifacts/swap_contract-v101.wasmis excluded by!**/*.wasm
📒 Files selected for processing (25)
.github/workflows/rust.yaml(3 hunks)Cargo.toml(1 hunks)Changelog.md(1 hunks)Makefile.toml(0 hunks)contracts/swap/.gitignore(0 hunks)contracts/swap/.gitpod.Dockerfile(0 hunks)contracts/swap/.gitpod.yml(0 hunks)contracts/swap/Cargo.toml(2 hunks)contracts/swap/rustfmt.toml(0 hunks)contracts/swap/src/contract.rs(5 hunks)contracts/swap/src/msg.rs(3 hunks)contracts/swap/src/queries.rs(19 hunks)contracts/swap/src/state.rs(2 hunks)contracts/swap/src/swap.rs(12 hunks)contracts/swap/src/testing/authz_tests.rs(5 hunks)contracts/swap/src/testing/config_tests.rs(5 hunks)contracts/swap/src/testing/integration_logic_tests.rs(0 hunks)contracts/swap/src/testing/integration_realistic_tests_exact_quantity.rs(61 hunks)contracts/swap/src/testing/migration_test.rs(5 hunks)contracts/swap/src/testing/mod.rs(0 hunks)contracts/swap/src/testing/queries_tests.rs(22 hunks)contracts/swap/src/testing/test_utils.rs(39 hunks)contracts/swap/src/types.rs(6 hunks)docs/admin.md(1 hunks)rust-toolchain.toml(1 hunks)
💤 Files with no reviewable changes (7)
- Makefile.toml
- contracts/swap/.gitignore
- contracts/swap/.gitpod.Dockerfile
- contracts/swap/.gitpod.yml
- contracts/swap/rustfmt.toml
- contracts/swap/src/testing/integration_logic_tests.rs
- contracts/swap/src/testing/mod.rs
✅ Files skipped from review due to trivial changes (3)
- contracts/swap/src/queries.rs
- docs/admin.md
- rust-toolchain.toml
🧰 Additional context used
🪛 actionlint
.github/workflows/rust.yaml
131-131: the runner of "actions-rs/cargo@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (40)
contracts/swap/Cargo.toml (3)
5-5: LGTM: Version bump follows semantic versioning.
The version increment from 1.0.2 to 1.1.0 appropriately reflects the addition of pagination feature as a minor version bump.
23-34: LGTM: Clean workspace dependency management.
Moving to workspace-level dependency management is a good practice that will:
- Ensure consistent versions across the workspace
- Simplify dependency updates
- Reduce potential version conflicts
37-40: Verify the impact of testing framework changes.
The switch to injective-specific testing tools might affect existing tests. Let's verify the test coverage hasn't decreased.
✅ Verification successful
Based on the test files and dependencies shown, I can provide a final response:
Testing framework changes have been properly implemented
The codebase has successfully migrated to injective-specific testing tools:
- All tests use
injective-test-tube,injective-testing, andinjective-stdfor integration testing - Unit tests use
injective-cosmwasmmock infrastructure - No active usage of
cw-multi-testin test code, only appears as transitive dependency - Comprehensive test coverage maintained with both unit and integration tests covering core swap functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any orphaned or broken test imports after the testing framework change
# Look for any remaining old test framework usage that might cause conflicts
rg -l "cw-multi-test|cosmos-sdk-proto"
# Check for any test files that might need updates
fd -e rs --type f . | rg "test" | xargs rg "use (cw_multi_test|cosmos_sdk_proto)"
Length of output: 133
Script:
#!/bin/bash
# Let's verify the test coverage and framework usage more comprehensively
# Check for test files and their imports
fd -e rs "test" | xargs cat
# Check Cargo.lock for any remaining old dependencies
rg "cw-multi-test|cosmos-sdk-proto|cosmwasm-std" Cargo.lock
# Look for test attributes and test modules
ast-grep --pattern 'mod $_ {
#[cfg(test)]
$$$
}'
Length of output: 186941
Cargo.toml (3)
5-6: LGTM: Good choice using Rust 2021 edition.
The upgrade to Rust 2021 edition aligns well with the toolchain update and brings several improvements in cargo resolution and language features.
Line range hint 27-37: LGTM: Good cleanup of profile configuration.
The removal of package-specific overrides while maintaining strong security settings (overflow checks, panic=abort) in the release profile is a good improvement.
8-25: Consider pinning more dependency versions for better reproducibility.
While critical dependencies like injective-cosmwasm and test-tube-inj are correctly pinned, consider using exact versions for other dependencies to ensure consistent builds.
Additionally, verify the compatibility between these dependency versions:
✅ Verification successful
Based on the results, I can now generate the final response:
Dependency versions appear compatible and well-structured
The workspace dependencies are properly configured:
- No dependency conflicts or warnings were detected
- All injective-related packages in contracts correctly use workspace inheritance
- Version combinations are compatible:
injective-cosmwasm(0.3.0) andinjective-math(0.3.0) are alignedinjective-std(1.13.0) andinjective-test-tube(1.13.2) are in the same major version
While pinning more versions could improve reproducibility, the current setup is sound and follows good practices with workspace inheritance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential dependency conflicts and verify versions
# Test: Look for any dependency resolution warnings or conflicts
cargo check 2>&1 | grep -i "warning.*dependency\|conflict"
# Test: Verify injective-related package versions are compatible
rg "injective-" -A 1 $(fd -t f "Cargo.toml$")
Length of output: 1026
contracts/swap/src/msg.rs (3)
1-4: LGTM! Using cw_serde follows CosmWasm best practices.
The change to use cw_serde macro simplifies serialization by automatically deriving common traits.
6-7: LGTM! Consistent use of cw_serde across all message types.
The change uniformly applies cw_serde to all contract message types, improving code consistency and maintainability.
Also applies to: 12-13, 18-19, 21-22, 50-51
66-69: Verify pagination implementation details.
Please ensure:
- A maximum limit is enforced in the contract implementation
- Pagination functionality is covered by tests
Let's check the implementation and tests:
contracts/swap/src/state.rs (1)
3-4: LGTM: Appropriate imports and pagination limit.
The addition of Bound import and DEFAULT_LIMIT constant (100) are appropriate for implementing pagination functionality.
Also applies to: 12-12
contracts/swap/src/types.rs (4)
1-11: LGTM! Good use of cw_serde macro
The switch to cw_serde is a positive change as it's the recommended approach in CosmWasm for handling serialization/deserialization. The enum structure is clean and well-defined.
Line range hint 33-107: LGTM! Well-structured types with clear responsibilities
The types are well-organized with good separation of concerns. The SwapRoute implementation elegantly handles bidirectional routing by reversing steps when needed.
108-114: Verify pagination implementation for routes query
The PR objectives mention adding pagination for queries, but I don't see any pagination-related types in this file. We should verify where the pagination parameters are defined.
#!/bin/bash
# Look for pagination-related types and parameters
rg -A 5 "start_after|limit.*Option<|pagination"
# Check if pagination is implemented in query messages
ast-grep --pattern 'struct $QueryMsg {
$$$
start_after: $type,
limit: $type,
$$$
}'Line range hint 12-31: Verify FPDecimal to Coin conversion behavior
The removal of .into() in the conversion from FPCoin to Coin needs verification. Let's ensure that Coin::new can properly handle FPDecimal values directly.
contracts/swap/src/testing/config_tests.rs (5)
1-1: LGTM: Updated to use message_info instead of deprecated mock_info
The change aligns with modern testing practices in the Cosmos ecosystem.
20-20: LGTM: Improved readability with consolidated config save
The single-line save operation is more concise and clearer.
Line range hint 37-54: LGTM: Improved address comparison reliability
The addition of .to_string() ensures consistent string-based comparisons of addresses in the event attributes, preventing potential false negatives in tests.
66-71: LGTM: Consistent updates with the admin test
The changes mirror those in the admin test, maintaining consistency across test cases.
Line range hint 1-80: Verify config changes don't affect pagination functionality
While these config tests look good, let's verify there are no pagination-related config parameters that should be added given the PR's main objective.
✅ Verification successful
Based on the search results, I can see that pagination is already properly implemented in the codebase:
DEFAULT_LIMITconstant is defined instate.rs- Pagination parameters (
limit,start_after) are present in bothmsg.rsand handled incontract.rs - The config structure in the test focuses only on
adminandfee_recipientwhich are unrelated to pagination
No pagination-related config parameters needed
The config changes are correctly scoped to administrative controls (admin and fee_recipient). Pagination is handled separately through query parameters and doesn't require any configuration settings, following common patterns where pagination is controlled by API consumers rather than contract configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any pagination-related configurations or types in the codebase
rg -i "pagination|page_size|limit" --type rust
Length of output: 4764
contracts/swap/src/testing/authz_tests.rs (4)
5-7: LGTM! Import organization improves readability
The consolidation of imports, particularly the injective_std related ones, enhances code organization.
Also applies to: 11-14
76-76: LGTM! Message encoding is correct
The duplicate execution properly tests the authorization behavior with increasing sequence numbers.
Also applies to: 87-87
92-92: Verify authorization behavior change
The error message assertion has changed from checking "failed to update grant with key" to "failed to get grant with given granter". This suggests a modification in the underlying authorization behavior that should be verified.
#!/bin/bash
# Search for related authorization error handling
rg -A 3 "failed to (update|get) grant" --type rust62-62: Verify route market compatibility
The test sets up a route from ETH to ATOM through USDT markets, but there's no explicit validation that these markets form a valid route (i.e., compatible quote/base assets).
contracts/swap/src/testing/migration_test.rs (4)
Line range hint 50-65: LGTM! Thorough contract verification.
The verification checks all essential contract properties: address, code ID, creator, and label.
Line range hint 81-98: LGTM! Comprehensive post-migration verification.
The code properly verifies the contract state after migration and includes an additional functional test to ensure the contract works correctly after migration.
73-76: Verify if empty migration message is intended.
The migration uses an empty message (MigrateMsg {}). Please verify if this is intentional and if any state migration is needed between v101 and v110.
#!/bin/bash
# Search for MigrateMsg usage and implementation
ast-grep --pattern 'struct MigrateMsg {
$$$
}'14-14: Verify version naming consistency.
The type alias V101InstantiateMsg suggests this was previously a v100->v101 migration test, but the code now tests v101->v110 migration. Consider renaming this type alias to match the current test scenario.
✅ Verification successful
Type alias naming is consistent with test implementation
After reviewing the test file content, the type alias V101InstantiateMsg is correctly named as it represents the instantiate message for the v1.0.1 contract version that's being migrated. This is evidenced by:
- The test loads
swap-contract-v101.wasmbytecode - Uses
V101InstantiateMsgto instantiate that v1.0.1 contract - Then migrates it to v1.1.0 (code stored via
"swap_contract")
The naming convention accurately reflects the source version being tested in the migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other v101/v110 version references
rg -i "v10[0-9]|v110" --type rust
Length of output: 50
Script:
#!/bin/bash
# Let's check the content of the migration test file to understand the context better
cat contracts/swap/src/testing/migration_test.rs
# Also search for any migration-related types and messages
ast-grep --pattern 'type V$_ = $_'
Length of output: 3481
.github/workflows/rust.yaml (1)
36-38: LGTM on toolchain updates.
The Rust toolchain version updates to 1.78.0 in these sections are consistent and align with the PR's objective of updating dependencies.
Also applies to: 101-103
contracts/swap/src/contract.rs (5)
1-12: LGTM! Well-organized imports.
The imports are logically grouped and follow Rust best practices by using explicit imports.
Line range hint 44-64: LGTM! Clean and well-structured execute handlers.
The match arms are well-organized with admin functions grouped together. The code is concise and maintains clear error handling.
104-104: LGTM! Pagination implementation.
The GetAllRoutes query now properly supports pagination with start_after and limit parameters, aligning with the PR objectives.
77-77: LGTM! Consistent error handling.
The change to Result<Binary, StdError> aligns with Rust and CosmWasm best practices for error handling in query functions.
117-136: Verify migration path support requirements.
The migration function only supports upgrading from version "1.0.1". Please verify if support for older versions is required.
✅ Verification successful
Let me gather more information about the contract versions and migration paths.
Let me check the Cargo.toml for version information and test directory structure.
Let me check the migration test implementation for more details about the migration path.
Migration path support is correctly implemented
The migration implementation is appropriate and doesn't need modification. Here's why:
- Current contract version is 1.1.0 (from Cargo.toml)
- Migration path from 1.0.1 is explicitly tested with a test artifact (
swap-contract-v101.wasm) - The migration test verifies both the upgrade process and post-migration functionality
- No older versions exist that would require migration support
The strict version check in the migration function is intentional and matches the tested upgrade path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing deployments that might need migration support
echo "Checking for version references in tests and documentation..."
rg -A 2 "contract_version|set_contract_version"
echo "Checking for migration-related test cases..."
rg -A 5 "test.*migrat"
Length of output: 2725
Script:
#!/bin/bash
# Check contract version constants and related files
echo "Checking contract version constants..."
rg -A 2 "CONTRACT_VERSION|CONTRACT_NAME"
echo "Checking changelog or release history..."
fd -g "CHANGELOG*" -x cat {}
echo "Checking test artifacts directory..."
fd -g "*.wasm" -p "test_artifacts"
Length of output: 1519
Script:
#!/bin/bash
# Check Cargo.toml for version info
echo "Checking Cargo.toml..."
fd -g "Cargo.toml" -x cat {}
echo "Checking test directory structure..."
fd -t d "testing"
fd -t f -p "testing"
Length of output: 3310
Script:
#!/bin/bash
# Check migration test implementation
echo "Checking migration test content..."
cat contracts/swap/src/testing/migration_test.rs
echo "Checking contract error definitions..."
rg -A 2 "MigrationError"
Length of output: 4014
contracts/swap/src/swap.rs (2)
1-18: LGTM: Well-organized imports
The imports are logically grouped and follow Rust conventions. The new imports (prost::Message and injective_std types) appropriately support the protobuf message parsing functionality.
Line range hint 158-273: Verify test coverage for error cases
The changes to error handling are significant. Let's verify that tests cover these scenarios:
- Market not found cases
- Missing message responses
- Invalid protobuf messages
✅ Verification successful
Test coverage for error cases is comprehensive
The test files reveal extensive coverage of error cases and edge scenarios:
- Market-related error cases:
- Tests with realistic market parameters from mainnet
- Tests for different market pairs and quote assets
- Buffer insufficiency scenarios
- Message response handling:
- Tests for atomic order execution
- Tests for swap execution with minimum output quantities
- Tests for handling execution failures
- Protobuf message handling:
- Tests with realistic decimal values and scaling factors
- Tests for handling price/quantity/fee calculations
The test suite includes both unit tests and integration tests with realistic values, covering the error paths mentioned in the review comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for relevant test cases
echo "Searching for market-related test cases..."
rg -l "query_spot_market" contracts/swap/src/testing/
echo "Searching for reply handling test cases..."
rg -l "handle_atomic_order_reply" contracts/swap/src/testing/
echo "Checking test coverage for error cases..."
rg "ContractError::" contracts/swap/src/testing/
Length of output: 447
Script:
#!/bin/bash
# Let's first check if there's a test directory with a different path
fd -t d "test" contracts/swap/
# Let's also check for any test files directly
fd -e rs "test" contracts/swap/
# Let's search in the entire contract directory for test cases
rg -l "mod tests" contracts/swap/
# Let's also check for any test-related imports
rg -l "#\\[test\\]" contracts/swap/
Length of output: 1254
Script:
#!/bin/bash
# Let's check the content of swap_tests.rs and integration tests for error cases
echo "=== Checking swap_tests.rs for error handling ==="
rg "ContractError::" contracts/swap/src/testing/swap_tests.rs -B 2 -A 2
echo "=== Checking integration tests for error handling ==="
rg "ContractError::" contracts/swap/src/testing/integration_realistic_tests_min_quantity.rs -B 2 -A 2
rg "ContractError::" contracts/swap/src/testing/integration_realistic_tests_exact_quantity.rs -B 2 -A 2
echo "=== Checking for market-related test cases ==="
rg "query_spot_market" contracts/swap/src/testing/swap_tests.rs -B 2 -A 2
echo "=== Checking for reply handling test cases ==="
rg "handle_atomic_order_reply" contracts/swap/src/testing/swap_tests.rs -B 2 -A 2
Length of output: 865
Script:
#!/bin/bash
# Let's try a different approach to check test content
echo "=== Checking test content for error cases ==="
cat contracts/swap/src/testing/swap_tests.rs
echo "=== Checking integration test content ==="
cat contracts/swap/src/testing/integration_realistic_tests_min_quantity.rs
Length of output: 48338
contracts/swap/src/testing/queries_tests.rs (3)
4-4: LGTM! Import changes improve test reliability.
The switch from mock_info to message_info and the reorganized imports enhance the testing framework.
Also applies to: 17-18
35-35: LGTM! Consistent usage of message_info across test functions.
The systematic replacement of mock_info with message_info improves test reliability by using the more appropriate testing utility.
Also applies to: 110-110, 184-184, 255-255, 332-332, 436-436, 525-525, 547-547
87-89: LGTM! Well-structured assertions with clear error messages.
The assertions are well-formatted with:
- Appropriate use of
are_fpdecimals_approximately_equalfor floating-point comparisons - Descriptive error messages that aid in debugging
- Consistent formatting across all test cases
Also applies to: 94-96, 161-165, 168-172, 232-236, 239-243, 307-311, 314-318, 382-385, 405-407, 418-421, 486-489, 505-507, 510-513
contracts/swap/src/testing/test_utils.rs (1)
Line range hint 1-991: Overall code structure and implementation look good.
The test utilities are well-organized and provide comprehensive helper functions for testing the swap contract functionality. The test cases cover various scenarios and edge cases.
contracts/swap/src/testing/integration_realistic_tests_exact_quantity.rs (1)
977-982: 🛠️ Refactor suggestion
Simplify error message assertion.
The multi-line assertion can be simplified for better readability.
Consider consolidating the assertion:
- assert!(
- execute_result
- .to_string()
- .contains("Provided amount of 608000000000000000000 is below required amount of 609714000000000000000"),
- "wrong error message"
- );
+ assert!(execute_result.to_string().contains("Provided amount of 608000000000000000000 is below required amount of 609714000000000000000"), "wrong error message");Likely invalid or redundant comment.
| @@ -16,10 +16,11 @@ All notable changes to this project will be documented in this file. | |||
|
|
|||
| - | |||
|
|
|||
| ## [1.0.2] - 2024-03-19 | |||
| ## [1.1.0] - 2024-10-30 | |||
There was a problem hiding this comment.
Update the release date to the actual release date.
The release date is set to 2024-10-30, which is in the future. This could cause confusion for users tracking the changelog. Consider updating it to the actual release date when merging this PR.
-## [1.1.0] - 2024-10-30
+## [1.1.0] - 2024-10-XX📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## [1.1.0] - 2024-10-30 | |
| ## [1.1.0] - 2024-10-XX |
| serde = { version = "1.0.137", default-features = false, features = [ "derive" ] } | ||
| serde-json-wasm = "1.0.1" | ||
| thiserror = { version = "1.0.31" } | ||
| cosmwasm-schema = { workspace = true } |
There was a problem hiding this comment.
Consider moving cosmwasm-schema to dev-dependencies.
cosmwasm-schema is typically used only during development for generating JSON schema files and is not needed at runtime.
Apply this change:
[dependencies]
-cosmwasm-schema = { workspace = true }
[dev-dependencies]
+cosmwasm-schema = { workspace = true }Committable suggestion was skipped due to low confidence.
contracts/swap/src/state.rs
Outdated
| pub fn get_all_swap_routes( | ||
| storage: &dyn Storage, | ||
| start_after: Option<(String, String)>, | ||
| limit: Option<u32>, | ||
| ) -> StdResult<Vec<SwapRoute>> { | ||
| let limit = limit.unwrap_or(DEFAULT_LIMIT) as usize; | ||
|
|
||
| let start_bound = start_after | ||
| .as_ref() | ||
| .map(|(s, t)| Bound::inclusive((s.clone(), t.clone()))); | ||
|
|
||
| let routes = SWAP_ROUTES | ||
| .range(storage, None, None, Order::Ascending) | ||
| .map(|item| item.unwrap().1) | ||
| .collect(); | ||
| .range(storage, start_bound, None, Order::Ascending) | ||
| .take(limit) | ||
| .map(|item| item.map(|(_, route)| route)) // Extract the `SwapRoute` from each item | ||
| .collect::<StdResult<Vec<SwapRoute>>>()?; |
There was a problem hiding this comment.
💡 Codebase verification
Pagination implementation needs additional test coverage
The current test coverage only verifies the limit parameter with a basic test case. The start_after parameter is not tested, which could lead to undiscovered issues with cursor-based pagination. Key improvements needed:
- Add test cases for
start_afterparameter in pagination - Add test cases for edge cases (e.g., non-existent start_after values)
- Add test cases combining both
start_afterandlimitparameters
🔗 Analysis chain
Verify pagination implementation in query handlers and tests.
Let's verify that the pagination parameters are properly handled in the corresponding query handlers and tests.
Based on the results, I can see that both the query handler and test coverage for pagination are implemented. Let me verify the completeness of pagination testing by checking if there are tests for the start_after parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check query handler implementation and test coverage for pagination
# Check query handler implementation
echo "Checking query handler implementation..."
rg -A 10 "GetAllRoutes" contracts/swap/src/contract.rs
# Check test coverage
echo "Checking test coverage..."
rg -A 10 "get_all_swap_routes" contracts/swap/src/testing
Length of output: 3798
Script:
#!/bin/bash
# Check for start_after parameter testing
rg -B 5 -A 15 "start_after" contracts/swap/src/testing/queries_tests.rs
Length of output: 71
| let wasm_byte_code = | ||
| std::fs::read("../../contracts/swap/src/testing/test_artifacts/swap-contract-v100.wasm") | ||
| .unwrap(); | ||
| let wasm_byte_code = std::fs::read("../../contracts/swap/src/testing/test_artifacts/swap-contract-v101.wasm").unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more robust path resolution method.
The hardcoded path to the test artifact might break in different environments or when the project structure changes. Consider using environment variables or a path relative to the test file's location.
-let wasm_byte_code = std::fs::read("../../contracts/swap/src/testing/test_artifacts/swap-contract-v101.wasm").unwrap();
+let wasm_byte_code = std::fs::read(
+ std::path::Path::new(env!("CARGO_MANIFEST_DIR"))
+ .join("src/testing/test_artifacts/swap-contract-v101.wasm")
+).unwrap();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let wasm_byte_code = std::fs::read("../../contracts/swap/src/testing/test_artifacts/swap-contract-v101.wasm").unwrap(); | |
| let wasm_byte_code = std::fs::read( | |
| std::path::Path::new(env!("CARGO_MANIFEST_DIR")) | |
| .join("src/testing/test_artifacts/swap-contract-v101.wasm") | |
| ).unwrap(); |
| toolchain: 1.78.0 | ||
| command: fmt | ||
| args: --all -- --check | ||
|
|
||
| - name: Run cargo clippy | ||
| uses: actions-rs/cargo@v1 | ||
| with: | ||
| toolchain: 1.74.0 | ||
| toolchain: 1.78.0 | ||
| command: clippy | ||
| args: --tests -- -D warnings |
There was a problem hiding this comment.
Update the actions-rs/cargo action version.
The current version of actions-rs/cargo@v1 is outdated and may have compatibility issues with the latest GitHub Actions runners.
Consider updating to a newer version of the action or switching to alternative maintained actions. For example:
- - name: Run cargo fmt
- uses: actions-rs/cargo@v1
+ - name: Run cargo fmt
+ uses: dtolnay/rust-toolchain@stable
+ - run: cargo fmt --all -- --check
- - name: Run cargo clippy
- uses: actions-rs/cargo@v1
+ - name: Run cargo clippy
+ uses: dtolnay/rust-toolchain@stable
+ - run: cargo clippy --tests -- -D warningsThis change uses the more actively maintained dtolnay/rust-toolchain action.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toolchain: 1.78.0 | |
| command: fmt | |
| args: --all -- --check | |
| - name: Run cargo clippy | |
| uses: actions-rs/cargo@v1 | |
| with: | |
| toolchain: 1.74.0 | |
| toolchain: 1.78.0 | |
| command: clippy | |
| args: --tests -- -D warnings | |
| toolchain: 1.78.0 | |
| command: fmt | |
| args: --all -- --check | |
| - name: Run cargo clippy | |
| uses: dtolnay/rust-toolchain@stable | |
| - run: cargo clippy --tests -- -D warnings | |
| ``` | |
| Note: The suggestion shows only the visible part of the code snippet, but the complete change would also need to update the cargo fmt section above in a similar manner. The full implementation would look like: | |
| ```suggestion | |
| - name: Run cargo fmt | |
| uses: dtolnay/rust-toolchain@stable | |
| - run: cargo fmt --all -- --check | |
| - name: Run cargo clippy | |
| uses: dtolnay/rust-toolchain@stable | |
| - run: cargo clippy --tests -- -D warnings |
🧰 Tools
🪛 actionlint
131-131: the runner of "actions-rs/cargo@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| .query_spot_market(&first_market_id)? | ||
| .market | ||
| .expect("market should be available"); | ||
| let first_market = querier.query_spot_market(&first_market_id)?.market.expect("market should be available"); |
There was a problem hiding this comment.
Critical: Replace .expect() with proper error handling
Using .expect() in a smart contract can cause a panic, which should be avoided. Instead, handle the None case gracefully with a proper error return.
Replace with proper error handling:
- let first_market = querier.query_spot_market(&first_market_id)?.market.expect("market should be available");
+ let first_market = querier.query_spot_market(&first_market_id)?.market.ok_or_else(||
+ ContractError::CustomError {
+ val: format!("Market {} not found", first_market_id)
+ }
+ )?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let first_market = querier.query_spot_market(&first_market_id)?.market.expect("market should be available"); | |
| let first_market = querier.query_spot_market(&first_market_id)?.market.ok_or_else(|| | |
| ContractError::CustomError { | |
| val: format!("Market {} not found", first_market_id) | |
| } | |
| )?; |
| pub fn parse_market_order_response(msg: Reply) -> StdResult<MsgCreateSpotMarketOrderResponse> { | ||
| let binding = msg.result.into_result().map_err(ContractError::SubMsgFailure).unwrap(); | ||
|
|
||
| let first_messsage = binding.msg_responses.first(); | ||
|
|
||
| let order_response = MsgCreateSpotMarketOrderResponse::decode(first_messsage.unwrap().value.as_slice()) | ||
| .map_err(|err| ContractError::ReplyParseFailure { | ||
| id: msg.id, | ||
| err: err.to_string(), | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| Ok(order_response) | ||
| } |
There was a problem hiding this comment.
Critical: Replace unwrap() calls with proper error handling
The parse_market_order_response function contains multiple unwrap() calls that could cause panics. This is unsafe in a smart contract environment.
Refactor to handle errors properly:
pub fn parse_market_order_response(msg: Reply) -> StdResult<MsgCreateSpotMarketOrderResponse> {
- let binding = msg.result.into_result().map_err(ContractError::SubMsgFailure).unwrap();
-
- let first_messsage = binding.msg_responses.first();
-
- let order_response = MsgCreateSpotMarketOrderResponse::decode(first_messsage.unwrap().value.as_slice())
- .map_err(|err| ContractError::ReplyParseFailure {
- id: msg.id,
- err: err.to_string(),
- })
- .unwrap();
-
- Ok(order_response)
+ let binding = msg.result.into_result().map_err(ContractError::SubMsgFailure)?;
+
+ let first_message = binding.msg_responses.first().ok_or_else(||
+ ContractError::CustomError {
+ val: "No message responses in reply".to_string()
+ }
+ )?;
+
+ MsgCreateSpotMarketOrderResponse::decode(first_message.value.as_slice())
+ .map_err(|err| ContractError::ReplyParseFailure {
+ id: msg.id,
+ err: err.to_string(),
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn parse_market_order_response(msg: Reply) -> StdResult<MsgCreateSpotMarketOrderResponse> { | |
| let binding = msg.result.into_result().map_err(ContractError::SubMsgFailure).unwrap(); | |
| let first_messsage = binding.msg_responses.first(); | |
| let order_response = MsgCreateSpotMarketOrderResponse::decode(first_messsage.unwrap().value.as_slice()) | |
| .map_err(|err| ContractError::ReplyParseFailure { | |
| id: msg.id, | |
| err: err.to_string(), | |
| }) | |
| .unwrap(); | |
| Ok(order_response) | |
| } | |
| pub fn parse_market_order_response(msg: Reply) -> StdResult<MsgCreateSpotMarketOrderResponse> { | |
| let binding = msg.result.into_result().map_err(ContractError::SubMsgFailure)?; | |
| let first_message = binding.msg_responses.first().ok_or_else(|| | |
| ContractError::CustomError { | |
| val: "No message responses in reply".to_string() | |
| } | |
| )?; | |
| MsgCreateSpotMarketOrderResponse::decode(first_message.value.as_slice()) | |
| .map_err(|err| ContractError::ReplyParseFailure { | |
| id: msg.id, | |
| err: err.to_string(), | |
| }) | |
| } |
| actual, expected, | ||
| "failed to convert integer with 18 decimal to dec" | ||
| ); | ||
| assert_eq!(actual, expected, "failed to convert integer with 18 decimal to dec"); |
There was a problem hiding this comment.
Fix incorrect error message in test.
The error message mentions "18 decimal" but the test is actually checking for 6 decimals.
- assert_eq!(actual, expected, "failed to convert integer with 18 decimal to dec");
+ assert_eq!(actual, expected, "failed to convert integer with 6 decimal to dec");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert_eq!(actual, expected, "failed to convert integer with 18 decimal to dec"); | |
| assert_eq!(actual, expected, "failed to convert integer with 6 decimal to dec"); |
| pub fn launch_realistic_inj_usdt_spot_market(exchange: &Exchange<InjectiveTestApp>, signer: &SigningAccount) -> String { | ||
| launch_spot_market_custom( | ||
| exchange, | ||
| signer, | ||
| INJ_2, | ||
| USDT, | ||
| dec_to_proto(FPDecimal::must_from_str("0.000000000000001")).as_str(), | ||
| dec_to_proto(FPDecimal::must_from_str("1000000000000000")).as_str(), | ||
| "INJ2/USDT".to_string(), | ||
| INJ_2.to_string(), | ||
| USDT.to_string(), | ||
| "0.000000000000001".to_string(), | ||
| "1000000000000000".to_string(), | ||
| ) | ||
| } | ||
|
|
||
| pub fn launch_realistic_weth_usdt_spot_market( | ||
| exchange: &Exchange<InjectiveTestApp>, | ||
| signer: &SigningAccount, | ||
| ) -> String { | ||
| launch_custom_spot_market( | ||
| pub fn launch_realistic_weth_usdt_spot_market(exchange: &Exchange<InjectiveTestApp>, signer: &SigningAccount) -> String { | ||
| launch_spot_market_custom( | ||
| exchange, | ||
| signer, | ||
| ETH, | ||
| USDT, | ||
| dec_to_proto(FPDecimal::must_from_str("0.0000000000001")).as_str(), | ||
| dec_to_proto(FPDecimal::must_from_str("1000000000000000")).as_str(), | ||
| "ETH/USDT".to_string(), | ||
| ETH.to_string(), | ||
| USDT.to_string(), | ||
| "0.0000000000001".to_string(), | ||
| "1000000000000000".to_string(), | ||
| ) | ||
| } | ||
|
|
||
| pub fn launch_realistic_atom_usdt_spot_market( | ||
| exchange: &Exchange<InjectiveTestApp>, | ||
| signer: &SigningAccount, | ||
| ) -> String { | ||
| launch_custom_spot_market( | ||
| pub fn launch_realistic_atom_usdt_spot_market(exchange: &Exchange<InjectiveTestApp>, signer: &SigningAccount) -> String { | ||
| launch_spot_market_custom( | ||
| exchange, | ||
| signer, | ||
| ATOM, | ||
| USDT, | ||
| dec_to_proto(FPDecimal::must_from_str("0.001")).as_str(), | ||
| dec_to_proto(FPDecimal::must_from_str("10000")).as_str(), | ||
| "ATOM/USDT".to_string(), | ||
| ATOM.to_string(), | ||
| USDT.to_string(), | ||
| "0.001".to_string(), | ||
| "10000".to_string(), | ||
| ) | ||
| } | ||
|
|
||
| pub fn launch_realistic_usdt_usdc_spot_market( | ||
| exchange: &Exchange<InjectiveTestApp>, | ||
| signer: &SigningAccount, | ||
| ) -> String { | ||
| launch_custom_spot_market( | ||
| pub fn launch_realistic_usdt_usdc_spot_market(exchange: &Exchange<InjectiveTestApp>, signer: &SigningAccount) -> String { | ||
| launch_spot_market_custom( | ||
| exchange, | ||
| signer, | ||
| USDT, | ||
| USDC, | ||
| dec_to_proto(FPDecimal::must_from_str("0.0001")).as_str(), | ||
| dec_to_proto(FPDecimal::must_from_str("100")).as_str(), | ||
| "USDT/USDC".to_string(), | ||
| USDT.to_string(), | ||
| USDC.to_string(), | ||
| "0.0001".to_string(), | ||
| "100".to_string(), | ||
| ) | ||
| } | ||
|
|
||
| pub fn launch_realistic_ninja_inj_spot_market( | ||
| exchange: &Exchange<InjectiveTestApp>, | ||
| signer: &SigningAccount, | ||
| ) -> String { | ||
| launch_custom_spot_market( | ||
| pub fn launch_realistic_ninja_inj_spot_market(exchange: &Exchange<InjectiveTestApp>, signer: &SigningAccount) -> String { | ||
| launch_spot_market_custom( | ||
| exchange, | ||
| signer, | ||
| NINJA, | ||
| INJ_2, | ||
| dec_to_proto(FPDecimal::must_from_str("1000000")).as_str(), | ||
| dec_to_proto(FPDecimal::must_from_str("10000000")).as_str(), | ||
| "NINJA/INJ2".to_string(), | ||
| NINJA.to_string(), | ||
| INJ_2.to_string(), | ||
| "1000000".to_string(), | ||
| "10000000".to_string(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameterizing market configuration values.
The market launch functions (launch_realistic_*_spot_market) contain hardcoded values for tick sizes. Consider extracting these values into constants or making them configurable parameters for better reusability and maintainability.
-pub fn launch_realistic_inj_usdt_spot_market(exchange: &Exchange<InjectiveTestApp>, signer: &SigningAccount) -> String {
+pub fn launch_realistic_inj_usdt_spot_market(
+ exchange: &Exchange<InjectiveTestApp>,
+ signer: &SigningAccount,
+ min_price_tick_size: Option<String>,
+ min_quantity_tick_size: Option<String>,
+) -> String {
launch_spot_market_custom(
exchange,
signer,
"INJ2/USDT".to_string(),
INJ_2.to_string(),
USDT.to_string(),
- "0.000000000000001".to_string(),
- "1000000000000000".to_string(),
+ min_price_tick_size.unwrap_or_else(|| "0.000000000000001".to_string()),
+ min_quantity_tick_size.unwrap_or_else(|| "1000000000000000".to_string()),
)
}Committable suggestion was skipped due to low confidence.
| @@ -618,15 +489,11 @@ fn it_swaps_inj_to_get_very_high_exact_amount_of_eth() { | |||
| ) | |||
| .unwrap(); | |||
|
|
|||
| let expected_difference = | |||
| human_to_dec(inj_to_swap, Decimals::Eighteen) - query_result.result_quantity; | |||
| let expected_difference = human_to_dec(inj_to_swap, Decimals::Eighteen) - query_result.result_quantity; | |||
| let swapper_inj_balance_after = query_bank_balance(&bank, INJ_2, swapper.address().as_str()); | |||
| let swapper_atom_balance_after = query_bank_balance(&bank, ETH, swapper.address().as_str()); | |||
There was a problem hiding this comment.
Fix misleading variable name.
The variable name swapper_atom_balance_after is used to store ETH balance, which is misleading. Consider renaming it to swapper_eth_balance_after to match the actual token being checked.
Apply this change across similar instances in other test cases for consistency:
-let swapper_atom_balance_after = query_bank_balance(&bank, ETH, swapper.address().as_str());
+let swapper_eth_balance_after = query_bank_balance(&bank, ETH, swapper.address().as_str());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let swapper_atom_balance_after = query_bank_balance(&bank, ETH, swapper.address().as_str()); | |
| let swapper_eth_balance_after = query_bank_balance(&bank, ETH, swapper.address().as_str()); |
chore: lints
jbernal87
temporarily deployed
to
github-pages
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
contracts/swap/src/testing/swap_tests.rs (1)
Line range hint
1-45: Missing tests for pagination functionality.According to the PR objectives and AI summary, the
get_all_swap_routesfunction has been updated to support pagination with newstart_afterandlimitparameters. However, this test file doesn't include any tests to verify the pagination functionality.Would you like me to help generate test cases that cover:
- Pagination with different limit values
- Pagination with different start_after values
- Edge cases (empty results, last page, etc.)
- Validation of limit bounds
contracts/swap/src/state.rs (1)
12-12: Consider adding documentation for DEFAULT_LIMITThe constant value is reasonable, but adding a doc comment explaining the rationale for 100 would improve maintainability.
+/// Default maximum number of swap routes to return in a single query. pub const DEFAULT_LIMIT: u32 = 100u32;contracts/swap/src/admin.rs (2)
94-94: Consider optimizing the route duplication check.The current implementation could be more efficient:
- Avoid unnecessary
clone()by collecting directly intoHashSet- Use a more idiomatic error message format
Consider this optimization:
- if route.clone().into_iter().collect::<HashSet<MarketId>>().len() < route.len() { + let unique_steps: HashSet<_> = route.iter().collect(); + if unique_steps.len() < route.len() { return Err(ContractError::CustomError { - val: "Route cannot have duplicate steps!".to_string(), + val: "route contains duplicate market IDs".to_string(), }); }For the market query error:
- let market = querier.query_spot_market(market_id)?.market.ok_or(CustomError { - val: format!("Market {} not found", market_id.as_str()).to_string(), - })?; + let market = querier.query_spot_market(market_id)?.market + .ok_or_else(|| CustomError { + val: format!("market not found: {}", market_id) + })?;Also applies to: 120-122
138-138: Consider more robust error handling for market denom validation.While the
unwrap()calls are safe due to the prior empty check, we can make the code more robust and idiomatic.Consider this pattern-matching approach:
- ensure!( - denoms.first().unwrap().quote_denom == route.source_denom || denoms.first().unwrap().base_denom == route.source_denom, - CustomError { - val: "Source denom not found in first market".to_string() - } - ); + let first_market = denoms.first().ok_or_else(|| CustomError { + val: "no markets in route".to_string() + })?; + ensure!( + first_market.quote_denom == route.source_denom || first_market.base_denom == route.source_denom, + CustomError { + val: "source denom not found in first market".to_string() + } + ); - ensure!( - denoms.last().unwrap().quote_denom == route.target_denom || denoms.last().unwrap().base_denom == route.target_denom, - CustomError { - val: "Target denom not found in last market".to_string() - } - ); + let last_market = denoms.last().ok_or_else(|| CustomError { + val: "no markets in route".to_string() + })?; + ensure!( + last_market.quote_denom == route.target_denom || last_market.base_denom == route.target_denom, + CustomError { + val: "target denom not found in last market".to_string() + } + );Also applies to: 144-144
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.github/workflows/rust.yaml(4 hunks)contracts/swap/src/admin.rs(6 hunks)contracts/swap/src/helpers.rs(3 hunks)contracts/swap/src/state.rs(2 hunks)contracts/swap/src/testing/integration_realistic_tests_min_quantity.rs(46 hunks)contracts/swap/src/testing/storage_tests.rs(24 hunks)contracts/swap/src/testing/swap_tests.rs(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- contracts/swap/src/helpers.rs
- contracts/swap/src/testing/integration_realistic_tests_min_quantity.rs
- contracts/swap/src/testing/storage_tests.rs
🧰 Additional context used
🪛 actionlint
.github/workflows/rust.yaml
131-131: the runner of "actions-rs/cargo@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (8)
contracts/swap/src/testing/swap_tests.rs (1)
9-9: LGTM: Code formatting improvements look good.
The consolidation of imports and formatting changes improve readability while maintaining the same functionality.
Also applies to: 22-22, 43-43
contracts/swap/src/state.rs (2)
3-4: LGTM: Required imports for pagination
The added imports are necessary and correctly specified for implementing the pagination functionality.
37-46: Add input validation for limit parameter
While the pagination implementation is solid, consider adding validation for the limit parameter to prevent potential performance issues with very large values.
pub fn get_all_swap_routes(storage: &dyn Storage, start_after: Option<(String, String)>, limit: Option<u32>) -> StdResult<Vec<SwapRoute>> {
- let limit = limit.unwrap_or(DEFAULT_LIMIT) as usize;
+ let limit = limit.unwrap_or(DEFAULT_LIMIT);
+ if limit > 1000 {
+ return Err(StdError::generic_err("limit cannot exceed 1000"));
+ }
+ let limit = limit as usize;Additionally, the previous review comment about test coverage for pagination remains valid. Please ensure comprehensive testing of the start_after parameter.
.github/workflows/rust.yaml (2)
36-36: LGTM: Consistent toolchain version updates.
The Rust toolchain version has been consistently updated to 1.78.0 across all jobs and steps. This update aligns with the PR objective of updating dependencies.
Also applies to: 65-65, 101-101, 126-126, 133-133
126-135: Previous warning about outdated actions-rs/cargo action still applies.
The current version of actions-rs/cargo@v1 remains outdated and may have compatibility issues with the latest GitHub Actions runners, as confirmed by the actionlint tool.
#!/bin/bash
# Description: Verify if the repository is using other outdated actions-rs actions
# that might need similar updates
rg -l 'uses:\s+actions-rs/[^@]+@v1' .github/🧰 Tools
🪛 actionlint
131-131: the runner of "actions-rs/cargo@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
contracts/swap/src/admin.rs (3)
6-6: LGTM! Good use of validation macros.
The addition of ensure and ensure_eq macros from cosmwasm_std improves error handling readability.
10-10: LGTM! Improved function signature formatting.
The reformatted signatures enhance code readability while maintaining the same functionality.
Also applies to: 21-21
46-46: LGTM! Clean formatting.
The simplified attribute push improves code readability.
| pub fn read_swap_route(storage: &dyn Storage, source_denom: &str, target_denom: &str) -> StdResult<SwapRoute> { | ||
| let key = route_key(source_denom, target_denom); | ||
| SWAP_ROUTES.load(storage, key).map_err(|_| { | ||
| StdError::generic_err(format!( | ||
| "No swap route not found from {source_denom} to {target_denom}", | ||
| )) | ||
| }) | ||
| SWAP_ROUTES | ||
| .load(storage, key) | ||
| .map_err(|_| StdError::generic_err(format!("No swap route not found from {source_denom} to {target_denom}",))) |
There was a problem hiding this comment.
Fix typo in error message
There's a double negative in the error message that should be fixed.
- .map_err(|_| StdError::generic_err(format!("No swap route not found from {source_denom} to {target_denom}",)))
+ .map_err(|_| StdError::generic_err(format!("No swap route found from {source_denom} to {target_denom}")))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn read_swap_route(storage: &dyn Storage, source_denom: &str, target_denom: &str) -> StdResult<SwapRoute> { | |
| let key = route_key(source_denom, target_denom); | |
| SWAP_ROUTES.load(storage, key).map_err(|_| { | |
| StdError::generic_err(format!( | |
| "No swap route not found from {source_denom} to {target_denom}", | |
| )) | |
| }) | |
| SWAP_ROUTES | |
| .load(storage, key) | |
| .map_err(|_| StdError::generic_err(format!("No swap route not found from {source_denom} to {target_denom}",))) | |
| pub fn read_swap_route(storage: &dyn Storage, source_denom: &str, target_denom: &str) -> StdResult<SwapRoute> { | |
| let key = route_key(source_denom, target_denom); | |
| SWAP_ROUTES | |
| .load(storage, key) | |
| .map_err(|_| StdError::generic_err(format!("No swap route found from {source_denom} to {target_denom}"))) |
In this PR added is:
Off-topic:
Summary by CodeRabbit
Release Notes
New Features
get_all_routesfunction.Improvements
Bug Fixes
Chores