Fix Serde Json Wasm issue #16
Conversation
WalkthroughThe recent updates involve version upgrades, bug fixes, and feature additions across various files. Version Changes
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
Cargo.lockis excluded by:!**/*.lockcontracts/swap/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (1)
- Changelog.md (2 hunks)
Additional comments: 2
Changelog.md (2)
- 19-23: The update for version
1.0.2is clearly documented, focusing on the bump in version ofserde-json-wasm. This change is crucial for maintaining compatibility and possibly enhancing serialization and deserialization efficiencies within the project's WebAssembly components.- 25-31: The documentation for version
1.0.1effectively captures the introduction of the Changelog file,rust-toolchainfile, contract migration templates, and a migrate function. Additionally, the fix for correctly rounding tomin_quantity_tick_sizein intermediate steps for multi-hop buys is a significant improvement, addressing precision issues.
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- contracts/swap/msg.rs (1 hunks)
- contracts/swap/src/testing/integration_tests/integration_logic_tests.rs (1 hunks)
- contracts/swap/src/testing/integration_tests/integration_realistic_tests_exact_quantity.rs (1 hunks)
- contracts/swap/src/testing/integration_tests/integration_realistic_tests_min_quantity.rs (1 hunks)
- contracts/swap/src/testing/integration_tests/mod.rs (1 hunks)
Files not summarized due to errors (3)
- contracts/swap/src/testing/integration_tests/integration_logic_tests.rs: Error: Message exceeds token limit
- contracts/swap/src/testing/integration_tests/integration_realistic_tests_exact_quantity.rs: Error: Message exceeds token limit
- contracts/swap/src/testing/integration_tests/integration_realistic_tests_min_quantity.rs: Error: Message exceeds token limit
Additional comments not posted (34)
contracts/swap/msg.rs (1)
1-70: The changes inmsg.rsare well-structured and follow best practices for defining message handling in a CosmWasm contract. The use of serialization attributes, custom types, and clear struct and enum definitions contribute to a maintainable and understandable codebase. Ensure that all new types and operations are covered by unit tests to verify their correctness and behavior.contracts/swap/src/testing/integration_tests/integration_realistic_tests_min_quantity.rs (6)
38-228: The testhappy_path_two_hops_swap_eth_atom_realistic_values_self_relayingis well-structured and covers a comprehensive set of assertions to validate the swap functionality. However, consider adding more detailed comments explaining the rationale behind specific values used in the test, especially those derived from external sources or calculations. This will enhance maintainability and understandability for future contributors.
230-419: The testhappy_path_two_hops_swap_inj_eth_realistic_values_self_relayingfollows a similar pattern to the first test, effectively testing the swap functionality from INJ to ETH. It's crucial to ensure that the hardcoded values, especially those related to market conditions and expected outcomes, are kept up-to-date with realistic scenarios. Regular updates or a dynamic way to fetch these values might be necessary to keep the tests relevant.
422-612: Inhappy_path_two_hops_swap_inj_atom_realistic_values_self_relaying, the assertions for swap results and contract balances are clear and concise. To further improve test robustness, consider scenarios where market conditions might lead to unexpected results, such as insufficient liquidity or price slippage, and ensure these cases are handled gracefully in the test or the underlying code.
615-829: The testit_executes_swap_between_markets_using_different_quote_assets_self_relayingintroduces a scenario with different quote assets, adding complexity to the swap process. It's commendable that the test checks for precise amounts post-swap and contract balance integrity. To enhance the test, consider edge cases where conversion rates between quote assets significantly fluctuate during the swap process, potentially affecting the expected outcomes.
832-968: The testit_doesnt_lose_buffer_if_executed_multiple_timesis crucial for ensuring that repeated swap executions do not lead to unexpected loss of funds or accumulation of dust. The loop construct used to simulate multiple swaps is effective. However, it's important to also consider the impact of transaction fees on the contract's balance over multiple iterations, as this could affect the accuracy of the test's assertions.
1281-1415: The testit_returns_all_funds_if_there_is_not_enough_buffer_realistic_valueseffectively simulates a scenario where the swap cannot be executed due to insufficient funds. This test is crucial for ensuring the integrity of user funds in failure scenarios. To further enhance this test, consider adding more varied scenarios of insufficient funds, including cases with different assets and market conditions.contracts/swap/src/testing/integration_tests/integration_realistic_tests_exact_quantity.rs (6)
23-37: The introductory comment block provides a good overview of the test suite's purpose and the source of the market parameters. However, it could be enhanced by briefly explaining the significance of the tests and how they contribute to ensuring the contract's reliability and functionality.Consider expanding the comment to include a brief explanation of how these tests contribute to the overall reliability and functionality of the contract.
41-254: The testsit_swaps_eth_to_get_minimum_exact_amount_of_atom_by_mildly_rounding_upthroughit_swaps_inj_to_get_minimum_exact_amount_of_atom_by_mildly_rounding_upfollow a similar pattern, calling a template function with different parameters. This approach is good for reducing code duplication and ensuring consistency across tests. However, there's an opportunity to further improve these tests by adding comments that explain the significance of each test case, especially the rationale behind the chosen parameters.Consider adding comments to each test case to explain the significance of the chosen parameters and what specific scenario or edge case is being tested. This will improve the readability and maintainability of the test suite.
68-252: The testit_correctly_swaps_eth_to_get_very_high_exact_amount_of_atomand similar tests for swapping INJ to ATOM and INJ to ETH are comprehensive and cover a wide range of operations, including initializing accounts with funds, launching markets, setting up orders, and executing swaps. These tests are well-structured and provide a thorough examination of the swap functionality under realistic conditions. However, there's a potential improvement in how the expected outcomes are verified, particularly in asserting the exact amounts received and the contract's balance changes.Consider adding more granular assertions to verify the state of the system after the swap operations. For example, in addition to checking the balances of the swapper and the contract, you could assert the state of the order book to ensure that orders were matched as expected. This would provide a more comprehensive validation of the swap functionality.
687-895: The testit_correctly_swaps_between_markets_using_different_quote_assets_self_relayingdemonstrates a complex scenario involving swapping between markets with different quote assets. This test is crucial for verifying the contract's ability to handle swaps across different market types. The setup and execution of the test are well done, but there's a minor opportunity for improvement in the clarity of the test's purpose and the specific conditions it aims to validate.Enhance the clarity of the test by adding a comment at the beginning that explicitly states the purpose of the test and the specific conditions or edge cases it aims to validate. This will help future maintainers understand the test's intent more quickly.
898-1032: The testit_doesnt_lose_buffer_if_exact_swap_of_eth_to_atom_is_executed_multiple_timesis an important test for ensuring the contract's robustness over multiple iterations of the same swap operation. The use of a loop to execute the swap multiple times is a good approach to simulate repeated operations. However, there's an opportunity to improve the test by verifying the state of the system after each iteration, rather than only at the end.Consider adding assertions within the loop to verify the state of the system after each swap operation. This could include checking the balances of the swapper and the contract, as well as the state of the order book. This would provide a more detailed insight into how the system behaves over multiple iterations and help catch any issues that might only manifest after several operations.
1035-1172: The testit_reverts_when_funds_provided_are_below_required_to_get_exact_amounteffectively verifies that the contract correctly reverts when the provided funds are insufficient for the desired swap. This test is crucial for ensuring the contract's security and preventing unintended behavior. The structure and execution of the test are well done. However, there's a minor opportunity to improve the clarity of the error message assertion.Consider using a more specific assertion method or library function that directly checks for the presence of the expected error message, rather than using
containson the error's string representation. This would make the test more robust and less susceptible to changes in the error formatting.contracts/swap/src/testing/integration_tests/integration_logic_tests.rs (21)
31-171: The testit_executes_a_swap_between_two_base_assets_with_multiple_price_levelsis comprehensive, covering the setup, execution, and validation of a swap between two base assets with multiple price levels. However, consider adding more detailed comments explaining the rationale behind each step and assertion to improve maintainability and readability for future developers.
173-292: The testit_executes_a_swap_between_two_base_assets_with_single_price_leveleffectively validates the swap functionality with a single price level. To enhance the test's clarity and maintainability, consider adding comments that detail the test's purpose and the significance of each assertion.
295-434: The testit_executes_swap_between_markets_using_different_quote_assetsis well-structured and tests an important scenario. However, it would benefit from additional comments explaining the setup and expected outcomes, especially regarding the handling of different quote assets and fees, to aid in understanding and future maintenance.
437-564: The testit_reverts_swap_between_markets_using_different_quote_asset_if_one_quote_buffer_is_insufficientcorrectly simulates a failure scenario due to an insufficient quote buffer. Enhancing this test with more detailed comments explaining the setup and expected failure conditions would improve its readability and maintainability.
567-687: The testit_executes_a_sell_of_base_asset_to_receive_min_output_quantityeffectively validates the sell operation with a minimum output quantity. Adding comments to explain the logic behind the calculations and assertions would make the test more accessible to future developers.
689-851: The testit_executes_a_buy_of_base_asset_to_receive_min_output_quantityis well-implemented, testing a crucial functionality. To further improve the test, consider adding detailed comments explaining the rationale behind the setup and the significance of each assertion, especially in the context of the buy operation and minimum output quantity.
853-1037: The testit_executes_a_swap_between_base_assets_with_external_fee_recipienteffectively validates the scenario with an external fee recipient. It would be beneficial to include comments detailing the fee calculation and distribution logic to enhance the test's clarity and maintainability.
1039-1151: The testit_reverts_the_swap_if_there_isnt_enough_buffer_for_buying_target_assetcorrectly simulates a failure scenario due to insufficient buffer for buying the target asset. Enhancing this test with more detailed comments explaining the setup and expected failure conditions would improve its readability and maintainability.
1153-1237: The testit_reverts_swap_if_no_funds_were_passedeffectively validates the contract's input validation mechanism. To improve the test's clarity, consider adding comments that detail the expected behavior when no funds are passed and the significance of the specific error message checked in the assertion.
1239-1329: The testit_reverts_swap_if_multiple_funds_were_passedis crucial for ensuring the contract's input validation works as expected when multiple funds are passed. Adding detailed comments explaining the rationale behind this input validation rule and the expected error message would enhance the test's maintainability.
1331-1432: The testit_reverts_if_user_passes_quantities_equal_to_zerocorrectly simulates a failure scenario due to invalid input values. To improve the test's clarity and maintainability, consider adding comments that detail the significance of testing for quantities equal to zero and the rationale behind the specific error messages checked.
1434-1517: The testit_reverts_if_user_passes_negative_quantitieseffectively validates the contract's handling of negative input quantities. Including comments explaining why passing negative quantities should result in a revert and the importance of the specific error message would make the test more informative and maintainable.
1519-1627: The testit_reverts_if_there_arent_enough_orders_to_satisfy_min_quantityis important for ensuring the contract's liquidity checks are functioning correctly. Adding comments to explain the setup, especially how the orders are created to simulate insufficient liquidity, and the rationale behind the expected error message would enhance the test's clarity.
1629-1718: The testit_reverts_if_min_quantity_cannot_be_reachedcorrectly simulates a scenario where the minimum quantity cannot be reached due to liquidity constraints. To improve the test's maintainability, consider adding detailed comments explaining the setup and the significance of the specific error message checked in the assertion.
1721-1823: The testit_reverts_if_market_is_pausedeffectively validates the contract's behavior when attempting to swap in a paused market. Including comments that detail the process of pausing the market and the rationale behind the expected error message would enhance the test's clarity and maintainability.
1825-1926: The testit_reverts_if_user_doesnt_have_enough_inj_to_pay_for_gasis crucial for ensuring the contract's fee handling works as expected. Adding comments to explain the setup, especially how the user's INJ balance is set to simulate insufficient funds for gas, and the significance of the specific error message would make the test more informative and maintainable.
1928-2017: The testit_reverts_if_target_quantity_is_not_multiple_of_min_quantity_tick_sizecorrectly validates an important input validation rule regarding quantity tick size. To improve the test's clarity, consider adding comments explaining the significance of the minimum quantity tick size and the rationale behind the specific error message checked in the assertion.
2020-2079: The testit_allows_admin_to_withdraw_all_funds_from_contract_to_his_addresseffectively validates an important administrative function of the contract. Adding comments to explain the setup, especially the initialization of the contract with specific balances, and the significance of each assertion would enhance the test's clarity and maintainability.
2081-2143: The testit_allows_admin_to_withdraw_all_funds_from_contract_to_other_addressis well-implemented, testing an important aspect of the contract's administrative functions. To further improve the test, consider adding detailed comments explaining the rationale behind allowing withdrawals to other addresses and the significance of each assertion.
2145-2209: The testit_doesnt_allow_non_admin_to_withdraw_anything_from_contractcorrectly validates the contract's access control mechanisms. Including comments that detail the setup, especially the distinction between the admin and a non-admin user, and the rationale behind the expected failure would make the test more informative and maintainable.
2211-2233: Utility functionscreate_eth_buy_ordersandcreate_atom_sell_ordersare used to set up test scenarios by creating buy and sell orders, respectively. These functions are well-implemented, but adding comments explaining the parameters (e.g.,market_id,OrderSide, and order quantities) would improve readability and maintainability.
| &trader1, | ||
| &trader2, | ||
| ); | ||
| create_realistic_atom_usdt_sell_orders_from_spreadsheet( | ||
| &app, | ||
| &spot_market_2_id, | ||
| &trader1, | ||
| &trader2, | ||
| &trader3, | ||
| ); | ||
|
|
||
| app.increase_time(1); | ||
|
|
||
| let eth_to_swap = "4.08"; | ||
|
|
||
| let swapper = must_init_account_with_funds( | ||
| &app, | ||
| &[ | ||
| str_coin(eth_to_swap, ETH, Decimals::Eighteen), | ||
| str_coin("1", INJ, Decimals::Eighteen), | ||
| ], | ||
| ); | ||
|
|
||
| let query_result: FPDecimal = wasm | ||
| .query( | ||
| &contr_addr, | ||
| &QueryMsg::GetOutputQuantity { | ||
| source_denom: ETH.to_string(), | ||
| target_denom: ATOM.to_string(), | ||
| from_quantity: human_to_dec(eth_to_swap, Decimals::Eighteen), | ||
| }, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!( | ||
| query_result, | ||
| human_to_dec("906.195", Decimals::Six), | ||
| "incorrect swap result estimate returned by query" | ||
| ); | ||
|
|
||
| let contract_balances_before = query_all_bank_balances(&bank, &contr_addr); | ||
| assert_eq!( | ||
| contract_balances_before.len(), | ||
| 1, | ||
| "wrong number of denoms in contract balances" | ||
| ); | ||
|
|
||
| wasm.execute( | ||
| &contr_addr, | ||
| &ExecuteMsg::SwapMinOutput { | ||
| target_denom: ATOM.to_string(), | ||
| min_output_quantity: FPDecimal::from(906u128), | ||
| }, | ||
| &[str_coin(eth_to_swap, ETH, Decimals::Eighteen)], | ||
| &swapper, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let from_balance = query_bank_balance(&bank, ETH, swapper.address().as_str()); | ||
| let to_balance = query_bank_balance(&bank, ATOM, swapper.address().as_str()); | ||
| assert_eq!( | ||
| from_balance, | ||
| FPDecimal::ZERO, | ||
| "some of the original amount wasn't swapped" | ||
| ); | ||
| assert_eq!( | ||
| to_balance, | ||
| human_to_dec("906.195", Decimals::Six), | ||
| "swapper did not receive expected amount" | ||
| ); | ||
|
|
||
| let contract_balances_after = query_all_bank_balances(&bank, contr_addr.as_str()); | ||
| assert_eq!( | ||
| contract_balances_after.len(), | ||
| 1, | ||
| "wrong number of denoms in contract balances" | ||
| ); | ||
|
|
||
| let atom_amount_below_min_tick_size = FPDecimal::must_from_str("0.0005463"); | ||
| let mut dust_value = atom_amount_below_min_tick_size * human_to_dec("8.89", Decimals::Six); | ||
|
|
||
| let fee_refund = dust_value | ||
| * FPDecimal::must_from_str(&format!( | ||
| "{}", | ||
| DEFAULT_TAKER_FEE * DEFAULT_ATOMIC_MULTIPLIER * DEFAULT_SELF_RELAYING_FEE_PART | ||
| )); | ||
|
|
||
| dust_value += fee_refund; | ||
|
|
||
| let expected_contract_usdt_balance = | ||
| FPDecimal::must_from_str(contract_balances_before[0].amount.as_str()) + dust_value; | ||
| let actual_contract_balance = | ||
| FPDecimal::must_from_str(contract_balances_after[0].amount.as_str()); | ||
| let contract_balance_diff = expected_contract_usdt_balance - actual_contract_balance; | ||
|
|
||
| // here the actual difference is 0.000067 USDT, which we attribute differences between decimal precision of Rust/Go and Google Sheets | ||
| assert!( | ||
| human_to_dec("0.0001", Decimals::Six) - contract_balance_diff > FPDecimal::ZERO, | ||
| "contract balance has changed too much after swap" | ||
| ); |
There was a problem hiding this comment.
The ignored test it_correctly_calculates_required_funds_when_querying_buy_with_minimum_buffer_and_realistic_values highlights an important aspect of swap functionality related to fund estimation. While the test is currently ignored, it's essential to address the underlying issue causing the discrepancy in fund estimation. This could involve adjusting the query logic or ensuring that the test setup accurately reflects real-world conditions.
| vec![ | ||
| spot_market_1_id.as_str().into(), | ||
| spot_market_2_id.as_str().into(), | ||
| ], | ||
| ); | ||
|
|
||
| let trader1 = init_rich_account(&app); | ||
| let trader2 = init_rich_account(&app); | ||
| let trader3 = init_rich_account(&app); | ||
|
|
||
| create_realistic_eth_usdt_buy_orders_from_spreadsheet( | ||
| &app, | ||
| &spot_market_1_id, | ||
| &trader1, | ||
| &trader2, | ||
| ); | ||
| create_realistic_atom_usdt_sell_orders_from_spreadsheet( | ||
| &app, | ||
| &spot_market_2_id, | ||
| &trader1, | ||
| &trader2, | ||
| &trader3, | ||
| ); | ||
|
|
||
| app.increase_time(1); | ||
|
|
||
| let eth_to_swap = "4.08"; | ||
|
|
||
| let swapper = must_init_account_with_funds( | ||
| &app, | ||
| &[ | ||
| str_coin(eth_to_swap, ETH, Decimals::Eighteen), | ||
| str_coin("0.01", INJ, Decimals::Eighteen), | ||
| ], | ||
| ); | ||
|
|
||
| let contract_balances_before = query_all_bank_balances(&bank, &contr_addr); | ||
| assert_eq!( | ||
| contract_balances_before.len(), | ||
| 1, | ||
| "wrong number of denoms in contract balances" | ||
| ); | ||
|
|
||
| wasm.execute( | ||
| &contr_addr, | ||
| &ExecuteMsg::SwapMinOutput { | ||
| target_denom: ATOM.to_string(), | ||
| min_output_quantity: FPDecimal::from(906u128), | ||
| }, | ||
| &[str_coin(eth_to_swap, ETH, Decimals::Eighteen)], | ||
| &swapper, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let from_balance = query_bank_balance(&bank, ETH, swapper.address().as_str()); | ||
| let to_balance = query_bank_balance(&bank, ATOM, swapper.address().as_str()); | ||
| assert_eq!( | ||
| from_balance, | ||
| FPDecimal::ZERO, | ||
| "some of the original amount wasn't swapped" | ||
| ); | ||
| assert_eq!( | ||
| to_balance, | ||
| human_to_dec("906.195", Decimals::Six), | ||
| "swapper did not receive expected amount" | ||
| ); | ||
|
|
||
| let contract_balances_after = query_all_bank_balances(&bank, contr_addr.as_str()); | ||
| assert_eq!( | ||
| contract_balances_after.len(), | ||
| 1, | ||
| "wrong number of denoms in contract balances" | ||
| ); | ||
|
|
||
| let contract_usdt_balance_before = | ||
| FPDecimal::must_from_str(contract_balances_before[0].amount.as_str()); | ||
| let contract_usdt_balance_after = | ||
| FPDecimal::must_from_str(contract_balances_after[0].amount.as_str()); | ||
|
|
||
| assert!( | ||
| contract_usdt_balance_after >= contract_usdt_balance_before, | ||
| "Contract lost some money after swap. Actual balance: {}, previous balance: {}", | ||
| contract_usdt_balance_after, | ||
| contract_usdt_balance_before | ||
| ); | ||
|
|
||
| // contract can earn max of 0.7 USDT, when exchanging ETH worth ~$8150 | ||
| let max_diff = human_to_dec("0.7", Decimals::Six); | ||
|
|
||
| assert!( | ||
| are_fpdecimals_approximately_equal( | ||
| contract_usdt_balance_after, | ||
| contract_usdt_balance_before, | ||
| max_diff, | ||
| ), | ||
| "Contract balance changed too much. Actual balance: {}, previous balance: {}. Max diff: {}", | ||
| contract_usdt_balance_after.scaled(Decimals::Six.get_decimals().neg()), | ||
| contract_usdt_balance_before.scaled(Decimals::Six.get_decimals().neg()), | ||
| max_diff.scaled(Decimals::Six.get_decimals().neg()) | ||
| ); |
There was a problem hiding this comment.
Similar to the previous ignored test, it_correctly_calculates_required_funds_when_executing_buy_with_minimum_buffer_and_realistic_values is critical for validating the swap execution with minimal buffer. The discrepancy in required funds estimation needs to be thoroughly investigated and resolved to ensure the swap functionality behaves as expected under tight fund conditions.
| @@ -2,7 +2,7 @@ | |||
| authors = [ "Markus Waas <[email protected]>" ] | |||
| edition = "2021" | |||
| name = "swap-contract" | |||
| version = "1.0.1" | |||
| version = "1.0.2" | |||
There was a problem hiding this comment.
Version changed, check migration constrains to check if still make sense.
added authz test for setting the route
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (4)
README.md (3)
Line range hint
1-24: Clarify documentation on contract instantiation.The section on contract instantiation is somewhat vague, particularly regarding the version of the contract uploaded to the Injective Mainnet. The phrase "There is version of this contract uploaded to the Injective Mainnet" is missing an article before "version". It should be corrected to "There is a version of this contract uploaded to the Injective Mainnet" for better clarity.
Line range hint
17-24: Improve clarity and conciseness in route setting description.The description of how routes are set is verbose and could be made more concise. Additionally, the phrase "so that the it knows the route" contains a typographical error and should be corrected to "so that it knows the route".
87-94: Add a comma for better readability.In the example command under the "Authz Permission" section, consider adding a comma after "Example" for better readability. It should be "Example, how to allow the contract admin to authorize other addresses to make contract calls on their behalf."
.github/workflows/rust.yaml (1)
53-59: Ensure optimal build flags for WASM compilation.The build flags for compiling WASM contracts have been set to optimize the build. However, it's important to ensure that these flags are indeed producing the optimal build for the specific needs of your project. Consider reviewing these flags periodically to adapt to changes in the project requirements or updates in the toolchain.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (8)
- .github/workflows/rust.yaml (1 hunks)
- Cargo.toml (1 hunks)
- README.md (1 hunks)
- contracts/swap/Cargo.toml (3 hunks)
- contracts/swap/src/contract.rs (1 hunks)
- contracts/swap/src/testing/authz_tests.rs (1 hunks)
- contracts/swap/src/testing/mod.rs (1 hunks)
- contracts/swap/src/testing/test_utils.rs (6 hunks)
Files skipped from review due to trivial changes (4)
- Cargo.toml
- contracts/swap/Cargo.toml
- contracts/swap/src/contract.rs
- contracts/swap/src/testing/mod.rs
Additional Context Used
LanguageTool (9)
README.md (9)
Near line 7: Possible missing article found.
Context: ...instance of the Swap contract. There is version of this contract uploaded to the Inject...
Near line 17: Replace ‘token’ with an appropriate verb.
Context: ...fined. Route means which markets should token A go through in order to get token B. F...
Near line 17: Consider a shorter alternative to avoid wordiness.
Context: ...which markets should token A go through in order to get token B. For example, if you would ...
Near line 17: Please verify. Did you mean “it” or “the IT” (= information technology)?
Context: ... IDs of ATOM/USDT and INJ/USDT, so that the it knows the route of swapping ATOM and IN...
Near line 18: For conciseness, consider replacing this expression with an adverb.
Context: ... and INJ would be ATOM <> USDT <> INJ. At this moment, the contract can only support markets ...
Near line 23: After ‘it’, use the third-person verb form “places”.
Context: ... buffer is used by the contract when it place orders. If the user wants to swap a big...
Near line 23: The word ‘big’ tends to be overused. Consider an alternative.
Context: ...ace orders. If the user wants to swap a big amount or swap in an illiquid market, then mor...
Near line 24: For conciseness, consider replacing this expression with an adverb.
Context: ...annot satisfy the user's input amount. At this moment, the buffer should only be in USDT. ##...
Near line 88: Possible missing comma found.
Context: ...esult ``` ## Authz Permission Example how to allow the contract admin to auth...
Additional comments not posted (9)
contracts/swap/src/testing/test_utils.rs (9)
1-1: Added import forScaledfromcrate::helpers.This import is necessary for the new functionality introduced in this file.
3-3: Changed import fromcosmos_sdk_proto::prost::Messagetoprost::Message.This change simplifies the import path and is consistent with Rust best practices for using external crates.
18-36: Reorganized imports forinjective_std.The reorganization improves readability and maintainability by grouping related imports together.
37-39: Reorganized imports forinjective_test_tube.Similar to the previous comment, this change enhances the structure and clarity of the import statements.
40-40: Added imports forHashMapandFromStr.These imports are necessary for the new functionality related to handling collections and parsing strings.
316-331: Modifiedwasm_filefunction to handle different architectures and directories.This modification increases the flexibility and robustness of the function, allowing it to adapt to different build environments and directory structures.
756-756: Addedcidparameter toOrderInfoincreate_limit_order.This addition is necessary for the new functionality related to contract interaction, allowing unique identifiers for orders.
819-819: Addedcidparameter toOrderInfoincreate_realistic_limit_order.Consistent with the previous change, this addition supports new features related to order management in the testing environment.
961-1021: Addedcreate_contract_authorizationfunction.This new function enhances the testing capabilities by allowing the simulation of contract authorization scenarios, which is crucial for comprehensive contract testing.
| #[test] | ||
| pub fn set_route_for_third_party_test() { | ||
| let app = InjectiveTestApp::new(); | ||
| let wasm = Wasm::new(&app); | ||
| let authz = Authz::new(&app); | ||
| let exchange = Exchange::new(&app); | ||
|
|
||
| let owner = must_init_account_with_funds( | ||
| &app, | ||
| &[ | ||
| str_coin("1", ETH, Decimals::Eighteen), | ||
| str_coin("1", ATOM, Decimals::Six), | ||
| str_coin("1_000", USDT, Decimals::Six), | ||
| str_coin("10_000", INJ, Decimals::Eighteen), | ||
| ], | ||
| ); | ||
|
|
||
| let spot_market_1_id = launch_realistic_weth_usdt_spot_market(&exchange, &owner); | ||
| let spot_market_2_id = launch_realistic_atom_usdt_spot_market(&exchange, &owner); | ||
|
|
||
| let contr_addr = init_self_relaying_contract_and_get_address( | ||
| &wasm, | ||
| &owner, | ||
| &[str_coin("1_000", USDT, Decimals::Six)], | ||
| ); | ||
|
|
||
| let trader1 = init_rich_account(&app); | ||
| let trader2 = init_rich_account(&app); | ||
| let trader3 = init_rich_account(&app); | ||
|
|
There was a problem hiding this comment.
Ensure proper cleanup and resource management in tests.
While the test function set_route_for_third_party_test initializes several accounts and contracts, it does not explicitly clean up or release resources after the test execution. This might lead to resource leakage or conflicts in subsequent tests. Consider adding teardown steps or using context managers to ensure resources are properly cleaned up.
| create_contract_authorization( | ||
| &app, | ||
| contr_addr.clone(), | ||
| &owner, | ||
| trader1.address().to_string(), | ||
| "set_route".to_string(), | ||
| 1, | ||
| None, | ||
| ); | ||
|
|
||
| create_realistic_eth_usdt_buy_orders_from_spreadsheet( | ||
| &app, | ||
| &spot_market_1_id, | ||
| &trader1, | ||
| &trader2, | ||
| ); | ||
| create_realistic_atom_usdt_sell_orders_from_spreadsheet( | ||
| &app, | ||
| &spot_market_2_id, | ||
| &trader1, | ||
| &trader2, | ||
| &trader3, | ||
| ); |
There was a problem hiding this comment.
Refactor to improve readability and maintainability.
The segments handling authorization and order creation are quite verbose and involve repeated code, especially in the handling of trader accounts and market setups. Consider refactoring these into helper functions or using loops to handle repetitive tasks, which would improve both readability and maintainability.
| let set_route_msg = ExecuteMsg::SetRoute { | ||
| source_denom: ETH.to_string(), | ||
| target_denom: ATOM.to_string(), | ||
| route: vec![ | ||
| spot_market_1_id.as_str().into(), | ||
| spot_market_2_id.as_str().into(), | ||
| ], | ||
| }; | ||
|
|
||
| let execute_msg = MsgExecuteContract { | ||
| contract: contr_addr.clone(), | ||
| sender: owner.address().to_string(), | ||
| msg: serde_json_wasm::to_vec(&set_route_msg).unwrap(), | ||
| funds: vec![], | ||
| }; | ||
|
|
||
| // execute on more time to excercise account sequence | ||
| let msg = MsgExec { | ||
| grantee: trader1.address().to_string(), | ||
| msgs: vec![Any { | ||
| type_url: "/cosmwasm.wasm.v1.MsgExecuteContract".to_string(), | ||
| value: execute_msg.to_bytes().unwrap(), | ||
| }], | ||
| }; | ||
|
|
||
| let _res = authz.exec(msg, &trader1).unwrap(); | ||
|
|
||
| // execute on more time to excercise account sequence | ||
| let msg = MsgExec { | ||
| grantee: trader1.address().to_string(), | ||
| msgs: vec![Any { | ||
| type_url: "/cosmwasm.wasm.v1.MsgExecuteContract".to_string(), | ||
| value: execute_msg.to_bytes().unwrap(), | ||
| }], | ||
| }; | ||
|
|
||
| let err = authz.exec(msg, &trader1).unwrap_err(); | ||
| assert!( | ||
| err.to_string().contains("failed to update grant with key"), | ||
| "incorrect error returned by execute" | ||
| ); |
There was a problem hiding this comment.
Add error handling for JSON serialization.
The test function uses serde_json_wasm::to_vec(&set_route_msg).unwrap() for JSON serialization without handling potential serialization errors. This could cause the test to panic if serialization fails. It's recommended to handle these errors gracefully to avoid test crashes and to provide more informative error messages.
See changes for more important information that I am omitting from my context description here............
Summary by CodeRabbit
migratefunction within the contract.Cargo.tomlrelated to resolver, release profiles, and dependencies.