⚡️ [API-2293] Swap Directly On Astroport Pools, Deprecate Router Use #84
Conversation
WalkthroughThe codebase underwent a significant shift away from using a router contract for swap operations in favor of direct swaps on pool contracts. This was achieved through refactoring functions, altering message structures, and removing redundant code. The changes streamlined swap-related contracts, enhancing their direct interaction with Astroport pools and eliminating the intermediate router layer. Changes
Assessment against linked issues
Poem
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's AI:
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 (
|
NotJeremyLiu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (7)
- contracts/adapters/swap/astroport/src/contract.rs (10 hunks)
- contracts/adapters/swap/astroport/src/state.rs (1 hunks)
- contracts/adapters/swap/astroport/tests/test_execute_receive.rs (5 hunks)
- contracts/adapters/swap/astroport/tests/test_execute_swap.rs (7 hunks)
- contracts/adapters/swap/lido-satellite/src/contract.rs (1 hunks)
- contracts/adapters/swap/osmosis-poolmanager/src/contract.rs (1 hunks)
- packages/skip/src/swap.rs (2 hunks)
Files skipped from review due to trivial changes (1)
- contracts/adapters/swap/astroport/tests/test_execute_receive.rs
Additional comments: 14
contracts/adapters/swap/astroport/src/state.rs (1)
- 2-4: Removal of
ROUTER_CONTRACT_ADDRESSis consistent with the PR's objective to deprecate router use.contracts/adapters/swap/astroport/tests/test_execute_swap.rs (2)
54-63: The addition of
AstroportPoolSwapmessage in the test cases is consistent with the PR's objective to enable direct swaps on Astroport pools.154-160: Verify the implementation of the
SwapOperationsEmptyerror to ensure it is correctly handled in the contract logic.Verification successful
The
SwapOperationsEmptyerror is implemented and used in various parts of the codebase, including theastroportadapter's contract logic and tests, as well as in theskippackage. The test case intest_execute_swap.rscorrectly expects this error when no swap operations are provided, which aligns with the error handling implemented in the contract logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of SwapOperationsEmpty error handling. rg --type rust 'SwapOperationsEmpty'Length of output: 2009
contracts/adapters/swap/astroport/src/contract.rs (9)
3-7: The removal of the router contract address and the addition of
ENTRY_POINT_CONTRACT_ADDRESSalign with the PR objectives to deprecate router use and enable direct swaps on Astroport pools.56-64: The instantiate function now correctly stores the
ENTRY_POINT_CONTRACT_ADDRESSinstead of the router contract address, which is consistent with the PR objectives.87-91: The
receive_cw20function has been updated to handle theSwapoperation directly, which is in line with the PR's goal to enable direct swaps on Astroport pools.108-109: The
executefunction now includes a check for a single coin in the message info, which is a good practice to prevent unexpected multiple coin transfers.176-207: The new function
execute_astroport_pool_swapis added to handle direct pool swaps, which is a key part of the PR objectives. Ensure that this function is thoroughly tested, especially the authorization logic and the swap message creation.141-154: The
execute_swapfunction now creates a response object for each swap operation, which is a change in the logic to support direct pool swaps. It's important to ensure that this change does not introduce any regressions.168-173: The
assert_max_spreadfunction remains unchanged, which is good as it's a critical part of ensuring that the swap does not exceed the maximum allowed slippage.214-219: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [214-255]
The query function has been updated to remove the
RouterContractAddressvariant, which is consistent with the PR's objective to deprecate router use.
- 254-260: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [257-298]
The function
query_simulate_swap_exact_asset_inhas been updated to remove the router logic, which aligns with the PR's objective to enable direct swaps on Astroport pools.packages/skip/src/swap.rs (2)
58-58: The addition of the
AstroportPoolSwapvariant to theExecuteMsgenum is necessary for the new direct swap functionality on Astroport pools.72-77: The removal of the
RouterContractAddressvariant from theQueryMsgenum is consistent with the PR's objective to deprecate router use.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- scripts/configs/neutron.toml
- scripts/configs/terra.toml
Files selected for processing (1)
- scripts/deploy.py (1 hunks)
Additional comments: 2
scripts/deploy.py (2)
- 151-156: The conditional block checking for the "lido_satellite_contract_address" in the
venuedictionary has been retained. This is consistent with the PR objectives, as it does not relate to the router contract address, which is being deprecated. Ensure that the "lido_satellite_contract_address" is still relevant to the new architecture and that its usage here is intentional and correct.Verification successful
The search for
lido_satellite_contract_addressin Python files only returned occurrences in thescripts/deploy.pyfile, which is the same file we are reviewing. There are no other references to this address in the Python codebase provided by the search results. This suggests that thelido_satellite_contract_addressis not widely used or referenced in other parts of the Python codebase, at least not under this exact name.Given this information, it seems that the
lido_satellite_contract_addressis still relevant to the deployment script and its usage in thedeploy.pyscript is intentional. However, without further context on how this address is used within the broader application, we cannot fully confirm its relevance to the new architecture.* 151-156: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-157]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the "lido_satellite_contract_address" is still in use and relevant to the new architecture. rg --type py 'lido_satellite_contract_address'Length of output: 257
The removal of the router contract address check in the
mainfunction is consistent with the PR objectives to deprecate router use. However, it's crucial to ensure that this change does not introduce any regressions or unintended side effects elsewhere in the deployment script. Verify that the router contract address is not referenced or required in any other part of the script or related modules.Verification successful
The search for references to
router_contract_addressin the deployment script did not yield any results. This suggests that the removal of the router contract address check from themainfunction indeploy.pyhas not left any lingering references to the router contract address in the script.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the router contract address in the deployment script. rg --type py 'router_contract_address'Length of output: 38
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- contracts/adapters/swap/astroport/src/contract.rs (10 hunks)
- contracts/adapters/swap/astroport/src/error.rs (1 hunks)
- contracts/adapters/swap/astroport/tests/test_execute_astroport_pool_swap.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- contracts/adapters/swap/astroport/src/error.rs
Files skipped from review as they are similar to previous changes (1)
- contracts/adapters/swap/astroport/src/contract.rs
Additional comments: 8
contracts/adapters/swap/astroport/tests/test_execute_astroport_pool_swap.rs (8)
1-17: The imports and module usage have been updated to reflect the new functionality for direct swaps on Astroport pools. Ensure that these changes are consistent with the rest of the codebase and that no unused imports remain.
32-39: The
Paramsstruct is well-defined and includes all necessary fields for testing theexecute_astroport_pool_swapfunction. It's important to ensure that the fields cover all scenarios that the function might encounter in production.42-74: The test case for "Native Swap Operation" appears to be correctly set up with the expected message and no error. It's important to ensure that the test case covers the scenario comprehensively and that the expected message matches the actual behavior of the contract.
76-111: The test case for "Cw20 Swap Operation" is set up to test the CW20 token swap. Ensure that the contract address and the amount are correctly set up and that the expected behavior aligns with the contract's logic.
113-124: The test case for "No Native Offer Asset In Contract Balance To Swap" correctly expects an error when there is no native offer asset in the contract balance. This is a good negative test case to ensure proper error handling in the contract.
126-137: The test case for "No Cw20 Offer Asset In Contract Balance To Swap" also correctly expects an error under the condition that there is no CW20 offer asset in the contract balance. This negative test case is essential for ensuring the contract's robustness.
139-152: The test case for "Unauthorized Caller" is designed to ensure that the contract correctly handles unauthorized access. The expected error is
ContractError::Unauthorized, which should be verified against the contract's authorization logic.154-234: The
test_execute_astroport_pool_swapfunction sets up the test environment and executes theexecutefunction with various parameters. Ensure that the mock dependencies, environment, and assertions are correctly implemented to accurately test the contract's behavior.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- contracts/adapters/swap/astroport/README.md (3 hunks)
- contracts/adapters/swap/astroport/src/bin/astroport-schema.rs (1 hunks)
- contracts/adapters/swap/astroport/src/contract.rs (10 hunks)
- contracts/adapters/swap/lido-satellite/tests/test_execute_swap.rs (1 hunks)
- contracts/adapters/swap/osmosis-poolmanager/src/bin/osmosis-poolmanager-schema.rs (1 hunks)
- contracts/adapters/swap/osmosis-poolmanager/src/contract.rs (2 hunks)
- packages/skip/src/swap.rs (3 hunks)
- scripts/deploy.ipynb (1 hunks)
Files skipped from review due to trivial changes (3)
- contracts/adapters/swap/astroport/src/bin/astroport-schema.rs
- contracts/adapters/swap/lido-satellite/tests/test_execute_swap.rs
- scripts/deploy.ipynb
Additional comments: 13
contracts/adapters/swap/osmosis-poolmanager/src/bin/osmosis-poolmanager-schema.rs (1)
- 2-2: The removal of
OsmosisInstantiateMsgfrom the import statement aligns with the PR objectives and the summary provided. Ensure that this change does not affect any other parts of the codebase whereOsmosisInstantiateMsgmight be used.contracts/adapters/swap/astroport/README.md (1)
- 4-5: The updates to the README.md file correctly reflect the new functionality of the contract, which now handles direct swaps with Astroport pools. This is consistent with the PR objectives.
contracts/adapters/swap/osmosis-poolmanager/src/contract.rs (1)
- 19-21: The changes to the import statements are in line with the PR objectives and the summary provided. The removal of
OsmosisInstantiateMsgand the addition ofInstantiateMsgandMigrateMsgreflect the updated functionality of the contract.packages/skip/src/swap.rs (3)
26-31: Renaming
OsmosisInstantiateMsgtoInstantiateMsgand removingAstroportInstantiateMsgare consistent with the PR objectives to streamline swap operations.50-50: Adding the
AstroportPoolSwapvariant to theExecuteMsgenum aligns with the new functionality for direct pool swaps.65-68: The removal of
RouterContractAddressfrom theQueryMsgenum is in line with the deprecation of router use, as described in the PR objectives.contracts/adapters/swap/astroport/src/contract.rs (7)
3-22: Reorganizing imports and removing unused ones, such as
state::ENTRY_POINT_CONTRACT_ADDRESS, is a good practice for maintainability and clarity of the code.55-63: Refactoring the
instantiatefunction to remove the storage of the router contract address is consistent with the PR's goal to deprecate router use.86-91: Modifying the
receive_cw20function to handle swap operations without the need for asset validation simplifies the contract logic in line with direct pool swaps.104-111: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [104-123]
Refactoring the
executefunction to handle different execution messages, including the newAstroportPoolSwapmessage, is necessary for the new functionality.
- 137-156: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [126-153]
The implementation of
execute_swapto create a response object and add an astroport pool swap message for each swap operation is a key part of the new direct swap functionality.
175-212: Introducing the
execute_astroport_pool_swapfunction to handle the newAstroportPoolSwapmessage is a crucial part of the contract's new responsibilities.218-223: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [218-259]
Removing the
RouterContractAddressquery and related functionality is in line with the PR's objective to remove router usage.
This PR
test_execute_astroport_pool_swap.rsfile to unit test the new swapping dispatching.Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Chores
Documentation
Style
Tests