Feat/stargate tester contract #220
Conversation
|
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe updates involve significant changes to dependency management and the introduction of new functionalities across various smart contract projects. Key updates include the upgrade of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract
participant Exchange
User->>Contract: Initiate Spot Market Order
Contract->>Exchange: Create Spot Market Order Message
Exchange->>Contract: Spot Market Order Created
Contract->>User: Order Confirmation
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Out of diff range and nitpick comments (5)
contracts/injective-cosmwasm-stargate-example/src/reply.rs (2)
6-16: Ensure consistent use of field annotations for clarity.Consider adding comments to the fields
market_idandsubaccount_idto maintain consistency with the detailed comment on line 8.
19-29: Ensure consistent use of field annotations for clarity.Consider adding comments to the fields
market_idandsubaccount_idto maintain consistency with the detailed comment on line 21.contracts/injective-cosmwasm-stargate-example/src/utils.rs (2)
38-41: Constants for decimal places are defined globally.Consider moving these constants into a configuration file or environment variables for easier management and modification without needing to recompile the code.
678-688: Utility functions for number conversions.Consider adding more detailed comments explaining the purpose and usage of these utility functions to improve code readability and maintainability.
packages/injective-std/src/types/injective/exchange/v1beta1.rs (1)
1854-1857: Add more detailed documentation formin_notional.While the comment explains the purpose of
min_notional, it could be enhanced to include examples or more detailed usage scenarios to aid developers in understanding its application in the context of market orders.
| #[prost(string, tag = "14")] | ||
| pub min_notional: ::prost::alloc::string::String, |
There was a problem hiding this comment.
Add documentation for min_notional.
The field min_notional is added without any accompanying documentation. Please add a descriptive comment similar to other fields to ensure clarity and consistency in the codebase.
| pub const ATOM_DENOM: &str = "atom"; | ||
| pub const BASE_DENOM: &str = "inj"; | ||
| pub const QUOTE_DENOM: &str = "usdt"; | ||
| pub const INJ_PYTH_PRICE_ID: &str = "0x7a5bc1d2b56ad029048cd63964b3ad2776eadf812edc1a43a31406cb54bff592"; | ||
| pub const USDT_PYTH_PRICE_ID: &str = "0x1fc18861232290221461220bd4e2acd1dcdfbc89c84092c93c18bdc7756c1588"; | ||
| pub const GOV_MODULE_ADDRESS: &str = "inj10d07y265gmmuvt4z0w9aw880jnsr700jstypyt"; |
There was a problem hiding this comment.
Currency denominations and contract addresses are hardcoded.
Hardcoding values such as currency denominations and contract addresses can lead to issues when deploying to different environments or when these values need to be updated. Consider fetching these values from a configuration file or environment variables.
| impl Setup { | ||
| pub fn new(exchange_type: ExchangeType) -> Self { | ||
| let app = InjectiveTestApp::new(); | ||
| let wasm = Wasm::new(&app); | ||
| let mut market_id = None; | ||
|
|
||
| let mut denoms = HashMap::new(); | ||
| denoms.insert("atom".to_string(), ATOM_DENOM.to_string()); | ||
| denoms.insert("quote".to_string(), QUOTE_DENOM.to_string()); | ||
| denoms.insert("base".to_string(), BASE_DENOM.to_string()); | ||
|
|
||
| let signer = app.init_account(&[str_coin("1000000", BASE_DENOM, BASE_DECIMALS)]).unwrap(); | ||
|
|
||
| let validator = app.get_first_validator_signing_account(BASE_DENOM.to_string(), 1.2f64).unwrap(); | ||
|
|
||
| let owner = app | ||
| .init_account(&[ | ||
| str_coin("1000000", ATOM_DENOM, ATOM_DECIMALS), | ||
| str_coin("1000000", BASE_DENOM, BASE_DECIMALS), | ||
| str_coin("1000000", QUOTE_DENOM, QUOTE_DECIMALS), | ||
| ]) | ||
| .unwrap(); | ||
|
|
||
| let mut users: Vec<UserInfo> = Vec::new(); | ||
| for _ in 0..10 { | ||
| let user = app | ||
| .init_account(&[ | ||
| str_coin("1000000", ATOM_DENOM, ATOM_DECIMALS), | ||
| str_coin("1000000", BASE_DENOM, BASE_DECIMALS), | ||
| str_coin("1000", QUOTE_DENOM, QUOTE_DECIMALS), | ||
| ]) | ||
| .unwrap(); | ||
|
|
||
| let user_subaccount_id = checked_address_to_subaccount_id(&Addr::unchecked(user.address()), 0u32); | ||
|
|
||
| users.push(UserInfo { | ||
| account: user, | ||
| subaccount_id: user_subaccount_id, | ||
| }); | ||
| } | ||
|
|
||
| let wasm_byte_code = std::fs::read(wasm_file("injective_cosmwasm_stargate_example".to_string())).unwrap(); | ||
| let code_id = wasm.store_code(&wasm_byte_code, None, &owner).unwrap().data.code_id; | ||
|
|
||
| // Instantiate contract | ||
| let contract_address: String = wasm | ||
| .instantiate(code_id, &InstantiateMsg {}, Some(&owner.address()), Some("mock-contract"), &[], &owner) | ||
| .unwrap() | ||
| .data | ||
| .address; | ||
|
|
||
| assert!(!contract_address.is_empty(), "Contract address is empty"); | ||
|
|
||
| send(&Bank::new(&app), "1000000000000000000000", BASE_DENOM, &owner, &validator); | ||
|
|
||
| launch_insurance_fund( | ||
| &app, | ||
| &owner, | ||
| "INJ/USDT", | ||
| denoms["quote"].as_str(), | ||
| denoms["base"].as_str(), | ||
| denoms["quote"].as_str(), | ||
| OracleType::PriceFeed, | ||
| ); | ||
|
|
||
| launch_price_feed_oracle( | ||
| &app, | ||
| &signer, | ||
| &validator, | ||
| denoms["base"].as_str(), | ||
| denoms["quote"].as_str(), | ||
| human_to_dec("10.01", BASE_DECIMALS).to_string(), | ||
| ); | ||
|
|
||
| match exchange_type { | ||
| ExchangeType::Spot => { | ||
| let exchange = Exchange::new(&app); | ||
| market_id = Some(launch_spot_market(&exchange, &owner, "INJ/USDT".to_string())); | ||
| } | ||
| ExchangeType::Derivative => { | ||
| let exchange = Exchange::new(&app); | ||
| market_id = Some(launch_perp_market(&exchange, &owner, "INJ/USDT".to_string())); | ||
| } | ||
| ExchangeType::None => {} | ||
| } | ||
|
|
||
| Self { | ||
| app, | ||
| owner, | ||
| signer, | ||
| validator, | ||
| users, | ||
| denoms, | ||
| contract_address, | ||
| code_id, | ||
| market_id, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Complex function Setup::new initializes various components.
This function is quite complex and does a lot of different things, which can make it hard to understand and maintain. Consider breaking it down into smaller, more focused functions.
| pub fn send(bank: &Bank<InjectiveTestApp>, amount: &str, denom: &str, from: &SigningAccount, to: &SigningAccount) { | ||
| bank.send( | ||
| MsgSend { | ||
| from_address: from.address(), | ||
| to_address: to.address(), | ||
| amount: vec![BaseCoin { | ||
| amount: amount.to_string(), | ||
| denom: denom.to_string(), | ||
| }], | ||
| }, | ||
| from, | ||
| ) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
Function send performs a transaction without checking for errors.
The function unwraps the result of the send method, which could panic if there's an error in the transaction. It's safer to handle potential errors gracefully.
| pub fn launch_price_feed_oracle( | ||
| app: &InjectiveTestApp, | ||
| signer: &SigningAccount, | ||
| validator: &SigningAccount, | ||
| base: &str, | ||
| quote: &str, | ||
| dec_price: String, | ||
| ) { | ||
| let gov = Gov::new(app); | ||
| let oracle = Oracle::new(app); | ||
|
|
||
| let mut buf = vec![]; | ||
| GrantPriceFeederPrivilegeProposal::encode( | ||
| &GrantPriceFeederPrivilegeProposal { | ||
| title: "test-proposal".to_string(), | ||
| description: "test-proposal".to_string(), | ||
| base: base.to_string(), | ||
| quote: quote.to_string(), | ||
| relayers: vec![signer.address()], | ||
| }, | ||
| &mut buf, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let res = gov | ||
| .submit_proposal_v1beta1( | ||
| MsgSubmitProposalV1Beta1 { | ||
| content: Some(Any { | ||
| type_url: "/injective.oracle.v1beta1.GrantPriceFeederPrivilegeProposal".to_string(), | ||
| value: buf, | ||
| }), | ||
| initial_deposit: vec![BaseCoin { | ||
| amount: "100000000000000000000".to_string(), | ||
| denom: "inj".to_string(), | ||
| }], | ||
| proposer: validator.address(), | ||
| }, | ||
| validator, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let proposal_id = res.events.iter().find(|e| e.ty == "submit_proposal").unwrap().attributes[0] | ||
| .value | ||
| .to_owned(); | ||
|
|
||
| gov.vote( | ||
| MsgVote { | ||
| proposal_id: u64::from_str(&proposal_id).unwrap(), | ||
| voter: validator.address(), | ||
| option: 1i32, | ||
| metadata: "".to_string(), | ||
| }, | ||
| validator, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| // NOTE: increase the block time in order to move past the voting period | ||
| app.increase_time(10u64); | ||
|
|
||
| oracle | ||
| .relay_price_feed( | ||
| MsgRelayPriceFeedPrice { | ||
| sender: signer.address(), | ||
| base: vec![base.to_string()], | ||
| quote: vec![quote.to_string()], | ||
| price: vec![dec_price], // 1.2@18dp | ||
| }, | ||
| signer, | ||
| ) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
Complex function launch_price_feed_oracle handles governance and oracle operations.
This function is doing too much by handling both governance and oracle operations. Consider splitting it into separate functions for better modularity and clarity.
| #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema)] | ||
| pub struct AuthParams { | ||
| pub max_memo_characters: String, | ||
| pub sig_verify_cost_ed25519: String, | ||
| } |
There was a problem hiding this comment.
Consider changing the type of max_memo_characters and sig_verify_cost_ed25519 from String to a more appropriate type like usize or u64 if they are intended to represent numeric values.
- pub max_memo_characters: String,
- pub sig_verify_cost_ed25519: String,
+ pub max_memo_characters: usize,
+ pub sig_verify_cost_ed25519: u64,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.
| #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema)] | |
| pub struct AuthParams { | |
| pub max_memo_characters: String, | |
| pub sig_verify_cost_ed25519: String, | |
| } | |
| #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema)] | |
| pub struct AuthParams { | |
| pub max_memo_characters: usize, | |
| pub sig_verify_cost_ed25519: u64, | |
| } |
| #[derive(Serialize, Deserialize, Debug)] | ||
| pub struct Pagination { | ||
| // Define fields based on your pagination structure, if any | ||
| } |
There was a problem hiding this comment.
Consider defining specific fields for the Pagination struct if pagination is required, or remove it if not used.
- // Define fields based on your pagination structure, if any
+ pub page_number: u32,
+ pub page_size: u32,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.
| #[derive(Serialize, Deserialize, Debug)] | |
| pub struct Pagination { | |
| // Define fields based on your pagination structure, if any | |
| } | |
| #[derive(Serialize, Deserialize, Debug)] | |
| pub struct Pagination { | |
| pub page_number: u32, | |
| pub page_size: u32, | |
| } |
| pub fn handle_create_order_reply_stargate(deps: DepsMut<InjectiveQueryWrapper>, _msg: &Reply) -> Result<Response, ContractError> { | ||
| let mut response_str = "Something went wrong".to_string(); | ||
| if let Some(mut cache) = ORDER_CALL_CACHE.may_load(deps.storage)? { | ||
| if !cache.is_empty() { | ||
| let order_info = &cache[0]; | ||
| let encode_query_message = encode_proto_message(QueryTraderSpotOrdersRequest { | ||
| market_id: order_info.market_id.clone().into(), | ||
| subaccount_id: order_info.subaccount.clone().into(), | ||
| }); | ||
| let stargate_response = handle_query_stargate( | ||
| &deps.querier, | ||
| "/injective.exchange.v1beta1.Query/TraderSpotTransientOrders".to_string(), | ||
| encode_query_message, | ||
| ); | ||
| response_str = match stargate_response { | ||
| Ok(binary) => String::from_utf8(binary.0).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)), | ||
| Err(e) => format!("Error: {:?}", e), | ||
| }; | ||
| cache.clear(); | ||
| ORDER_CALL_CACHE.save(deps.storage, &cache)?; | ||
| } | ||
| }; | ||
|
|
||
| Ok(Response::new().add_event(Event::new("transient_order").add_attributes([("query_str", response_str)]))) |
There was a problem hiding this comment.
Optimize error handling in string operations.
- Ok(binary) => String::from_utf8(binary.0).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)),
+ Ok(binary) => String::from_utf8(binary.0).unwrap_or_else(|e| format!("Failed to decode binary to string: {}", e)),Change {:?} to {} for error formatting to ensure the error message is displayed correctly without debug formatting.
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.
| pub fn handle_create_order_reply_stargate(deps: DepsMut<InjectiveQueryWrapper>, _msg: &Reply) -> Result<Response, ContractError> { | |
| let mut response_str = "Something went wrong".to_string(); | |
| if let Some(mut cache) = ORDER_CALL_CACHE.may_load(deps.storage)? { | |
| if !cache.is_empty() { | |
| let order_info = &cache[0]; | |
| let encode_query_message = encode_proto_message(QueryTraderSpotOrdersRequest { | |
| market_id: order_info.market_id.clone().into(), | |
| subaccount_id: order_info.subaccount.clone().into(), | |
| }); | |
| let stargate_response = handle_query_stargate( | |
| &deps.querier, | |
| "/injective.exchange.v1beta1.Query/TraderSpotTransientOrders".to_string(), | |
| encode_query_message, | |
| ); | |
| response_str = match stargate_response { | |
| Ok(binary) => String::from_utf8(binary.0).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)), | |
| Err(e) => format!("Error: {:?}", e), | |
| }; | |
| cache.clear(); | |
| ORDER_CALL_CACHE.save(deps.storage, &cache)?; | |
| } | |
| }; | |
| Ok(Response::new().add_event(Event::new("transient_order").add_attributes([("query_str", response_str)]))) | |
| pub fn handle_create_order_reply_stargate(deps: DepsMut<InjectiveQueryWrapper>, _msg: &Reply) -> Result<Response, ContractError> { | |
| let mut response_str = "Something went wrong".to_string(); | |
| if let Some(mut cache) = ORDER_CALL_CACHE.may_load(deps.storage)? { | |
| if !cache.is_empty() { | |
| let order_info = &cache[0]; | |
| let encode_query_message = encode_proto_message(QueryTraderSpotOrdersRequest { | |
| market_id: order_info.market_id.clone().into(), | |
| subaccount_id: order_info.subaccount.clone().into(), | |
| }); | |
| let stargate_response = handle_query_stargate( | |
| &deps.querier, | |
| "/injective.exchange.v1beta1.Query/TraderSpotTransientOrders".to_string(), | |
| encode_query_message, | |
| ); | |
| response_str = match stargate_response { | |
| Ok(binary) => String::from_utf8(binary.0).unwrap_or_else(|e| format!("Failed to decode binary to string: {}", e)), | |
| Err(e) => format!("Error: {:?}", e), | |
| }; | |
| cache.clear(); | |
| ORDER_CALL_CACHE.save(deps.storage, &cache)?; | |
| } | |
| }; | |
| Ok(Response::new().add_event(Event::new("transient_order").add_attributes([("query_str", response_str)]))) |
| pub fn handle_create_derivative_order_reply_stargate(deps: DepsMut<InjectiveQueryWrapper>, _msg: &Reply) -> Result<Response, ContractError> { | ||
| let mut response_str = "Something went wrong".to_string(); | ||
|
|
||
| if let Some(mut cache) = ORDER_CALL_CACHE.may_load(deps.storage)? { | ||
| if !cache.is_empty() { | ||
| let order_info = &cache[0]; | ||
| let encode_query_message = encode_proto_message(QueryTraderDerivativeOrdersRequest { | ||
| market_id: order_info.market_id.clone().into(), | ||
| subaccount_id: order_info.subaccount.clone().into(), | ||
| }); | ||
| let stargate_response = handle_query_stargate( | ||
| &deps.querier, | ||
| "/injective.exchange.v1beta1.Query/TraderDerivativeTransientOrders".to_string(), | ||
| encode_query_message, | ||
| ); | ||
| response_str = match stargate_response { | ||
| Ok(binary) => String::from_utf8(binary.0).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)), | ||
| Err(e) => format!("Error: {:?}", e), | ||
| }; | ||
| cache.clear(); | ||
| ORDER_CALL_CACHE.save(deps.storage, &cache)?; | ||
| } | ||
| }; | ||
|
|
||
| Ok(Response::new().add_event(Event::new("transient_derivative_order").add_attributes([("query_str", response_str)]))) |
There was a problem hiding this comment.
Optimize error handling in string operations.
- Ok(binary) => String::from_utf8(binary.0).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)),
+ Ok(binary) => String::from_utf8(binary.0).unwrap_or_else(|e| format!("Failed to decode binary to string: {}", e)),Change {:?} to {} for error formatting to ensure the error message is displayed correctly without debug formatting.
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.
| pub fn handle_create_derivative_order_reply_stargate(deps: DepsMut<InjectiveQueryWrapper>, _msg: &Reply) -> Result<Response, ContractError> { | |
| let mut response_str = "Something went wrong".to_string(); | |
| if let Some(mut cache) = ORDER_CALL_CACHE.may_load(deps.storage)? { | |
| if !cache.is_empty() { | |
| let order_info = &cache[0]; | |
| let encode_query_message = encode_proto_message(QueryTraderDerivativeOrdersRequest { | |
| market_id: order_info.market_id.clone().into(), | |
| subaccount_id: order_info.subaccount.clone().into(), | |
| }); | |
| let stargate_response = handle_query_stargate( | |
| &deps.querier, | |
| "/injective.exchange.v1beta1.Query/TraderDerivativeTransientOrders".to_string(), | |
| encode_query_message, | |
| ); | |
| response_str = match stargate_response { | |
| Ok(binary) => String::from_utf8(binary.0).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)), | |
| Err(e) => format!("Error: {:?}", e), | |
| }; | |
| cache.clear(); | |
| ORDER_CALL_CACHE.save(deps.storage, &cache)?; | |
| } | |
| }; | |
| Ok(Response::new().add_event(Event::new("transient_derivative_order").add_attributes([("query_str", response_str)]))) | |
| pub fn handle_create_derivative_order_reply_stargate(deps: DepsMut<InjectiveQueryWrapper>, _msg: &Reply) -> Result<Response, ContractError> { | |
| let mut response_str = "Something went wrong".to_string(); | |
| if let Some(mut cache) = ORDER_CALL_CACHE.may_load(deps.storage)? { | |
| if !cache.is_empty() { | |
| let order_info = &cache[0]; | |
| let encode_query_message = encode_proto_message(QueryTraderDerivativeOrdersRequest { | |
| market_id: order_info.market_id.clone().into(), | |
| subaccount_id: order_info.subaccount.clone().into(), | |
| }); | |
| let stargate_response = handle_query_stargate( | |
| &deps.querier, | |
| "/injective.exchange.v1beta1.Query/TraderDerivativeTransientOrders".to_string(), | |
| encode_query_message, | |
| ); | |
| response_str = match stargate_response { | |
| Ok(binary) => String::from_utf8(binary.0).unwrap_or_else(|e| format!("Failed to decode binary to string: {}", e)), | |
| Err(e) => format!("Error: {:?}", e), | |
| }; | |
| cache.clear(); | |
| ORDER_CALL_CACHE.save(deps.storage, &cache)?; | |
| } | |
| }; | |
| Ok(Response::new().add_event(Event::new("transient_derivative_order").add_attributes([("query_str", response_str)]))) |
| injective-test-tube = { version = "1.1.7" } | ||
| injective-std = { version = "0.1.5" } | ||
| injective-std = { path = "../../packages/injective-std" } | ||
| injective-test-tube = { path = "/Users/markuswaas/work/injective/test-tube/packages/injective-test-tube" } |
There was a problem hiding this comment.
injective-test-tube = { path = "/Users/markuswaas/work/injective/test-tube/packages/injective-test-tube" } needs revert to injective-test-tube = { version = "1.1.7" }
There was a problem hiding this comment.
Actionable comments posted: 18
Outside diff range and nitpick comments (2)
contracts/injective-cosmwasm-mock/src/handle.rs (2)
Line range hint
16-36: Ensure Proper Error Handling for Market QueryThe function
handle_test_transient_spot_orderdoes not handle potential errors fromquerier.query_spot_market(&market_id)which could returnNone. This could lead to a panic ifunwrap()is called on aNonevalue.- let spot_market = querier.query_spot_market(&market_id).unwrap().market.unwrap(); + let spot_market = match querier.query_spot_market(&market_id)? { + Some(market_info) => market_info.market.ok_or(ContractError::MarketNotFound)?, + None => return Err(ContractError::MarketQueryFailed), + };This change introduces proper error handling for market queries, preventing potential runtime panics and providing clearer error messages.
Line range hint
81-121: Ensure Proper Error Handling for Derivative Market QuerySimilar to the spot order function,
handle_test_transient_derivative_ordershould handle potentialNonevalues returned byquerier.query_derivative_market(&market_id).- let market = querier.query_derivative_market(&market_id).unwrap().market.unwrap(); + let market = match querier.query_derivative_market(&market_id)? { + Some(market_info) => market_info.market.ok_or(ContractError::MarketNotFound)?, + None => return Err(ContractError::MarketQueryFailed), + };This change ensures that the function does not panic at runtime due to unhandled
Nonevalues and improves the robustness of error handling.
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 (31)
- contracts/atomic-order-example/Cargo.toml (1 hunks)
- contracts/atomic-order-example/src/tests.rs (1 hunks)
- contracts/dummy/Cargo.toml (1 hunks)
- contracts/injective-cosmwasm-mock/Cargo.toml (1 hunks)
- contracts/injective-cosmwasm-mock/src/contract.rs (1 hunks)
- contracts/injective-cosmwasm-mock/src/handle.rs (1 hunks)
- contracts/injective-cosmwasm-mock/src/testing/test_exchange.rs (2 hunks)
- contracts/injective-cosmwasm-mock/src/testing/test_exchange_derivative.rs (2 hunks)
- contracts/injective-cosmwasm-mock/src/utils.rs (2 hunks)
- contracts/injective-cosmwasm-stargate-example/Cargo.toml (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/contract.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/handle.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/lib.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/msg.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/query.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/reply.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/spot_market_order_msg.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_auction.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_auth.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_bank.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_oracle.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/utils.rs (1 hunks)
- packages/injective-cosmwasm/src/exchange_mock_querier.rs (1 hunks)
- packages/injective-math/Cargo.toml (1 hunks)
- packages/injective-math/src/fp_decimal/display.rs (2 hunks)
- packages/injective-math/src/fp_decimal/mod.rs (1 hunks)
- packages/injective-math/src/fp_decimal/scale.rs (2 hunks)
- packages/injective-std/src/types/injective/exchange/v1beta1.rs (1 hunks)
- packages/injective-testing/Cargo.toml (1 hunks)
Files not reviewed due to errors (3)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_bank.rs (no review received)
- contracts/injective-cosmwasm-stargate-example/src/handle.rs (no review received)
- contracts/injective-cosmwasm-mock/src/testing/test_exchange.rs (no review received)
Files skipped from review due to trivial changes (6)
- contracts/atomic-order-example/Cargo.toml
- contracts/dummy/Cargo.toml
- packages/injective-math/Cargo.toml
- packages/injective-math/src/fp_decimal/mod.rs
- packages/injective-std/src/types/injective/exchange/v1beta1.rs
- packages/injective-testing/Cargo.toml
Additional comments not posted (21)
contracts/injective-cosmwasm-stargate-example/src/lib.rs (1)
1-17: Well-structured module declarations and exports.The modular structure enhances the maintainability and readability of the contract. Ensuring
errorandutilsmodules are properly encapsulated while exposing necessary interfaces likemsgandContractErroris a good practice.packages/injective-math/src/fp_decimal/scale.rs (2)
3-3: Clear definition of the decimal scale factor.Defining
DEC_SCALE_FACTORas a constant enhances code readability and maintainability by centralizing the scale factor.
16-16: Correct implementation ofdec_scale_factorfunction.Using the
scaledmethod to apply theDEC_SCALE_FACTORensures consistency in handling decimal scaling across the application.packages/injective-math/src/fp_decimal/display.rs (2)
1-4: Proper modular imports and structuring.Grouping related imports and using structured paths enhances code readability and maintainability.
33-37: Efficient implementation ofToPrototrait forFPDecimal.Implementing the
to_proto_stringmethod to scaleFPDecimalaccording toDEC_SCALE_FACTORbefore converting to string ensures consistency in data representation when interacting with protobufs.contracts/injective-cosmwasm-stargate-example/src/msg.rs (1)
1-47: Comprehensive and well-defined message structures.The message structures and enums are well-defined with appropriate serialization attributes and data types, which will facilitate clear and error-free contract interactions.
contracts/injective-cosmwasm-stargate-example/src/testing/test_auth.rs (1)
1-44: Thorough and well-structured authentication tests.The tests are well-structured with clear setup and assertions. Using the
ignoreattribute conditionally on non-integration builds is a good practice to avoid running these tests in inappropriate environments.contracts/injective-cosmwasm-stargate-example/src/query.rs (1)
33-36: Good practice in handling bank parameters query.The function
handle_query_bank_paramsis concise and uses the correct querying pattern. It simplifies the error handling by directly propagating it, which is acceptable in this context.contracts/injective-cosmwasm-stargate-example/src/spot_market_order_msg.rs (1)
27-54: Optimize price and quantity rounding logic.The rounding functions are called sequentially which might be less efficient. Consider combining these operations if possible to streamline the logic.
contracts/injective-cosmwasm-stargate-example/Cargo.toml (1)
32-53: Validate new dependencies and versions.Ensure that the newly added dependencies and updated versions are compatible with the project's existing architecture and do not introduce any security vulnerabilities.
contracts/injective-cosmwasm-stargate-example/src/reply.rs (1)
57-81: Optimize Error Handling and Debug FormattingSimilar to the spot order reply handler,
handle_create_derivative_order_reply_stargateshould also optimize error handling and debug formatting.contracts/injective-cosmwasm-stargate-example/src/testing/test_auction.rs (2)
67-83: Ensure Proper Error Handling in TestSimilar to the previous test,
test_auction_paramsshould also handle potential errors gracefully.
85-102: Ensure Proper Error Handling in TestSimilar to other tests,
test_last_auction_resultshould also handle potential errors gracefully.contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange.rs (1)
99-136: Ensure Accuracy in Transient Spot Order TestsThe function
test_query_trader_transient_spot_orderstests querying transient spot orders. Ensure the accuracy of the queries and the correctness of the responses, especially in edge cases.contracts/injective-cosmwasm-stargate-example/src/utils.rs (6)
1-1: Ensure consistency in import statements.The import statement combines multiple entities from
msg. It's good practice to keep imports organized and perhaps split into multiple lines if too many entities are imported from the same module, which can improve readability.
43-48: Hardcoded values need attention.The currency denominations and contract addresses are hardcoded, which can lead to issues when deploying to different environments or when these values need to be updated. Consider fetching these values from a configuration file or environment variables.
72-170: Complex functionSetup::newinitializes various components.This function is quite complex and does a lot of different things, which can make it hard to understand and maintain. Consider breaking it down into smaller, more focused functions.
202-215: Functionsendperforms a transaction without checking for errors.The function unwraps the result of the
sendmethod, which could panic if there's an error in the transaction. It's safer to handle potential errors gracefully.
217-287: Complex functionlaunch_price_feed_oraclehandles governance and oracle operations.This function is doing too much by handling both governance and oracle operations. Consider splitting it into separate functions for better modularity and clarity.
2-2: Validate the usage ofcoinfunction.The
coinfunction is used throughout the file. Ensure that the denominations and amounts passed to this function are validated elsewhere to prevent issues like overflow or underflow, especially since monetary values are involved.packages/injective-cosmwasm/src/exchange_mock_querier.rs (1)
68-68: The implementation for handling trader spot orders in the mock is correct.This function appropriately returns a constant empty response, which is suitable for testing scenarios where no specific data setup is required.
| @@ -65,6 +65,7 @@ fn default_spot_market_response_handler(market_id: MarketId) -> QuerierResult { | |||
| status: MarketStatus::Active, | |||
| min_price_tick_size: FPDecimal::from_str("0.01").unwrap(), | |||
| min_quantity_tick_size: FPDecimal::from_str("1000000000000000.0").unwrap(), | |||
| min_notional: FPDecimal::from_str("0.01").unwrap(), | |||
There was a problem hiding this comment.
Consider replacing unwrap() with proper error handling to prevent potential runtime panics.
This will make the mock more robust and prevent misleading test failures if the string parsing fails.
- FPDecimal::from_str("0.01").unwrap(),
+ match FPDecimal::from_str("0.01") {
+ Ok(value) => value,
+ Err(e) => return SystemResult::Err(SystemError::InvalidRequest {
+ error: format!("Failed to parse FPDecimal: {e}"),
+ request: bin_request.into(),
+ }),
+ }Improve error handling in default_derivative_market_response_handler by replacing unwrap().
Using match or if let could prevent runtime panics and make the function more reliable during testing.
- FPDecimal::from_str("0.1").unwrap(),
+ match FPDecimal::from_str("0.1") {
+ Ok(value) => value,
+ Err(e) => return SystemResult::Err(SystemError::InvalidRequest {
+ error: format!("Failed to parse FPDecimal: {e}"),
+ request: bin_request.into(),
+ }),
+ }| pub const EXCHANGE_DECIMALS: i32 = 18i32; | ||
| pub const BASE_DECIMALS: i32 = 18i32; | ||
| pub const ATOM_DECIMALS: i32 = 8i32; | ||
| pub const QUOTE_DECIMALS: i32 = 6i32; |
There was a problem hiding this comment.
Consider using a centralized configuration for constants.
The constants for decimal values are defined separately. It would be more maintainable to centralize these, especially if they are shared across different parts of the application.
- pub const EXCHANGE_DECIMALS: i32 = 18i32;
- pub const BASE_DECIMALS: i32 = 18i32;
- pub const ATOM_DECIMALS: i32 = 8i32;
- pub const QUOTE_DECIMALS: i32 = 6i32;
+ pub struct DecimalConstants {
+ pub exchange: i32,
+ pub base: i32,
+ pub atom: i32,
+ pub quote: i32,
+ }
+
+ impl DecimalConstants {
+ pub const DEFAULT: DecimalConstants = DecimalConstants {
+ exchange: 18,
+ base: 18,
+ atom: 8,
+ quote: 6,
+ };
+ }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 const EXCHANGE_DECIMALS: i32 = 18i32; | |
| pub const BASE_DECIMALS: i32 = 18i32; | |
| pub const ATOM_DECIMALS: i32 = 8i32; | |
| pub const QUOTE_DECIMALS: i32 = 6i32; | |
| pub struct DecimalConstants { | |
| pub exchange: i32, | |
| pub base: i32, | |
| pub atom: i32, | |
| pub quote: i32, | |
| } | |
| impl DecimalConstants { | |
| pub const DEFAULT: DecimalConstants = DecimalConstants { | |
| exchange: 18, | |
| base: 18, | |
| atom: 8, | |
| quote: 6, | |
| }; | |
| } |
| let wasm = Wasm::new(&env.app); | ||
| let exchange = Exchange::new(&env.app); | ||
|
|
||
| let ticker = "INJ/USDT".to_string(); |
There was a problem hiding this comment.
Consider using constants for repeated string literals.
Repeated string values like "INJ/USDT" and paths used in queries ("/injective.exchange.v1beta1.Query/PerpetualMarketInfo") could be defined as constants to improve maintainability and reduce the risk of typos in multiple uses.
const MARKET_TICKER: &str = "INJ/USDT";
const PERPETUAL_MARKET_INFO_QUERY_PATH: &str = "/injective.exchange.v1beta1.Query/PerpetualMarketInfo";Then replace the literals in the code with these constants.
Also applies to: 35-40, 56-60
| let env = Setup::new(ExchangeType::None); | ||
| let wasm = Wasm::new(&env.app); | ||
| let exchange = Exchange::new(&env.app); | ||
| let ticker = "INJ/USDT".to_string(); | ||
| let initial_margin_ratio = FPDecimal::must_from_str("0.195"); | ||
| let maintenance_margin_ratio = FPDecimal::must_from_str("0.05"); | ||
| let min_price_tick_size = FPDecimal::must_from_str("1000.0"); | ||
| let min_quantity_tick_size = FPDecimal::must_from_str("1000000000000000"); | ||
| let min_notional = FPDecimal::must_from_str("1000000000000000"); | ||
|
|
||
| let quote_denom = QUOTE_DENOM.to_string(); | ||
| let maker_fee_rate = FPDecimal::ZERO; | ||
| let taker_fee_rate = FPDecimal::ZERO; | ||
|
|
||
| exchange | ||
| .instant_perpetual_market_launch( | ||
| MsgInstantPerpetualMarketLaunch { | ||
| sender: env.signer.address(), | ||
| ticker: ticker.to_owned(), | ||
| quote_denom: quote_denom.to_owned(), | ||
| oracle_base: BASE_DENOM.to_owned(), | ||
| oracle_quote: quote_denom.to_owned(), | ||
| oracle_scale_factor: 6u32, | ||
| oracle_type: 2i32, | ||
| maker_fee_rate: dec_to_proto(maker_fee_rate).to_string(), | ||
| taker_fee_rate: dec_to_proto(taker_fee_rate), | ||
| initial_margin_ratio: dec_to_proto(initial_margin_ratio), | ||
| maintenance_margin_ratio: dec_to_proto(maintenance_margin_ratio), | ||
| min_price_tick_size: dec_to_proto(min_price_tick_size), | ||
| min_quantity_tick_size: dec_to_proto(min_quantity_tick_size), | ||
| min_notional: dec_to_proto(min_notional), | ||
| }, | ||
| &env.signer, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let derivative_market_id = get_perpetual_market_id(&exchange, ticker.to_owned()); | ||
| let query_msg = QueryMsg::QueryStargateRaw { | ||
| path: "/injective.exchange.v1beta1.Query/DerivativeMarket".to_string(), |
There was a problem hiding this comment.
Refactor to reduce duplication in test setup.
The setup code in this function is very similar to that in other test functions. Consider creating a helper function to handle the common setup tasks to reduce code duplication and improve test maintainability.
fn setup_test_environment(ticker: &str, exchange_type: ExchangeType) -> (Wasm, Exchange) {
let env = Setup::new(exchange_type);
let wasm = Wasm::new(&env.app);
let exchange = Exchange::new(&env.app);
(wasm, exchange)
}Use this function in your test cases to replace the duplicated setup code.
| pub fn handle_query_spot_market(deps: Deps<InjectiveQueryWrapper>, market_id: &str) -> StdResult<Binary> { | ||
| let querier = ExchangeQuerier::new(&deps.querier); | ||
| to_json_binary(&querier.spot_market(market_id.to_string())?) | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in handle_query_spot_market.
Currently, the function directly uses the ? operator without handling specific errors that might arise from the spot_market method. Consider adding specific error handling to provide clearer error messages.
- to_json_binary(&querier.spot_market(market_id.to_string())?)
+ let market_info = querier.spot_market(market_id.to_string()).map_err(|e| StdError::generic_err(format!("Failed to query spot market with ID {}: {}", market_id, e)))?;
+ to_json_binary(&market_info)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 handle_query_spot_market(deps: Deps<InjectiveQueryWrapper>, market_id: &str) -> StdResult<Binary> { | |
| let querier = ExchangeQuerier::new(&deps.querier); | |
| to_json_binary(&querier.spot_market(market_id.to_string())?) | |
| } | |
| pub fn handle_query_spot_market(deps: Deps<InjectiveQueryWrapper>, market_id: &str) -> StdResult<Binary> { | |
| let querier = ExchangeQuerier::new(&deps.querier); | |
| let market_info = querier.spot_market(market_id.to_string()).map_err(|e| StdError::generic_err(format!("Failed to query spot market with ID {}: {}", market_id, e)))?; | |
| to_json_binary(&market_info) | |
| } |
| #[test] | ||
| #[cfg_attr(not(feature = "integration"), ignore)] | ||
| fn test_query_pyth_oracle_price() { | ||
| let env = Setup::new(ExchangeType::None); | ||
| let wasm = Wasm::new(&env.app); | ||
| let oracle = Oracle::new(&env.app); | ||
|
|
||
| let validator = env.app.get_first_validator_signing_account(BASE_DENOM.to_string(), 1.2f64).unwrap(); | ||
| let pyth_contract = env.app.init_account(&[str_coin("1000000", BASE_DENOM, BASE_DECIMALS)]).unwrap(); | ||
|
|
||
| set_address_of_pyth_contract(&env.app, &validator, &pyth_contract); | ||
| let price_attestations = vec![create_some_inj_price_attestation("7", 5, env.app.get_block_time_seconds())]; | ||
| relay_pyth_price(&oracle, price_attestations, &pyth_contract); | ||
|
|
||
| let price_pyth_oracle_response = oracle | ||
| .query_pyth_price(&QueryPythPriceRequest { | ||
| price_id: INJ_PYTH_PRICE_ID.to_string(), | ||
| }) | ||
| .unwrap(); | ||
| let price_pyth_oracle_response = FPDecimal::must_from_str(price_pyth_oracle_response.price_state.unwrap().ema_price.as_str()); | ||
|
|
||
| let query_msg = QueryMsg::QueryStargateRaw { | ||
| path: "/injective.oracle.v1beta1.Query/PythPrice".to_string(), | ||
| query_request: encode_proto_message(QueryPythPriceRequest { | ||
| price_id: INJ_PYTH_PRICE_ID.to_string(), | ||
| }), | ||
| }; | ||
|
|
||
| let contract_response = get_stargate_query_result::<MyPythPriceResponse>(wasm.query(&env.contract_address, &query_msg)).unwrap(); | ||
| assert_eq!( | ||
| contract_response.price_state.unwrap().ema_price.scaled(BASE_DECIMALS), | ||
| price_pyth_oracle_response | ||
| ); |
There was a problem hiding this comment.
Optimize Test Setup and Reuse
The function test_query_pyth_oracle_price sets up the environment and performs similar actions to other test functions. Consider refactoring to share common setup code across tests to reduce redundancy and improve maintainability.
// Suggesting a common setup function that can be reused by multiple test functions.
fn common_setup() -> (Env, Wasm, Oracle) {
let env = Setup::new(ExchangeType::None);
let wasm = Wasm::new(&env.app);
let oracle = Oracle::new(&env.app);
(env, wasm, oracle)
}
// Use the common setup function in test functions.
let (env, wasm, oracle) = common_setup();| #[test] | ||
| #[cfg_attr(not(feature = "integration"), ignore)] | ||
| fn test_query_oracle_volatility() { | ||
| let env = Setup::new(ExchangeType::None); | ||
| let wasm = Wasm::new(&env.app); | ||
|
|
||
| let base_info = Some(OracleInfo { | ||
| symbol: env.denoms["base"].to_owned(), | ||
| oracle_type: OracleType::PriceFeed as i32, | ||
| }); | ||
|
|
||
| let quote_info = Some(OracleInfo { | ||
| symbol: env.denoms["quote"].to_owned(), | ||
| oracle_type: OracleType::PriceFeed as i32, | ||
| }); | ||
|
|
||
| let oracle_history_options = Some(OracleHistoryOptions { | ||
| max_age: 60u64, | ||
| include_raw_history: true, | ||
| include_metadata: true, | ||
| }); | ||
|
|
||
| let query_msg = QueryMsg::QueryStargateRaw { | ||
| path: "/injective.oracle.v1beta1.Query/OracleVolatility".to_string(), | ||
| query_request: encode_proto_message(QueryOracleVolatilityRequest { | ||
| base_info, | ||
| quote_info, | ||
| oracle_history_options, | ||
| }), | ||
| }; | ||
|
|
||
| let res = get_stargate_query_result::<MyVolatilityResponse>(wasm.query(&env.contract_address, &query_msg)).unwrap(); | ||
| assert!(res.volatility.is_none()); |
There was a problem hiding this comment.
Tip
Codebase Verification
Check for Potential False Positives in Test Assertions
In test_query_oracle_volatility, the test setup uses ExchangeType::None, which might not adequately simulate conditions for a non-None volatility response. To ensure the test environment and inputs are correctly configured to test various scenarios, consider modifying the setup to use different exchange types or parameters.
contracts/injective-cosmwasm-stargate-example/src/testing/test_oracle.rs: Modify the setup intest_query_oracle_volatilityto use differentExchangeTypevalues or additional parameters to simulate various conditions.
Analysis chain
Check for Potential False Positives in Test Assertions
In test_query_oracle_volatility, the assertion checks if res.volatility is None. This might be a false positive if the test setup does not adequately simulate conditions for a non-None response. Ensure the test environment and inputs are correctly configured to test various scenarios.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the test setup for oracle volatility.
# Test: Check the configuration and simulation conditions for volatility.
rg --type rust $'test_query_oracle_volatility' contracts/injective-cosmwasm-stargate-example/src/testing/test_oracle.rs
Length of output: 154
Script:
#!/bin/bash
# Description: Inspect the setup and related configurations for the test `test_query_oracle_volatility`.
# Search for the `Setup::new` function call to understand the environment setup.
rg 'Setup::new' contracts/injective-cosmwasm-stargate-example/src/testing/test_oracle.rs
# Search for other relevant setup functions or configurations in the test file.
rg 'let env' contracts/injective-cosmwasm-stargate-example/src/testing/test_oracle.rs
# Search for the `get_stargate_query_result` function to understand how the query result is fetched and processed.
rg 'get_stargate_query_result' contracts/injective-cosmwasm-stargate-example/src/testing/test_oracle.rs
Length of output: 1096
Script:
#!/bin/bash
# Description: Inspect the `Setup` struct or class and the `get_stargate_query_result` function to understand the environment configuration and query result processing.
# Search for the `Setup` struct or class definition to understand its configuration.
rg 'struct Setup' --type rust
# Search for the `get_stargate_query_result` function definition to understand how the query result is processed.
rg 'fn get_stargate_query_result' --type rust
Length of output: 419
Script:
#!/bin/bash
# Description: Extract the definitions of the `Setup` struct and the `get_stargate_query_result` function for analysis.
# Extract the definition of the `Setup` struct from `contracts/injective-cosmwasm-stargate-example/src/utils.rs`.
rg -A 20 'pub struct Setup' contracts/injective-cosmwasm-stargate-example/src/utils.rs
# Extract the definition of the `Setup` struct from `contracts/injective-cosmwasm-mock/src/utils.rs`.
rg -A 20 'pub struct Setup' contracts/injective-cosmwasm-mock/src/utils.rs
# Extract the definition of the `get_stargate_query_result` function from `contracts/injective-cosmwasm-stargate-example/src/utils.rs`.
rg -A 20 'pub fn get_stargate_query_result' contracts/injective-cosmwasm-stargate-example/src/utils.rs
Length of output: 1588
| @@ -136,6 +136,7 @@ fn create_spot_market_handler() -> impl HandlesMarketIdQuery { | |||
| status: MarketStatus::Active, | |||
| min_price_tick_size: FPDecimal::from_str("0.000000000000001").unwrap(), | |||
| min_quantity_tick_size: FPDecimal::from_str("1000000000000000").unwrap(), | |||
| min_notional: FPDecimal::ONE, | |||
There was a problem hiding this comment.
Ensure Minimum Notional Value is Enforced
The creation of the spot market handler should enforce a minimum notional value. This is critical for maintaining market stability and preventing manipulation.
- min_notional: FPDecimal::ONE,
+ min_notional: FPDecimal::from_str("10").unwrap(), // Adjust according to market requirements| #[test] | ||
| #[cfg_attr(not(feature = "integration"), ignore)] | ||
| fn test_exchange_param() { | ||
| let env = Setup::new(ExchangeType::None); | ||
| let wasm = Wasm::new(&env.app); | ||
|
|
||
| let query_msg = QueryMsg::QueryStargateRaw { | ||
| path: "/injective.exchange.v1beta1.Query/QueryExchangeParams".to_string(), | ||
| query_request: "".to_string(), | ||
| }; | ||
|
|
||
| let contract_response: QueryStargateResponse = wasm.query(&env.contract_address, &query_msg).unwrap(); | ||
| let contract_response = contract_response.value; | ||
| println!("{:?}", contract_response); | ||
| let response: ParamResponse<ExchangeParams> = from_json(&contract_response).unwrap(); | ||
| println!("{:?}", response); | ||
| let listing_fee_coin = str_coin("1000", BASE_DENOM, BASE_DECIMALS); | ||
| assert_eq!(response.params.spot_market_instant_listing_fee, listing_fee_coin); | ||
| } |
There was a problem hiding this comment.
Validate Exchange Parameter Query Logic
The test_exchange_param function tests querying exchange parameters. Ensure the query logic is correct and that the response is properly validated against expected results.
- assert_eq!(response.params.spot_market_instant_listing_fee, listing_fee_coin);
+ assert!(response.params.spot_market_instant_listing_fee >= listing_fee_coin, "Listing fee should meet minimum requirements");| #[test] | ||
| #[cfg_attr(not(feature = "integration"), ignore)] | ||
| fn test_query_subaccount_deposit() { | ||
| let env = Setup::new(ExchangeType::None); | ||
| let exchange = Exchange::new(&env.app); | ||
| let wasm = Wasm::new(&env.app); | ||
|
|
||
| let subaccount_id = checked_address_to_subaccount_id(&Addr::unchecked(env.users[0].account.address()), 1u32); | ||
|
|
||
| let make_deposit = |amount: &str, denom_key: &str| { | ||
| exchange | ||
| .deposit( | ||
| MsgDeposit { | ||
| sender: env.users[0].account.address(), | ||
| subaccount_id: subaccount_id.to_string(), | ||
| amount: Some(injective_std::types::cosmos::base::v1beta1::Coin { | ||
| amount: amount.to_string(), | ||
| denom: env.denoms[denom_key].clone(), | ||
| }), | ||
| }, | ||
| &env.users[0].account, | ||
| ) | ||
| .unwrap(); | ||
| }; | ||
|
|
||
| make_deposit("10000000000000000000", "base"); | ||
| make_deposit("100000000", "quote"); | ||
|
|
||
| let response = exchange | ||
| .query_subaccount_deposits(&QuerySubaccountDepositsRequest { | ||
| subaccount_id: subaccount_id.to_string(), | ||
| subaccount: None, | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!( | ||
| response.deposits[&env.denoms["base"].clone()], | ||
| Deposit { | ||
| available_balance: human_to_proto("10.0", BASE_DECIMALS), | ||
| total_balance: human_to_proto("10.0", BASE_DECIMALS), | ||
| } | ||
| ); | ||
| assert_eq!( | ||
| response.deposits[&env.denoms["quote"].clone()], | ||
| Deposit { | ||
| available_balance: human_to_proto("100.0", QUOTE_DECIMALS), | ||
| total_balance: human_to_proto("100.0", QUOTE_DECIMALS), | ||
| } | ||
| ); | ||
|
|
||
| let query_msg = QueryMsg::QueryStargateRaw { | ||
| path: "/injective.exchange.v1beta1.Query/SubaccountDeposit".to_string(), | ||
| query_request: encode_proto_message(QuerySubaccountDepositRequest { | ||
| subaccount_id: subaccount_id.to_string(), | ||
| denom: env.denoms["base"].clone(), | ||
| }), | ||
| }; | ||
| let contract_response: QueryStargateResponse = wasm.query(&env.contract_address, &query_msg).unwrap(); | ||
| let contract_response = contract_response.value; | ||
| let contract_response: SubaccountDepositResponse = serde_json::from_str(&contract_response).unwrap(); | ||
| let deposit = contract_response.deposits; | ||
| assert_eq!(deposit.total_balance, human_to_dec("10.0", BASE_DECIMALS)); | ||
| } |
There was a problem hiding this comment.
Optimize Subaccount Deposit Query Tests
The test_query_subaccount_deposit function tests querying subaccount deposits. The function could be optimized by reusing query setups and improving the logic for handling different deposit scenarios.
// Suggest reusing query setups and improving logic.
fn query_subaccount_deposit(deps: DepsMut, env: Env, subaccount_id: String) -> Result<QueryResponse, ContractError> {
// Query logic here
}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (2)
contracts/injective-cosmwasm-stargate-example/src/utils.rs (1)
382-396: Consider handling the result of the execution or adding a comment.The
revoke_authorizationfunction executes aMsgRevokemessage to revoke the authorization, but the result of the execution is ignored and not used. Consider handling the result or adding a comment to explain why it is ignored. This can help improve the clarity and maintainability of the code.packages/injective-cosmwasm/src/exchange_mock_querier.rs (1)
68-68: Consider replacingunwrap()with proper error handling to prevent potential runtime panics in the test setup.Using
unwrap()here could cause unexpected panics if the hardcoded string "0.01" is ever changed to an invalid decimal format. Gracefully handling the parsing error will make the test setup more robust.- min_notional: FPDecimal::from_str("0.01").unwrap(), + min_notional: match FPDecimal::from_str("0.01") { + Ok(value) => value, + Err(e) => return SystemResult::Err(SystemError::InvalidRequest { + error: format!("Failed to parse min_notional: {e}"), + request: bin_request.into(), + }), + },
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 (19)
- Cargo.toml (2 hunks)
- contracts/atomic-order-example/src/tests.rs (1 hunks)
- contracts/injective-cosmwasm-mock/src/testing/test_exchange.rs (2 hunks)
- contracts/injective-cosmwasm-mock/src/testing/test_exchange_derivative.rs (2 hunks)
- contracts/injective-cosmwasm-stargate-example/Cargo.toml (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/contract.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/handle.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/lib.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/order_management.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/query.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/reply.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/spot_market_order_msg.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_auction.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_bank.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/test_oracle.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/utils.rs (1 hunks)
- packages/injective-cosmwasm/src/exchange_mock_querier.rs (1 hunks)
Additional comments not posted (72)
contracts/injective-cosmwasm-stargate-example/src/lib.rs (13)
1-1: LGTM!The
contractmodule is declared as public, which is appropriate for a module that contains the main contract logic.
2-2: LGTM!The
encode_helpermodule is declared as private, which is appropriate for a module that contains helper functions.
3-3: LGTM!The
errormodule is declared as private, which is appropriate for a module that contains error definitions.
4-4: LGTM!The
handlemodule is declared as private, which is appropriate for a module that contains message handling logic.
5-5: LGTM!The
msgmodule is declared as public, which is appropriate for a module that contains message definitions.
6-6: LGTM!The
order_managementmodule is declared as private, which is appropriate for a module that contains order management logic.
7-7: LGTM!The
querymodule is declared as private, which is appropriate for a module that contains query handling logic.
8-8: LGTM!The
replymodule is declared as private, which is appropriate for a module that contains reply handling logic.
9-9: LGTM!The
spot_market_order_msgmodule is declared as private, which is appropriate for a module that contains spot market order message definitions.
10-10: LGTM!The
statemodule is declared as private, which is appropriate for a module that contains state management logic.
11-12: LGTM!The
testingmodule is declared as private and conditionally compiled, which is appropriate for a module that contains testing logic.
13-14: LGTM!The
utilsmodule is declared as public and conditionally compiled, which is appropriate for a module that contains utility functions.
16-16: LGTM!The
ContractErrortype is re-exported publicly from theerrormodule, which is appropriate for a type that is used externally.contracts/injective-cosmwasm-stargate-example/Cargo.toml (1)
1-42: LGTM!The
Cargo.tomlfile correctly sets up the configuration for theinjective-cosmwasm-stargate-examplepackage. The metadata, library crate types, dependencies, and development dependencies are all specified correctly.The optional features in the
[features]section are currently empty. Consider expanding them in the future as the project develops to enable or disable specific functionality as needed.contracts/injective-cosmwasm-stargate-example/src/query.rs (3)
9-27: The past review comment is still valid and applicable.The error messages could be more descriptive to aid debugging and maintenance. Consider including more details in the error messages, such as the input values that caused the error.
29-32: The past review comment is still valid and applicable.Currently, the function directly uses the
?operator without handling specific errors that might arise from thespot_marketmethod. Consider adding specific error handling to provide clearer error messages.
34-37: LGTM!The function logic is correct, and the implementation is accurate.
contracts/injective-cosmwasm-stargate-example/src/spot_market_order_msg.rs (1)
27-54: LGTM!The function correctly constructs a
MsgCreateSpotMarketOrdermessage with the provided parameters. It also ensures that the price and quantity adhere to the market's minimum tick sizes by rounding them using helper functions from theinjective_mathcrate.contracts/injective-cosmwasm-stargate-example/src/order_management.rs (4)
9-14: LGTM!The function correctly constructs a
CosmosMsgencapsulating anAnyMsgwith the providedtype_urlandvalue. The implementation is straightforward and accurate.
16-39: LGTM!The function correctly constructs a
MsgCreateSpotLimitOrderwith the provided parameters. The fields are populated accurately, and the necessary type conversions are performed. The implementation looks good.
41-68: LGTM!The function correctly constructs a
MsgCreateDerivativeLimitOrderwith the provided parameters. It retrieves themarket_idfrom theFullDerivativeMarketstructure and populates the fields accurately. The necessary type conversions are performed, and the implementation looks good.
70-74: LGTM!The function correctly encodes any message type implementing the
prost::Messagetrait into a byte vector. It utilizes theencodemethod from theprostlibrary and returns the encoded message as aResulttype. The implementation is straightforward and accurate.Cargo.toml (2)
11-11: Verify compatibility and test thoroughly.The
base64dependency has been updated to version0.21.5. This update may include enhancements, bug fixes, or potentially breaking changes.Please ensure that this version update doesn't introduce any compatibility issues or unexpected behavior changes in the functionality that relies on the
base64crate. Thoroughly test the base64 encoding and decoding logic to confirm that it behaves as expected.
41-41: Test the JSON serialization and deserialization logic.The
serde_jsondependency has been added at version1.0.111. This suggests an intention to utilize JSON serialization and deserialization capabilities within the codebase.Please ensure that the JSON serialization and deserialization logic is implemented correctly and tested thoroughly. Verify that the expected data structures are correctly serialized to JSON and deserialized from JSON without any data loss or inconsistencies.
contracts/injective-cosmwasm-stargate-example/src/contract.rs (3)
18-22: LGTM!The
instantiatefunction correctly sets the contract version and returns a default response. The implementation looks good.
24-52: LGTM!The
executefunction correctly matches the receivedExecuteMsgand delegates to the corresponding handler functions. The implementation looks good.
54-61: LGTM!The
queryfunction correctly matches the receivedQueryMsgand delegates to the corresponding handler functions. The implementation looks good.contracts/injective-cosmwasm-stargate-example/src/testing/test_auction.rs (4)
46-65: LGTM!The test correctly queries the current auction basket and asserts the expected default values.
56-58: Handle errors gracefully in the test.The test uses
unwrap()which might panic if the query fails. It's better to handle errors gracefully to provide clear messages about what went wrong.Apply this diff to handle errors gracefully:
- let contract_response: QueryStargateResponse = wasm.query(&env.contract_address, &query_msg).unwrap(); - let response: QueryCurrentAuctionBasketResponse = from_json(contract_response).unwrap(); + let contract_response: QueryStargateResponse = wasm.query(&env.contract_address, &query_msg).expect("Failed to query contract"); + let response: QueryCurrentAuctionBasketResponse = from_json(contract_response.value).expect("Failed to decode response");
67-83: LGTM!The test correctly queries the auction parameters and asserts the expected default values. The error handling looks good as well.
85-102: LGTM!The test correctly queries the last auction result and asserts the expected default values. The error handling looks good as well.
contracts/injective-cosmwasm-stargate-example/src/testing/test_bank.rs (5)
11-20: LGTM!The test correctly verifies that the default sending capability is enabled in the bank parameters.
22-36: LGTM!The test correctly verifies that the raw query method returns the expected bank parameters.
38-66: LGTM!The test correctly verifies that the metadata returned from the bank module matches the expected denomination name after creating a new token denomination.
68-92: LGTM!The test correctly verifies that the balance query returns the expected value for a specific user account and denomination.
94-114: LGTM!The test correctly verifies that the total supply query returns the expected amount for a specific denomination.
contracts/injective-cosmwasm-stargate-example/src/handle.rs (4)
16-38: LGTM!The function correctly handles the market spot order by querying the spot market, creating a market order message, and returning a response containing the message.
40-79: LGTM!The function correctly handles the transient spot order by querying the spot market, creating a limit order message, encoding it, and preparing a
MsgExecmessage for execution. It also creates a submessage for execution and caches the order information for future reference.
81-122: LGTM!The function correctly handles the transient derivative order by querying the derivative market, creating a limit order message, encoding it, and preparing a
MsgExecmessage for execution. It also creates a submessage for execution and caches the order information for future reference.
124-139: LGTM!The function correctly saves the order information in the cache. It creates a
CacheOrderInfostruct, loads the existing cache, appends the new order information, and saves the updated cache back to the storage.contracts/injective-cosmwasm-stargate-example/src/testing/test_oracle.rs (3)
20-56: Ensure Robust Error Handling in Test CasesThe test function
test_query_oracle_pricecorrectly sets up an environment and queries oracle prices. However, it uses.unwrap()extensively, which may panic if an error occurs. Consider adding error handling to make the tests more robust and informative in case of failures.
58-91: Check for Potential False Positives in Test AssertionsIn
test_query_oracle_volatility, the test setup usesExchangeType::None, which might not adequately simulate conditions for a non-Nonevolatilityresponse. To ensure the test environment and inputs are correctly configured to test various scenarios, consider modifying the setup to use different exchange types or parameters.
93-126: Optimize Test Setup and ReuseThe function
test_query_pyth_oracle_pricesets up the environment and performs similar actions to other test functions. Consider refactoring to share common setup code across tests to reduce redundancy and improve maintainability.contracts/atomic-order-example/src/tests.rs (1)
147-147: ** Ensure an appropriate minimum notional value is set.**The past review comment suggesting to adjust the
min_notionalvalue according to market requirements is still valid and applicable to the current code segment.Setting the minimum notional value to
FPDecimal::ONEmay not effectively maintain market stability and prevent manipulation. Please consider adjusting the value based on the specific market requirements.contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange.rs (3)
20-37: LGTM!The
test_exchange_paramfunction correctly queries the exchange parameters and asserts the spot market instant listing fee. The logic is sound and the test should pass.
41-101: Test logic is correct but consider optimizing the function.The
test_query_subaccount_depositfunction correctly tests querying subaccount deposits by making deposits, querying the deposits, and asserting the balances. The test logic is comprehensive.However, as mentioned in the past review, the function could be optimized by:
- Reusing query setups
- Improving the logic for handling different deposit scenarios
105-141: LGTM!The
test_query_trader_transient_spot_ordersfunction correctly tests querying transient spot orders by executing an order, extracting thequery_strfrom the emitted event, and asserting that it contains the expected order information. The test logic is sound.contracts/injective-cosmwasm-mock/src/testing/test_exchange_derivative.rs (1)
Line range hint
55-76: LGTM!The introduction of the
min_notionalvariable improves code clarity and maintainability by avoiding the use of a hardcoded string value. The variable is initialized usingFPDecimal::must_from_str("1"), which is a safe way to convert a string to a fixed-point decimal value. The usage of themin_notionalvariable in the market query structure is consistent with the variable declaration.contracts/injective-cosmwasm-stargate-example/src/testing/test_exchange_derivative.rs (9)
31-52: LGTM!The test follows the correct setup, execution, and assertion flow. It validates the key aspects of the perpetual market info response.
56-112: LGTM!The test follows the correct setup, execution, and assertion flow. It validates the key parameters of the derivative market response against the values used during market creation.
116-149: LGTM!The test follows the correct setup, execution, and assertion flow. It validates the presence of the effective subaccount position state in the response.
153-199: LGTM!The test follows the correct setup, execution, and assertion flow. It validates the presence of the vanilla subaccount position state in the response after placing an order and adding more liquidity.
203-249: LGTM!The test follows the correct setup, execution, and assertion flow. It validates the key parameters of the trader's derivative order response against the values used during order placement.
253-270: LGTM!The test follows the correct setup, execution, and assertion flow. It validates the initial state of the perpetual market funding, ensuring that the cumulative funding and price are zero.
274-290: LGTM!The test follows the correct setup, execution, and assertion flow. It validates the derivative market's mid price and the best buy and sell prices after adding initial liquidity.
294-344: LGTM!The test follows the correct setup, execution, and assertion flow. It validates the price levels of the buy and sell orders in the derivative market's orderbook after adding liquidity orders.
348-394: LGTM!The test follows the correct setup, execution, and assertion flow. It validates the presence of the expected order information in the trader's transient derivative orders query response.
contracts/injective-cosmwasm-mock/src/testing/test_exchange.rs (1)
157-157: LGTM!The change enhances the clarity and maintainability of the code by using a variable
min_notionalinstead of a literal string value. This may facilitate future adjustments to the minimum notional value without requiring changes in multiple places.The change is unlikely to have any negative impact on the behavior of the test or the system. It improves the readability and maintainability of the code.
Also applies to: 168-168
contracts/injective-cosmwasm-stargate-example/src/utils.rs (14)
51-54: LGTM!The
UserInfostruct is well-defined and groups together relevant fields.
55-65: LGTM!The
Setupstruct is well-defined and encapsulates the necessary components for initializing a test environment.
67-71: LGTM!The
ExchangeTypeenum is well-defined and represents the different types of exchanges.
182-202: LGTM!The
get_perpetual_market_idfunction correctly retrieves the perpetual market ID based on the provided ticker.
205-209: LGTM!The
HumanOrderstruct is well-defined and represents a human-readable order with relevant fields.
210-232: LGTM!The
add_spot_order_asfunction correctly adds a spot order for a specific trader using the provided parameters.
234-250: LGTM!The
add_spot_ordersfunction correctly adds multiple spot orders by initializing a new account and iterating over the provided orders.
252-295: LGTM!The
get_initial_liquidity_orders_vectorfunction correctly returns a vector of initial liquidity orders.
297-299: LGTM!The
add_spot_initial_liquidityfunction correctly adds initial liquidity for a spot market by calling theadd_spot_ordersfunction with the appropriate parameters.
301-324: LGTM!The
get_initial_perp_liquidity_orders_vectorfunction correctly returns a vector of initial liquidity orders for a perpetual market.
326-359: LGTM!The
add_derivative_order_asfunction correctly adds a derivative order for a specific trader using the provided parameters.
361-376: LGTM!The
add_derivative_ordersfunction correctly adds multiple derivative orders by initializing a new trader account and iterating over the provided orders.
378-380: LGTM!The
add_perp_initial_liquidityfunction correctly adds initial liquidity for a perpetual market by calling theadd_derivative_ordersfunction with the appropriate parameters.
398-420: LGTM!The
create_generic_authorizationfunction correctly creates a generic authorization for a specific granter and grantee using the provided parameters.
| let msg = create_spot_market_order(price, quantity, order_type, sender, subaccount_id, fee_recipient, market); | ||
|
|
||
| let mut order_bytes = vec![]; | ||
| Exchange::MsgCreateSpotMarketOrder::encode(&msg, &mut order_bytes).unwrap(); |
There was a problem hiding this comment.
Handle the potential error instead of using .unwrap().
The .unwrap() call on line 19 can cause a panic if the encoding fails. It's safer to handle this error properly and return a StdResult instead.
Apply this diff to handle the error:
-Exchange::MsgCreateSpotMarketOrder::encode(&msg, &mut order_bytes).unwrap();
+Exchange::MsgCreateSpotMarketOrder::encode(&msg, &mut order_bytes).map_err(|e| StdError::generic_err(format!("Failed to encode spot market order: {}", e)))?;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.
| Exchange::MsgCreateSpotMarketOrder::encode(&msg, &mut order_bytes).unwrap(); | |
| Exchange::MsgCreateSpotMarketOrder::encode(&msg, &mut order_bytes).map_err(|e| StdError::generic_err(format!("Failed to encode spot market order: {}", e)))?; |
| #[cfg_attr(not(feature = "library"), entry_point)] | ||
| pub fn reply(deps: DepsMut<InjectiveQueryWrapper>, _env: Env, msg: Reply) -> Result<Response, ContractError> { | ||
| match msg.id { | ||
| CREATE_SPOT_ORDER_REPLY_ID => handle_create_order_reply_stargate(deps, &msg), | ||
| CREATE_DERIVATIVE_ORDER_REPLY_ID => handle_create_derivative_order_reply_stargate(deps, &msg), | ||
| _ => Err(ContractError::UnrecognizedReply(msg.id)), | ||
| } | ||
| } |
There was a problem hiding this comment.
Address potential conflict with existing reply IDs and improve error handling.
The past review comments indicate a potential conflict with the CREATE_SPOT_ORDER_REPLY_ID and CREATE_DERIVATIVE_ORDER_REPLY_ID constants across different modules. Please ensure that these constants are unique to avoid any unexpected behavior.
Additionally, consider improving the error handling in the reply function by providing a more descriptive error message for unrecognized reply IDs.
Apply this diff to improve the error handling:
- _ => Err(ContractError::UnrecognizedReply(msg.id)),
+ _ => Err(ContractError::UnrecognizedReply(format!("Unrecognized reply ID: {}", msg.id))),Committable suggestion was skipped due to low confidence.
| response_str = match stargate_response { | ||
| Ok(binary) => String::from_utf8(binary.to_vec()).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)), | ||
| Err(e) => format!("Error: {:?}", e), | ||
| }; |
There was a problem hiding this comment.
Optimize Error Handling and Response Decoding
The error messages use debug formatting ({:?}) which might not be suitable for production logs. Also, String::from_utf8 does not handle potential invalid UTF-8 data in the response.
- response_str = match stargate_response {
- Ok(binary) => String::from_utf8(binary.to_vec()).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)),
- Err(e) => format!("Error: {:?}", e),
+ response_str = match stargate_response {
+ Ok(binary) => String::from_utf8_lossy(&binary).into_owned(),
+ Err(e) => format!("Error: {}", e),
};This change ensures that:
- Error messages are formatted properly for production logs.
- Potential invalid UTF-8 data in the response is handled gracefully using
String::from_utf8_lossy.
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.
| response_str = match stargate_response { | |
| Ok(binary) => String::from_utf8(binary.to_vec()).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)), | |
| Err(e) => format!("Error: {:?}", e), | |
| }; | |
| response_str = match stargate_response { | |
| Ok(binary) => String::from_utf8_lossy(&binary).into_owned(), | |
| Err(e) => format!("Error: {}", e), | |
| }; |
| response_str = match stargate_response { | ||
| Ok(binary) => String::from_utf8(binary.to_vec()).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)), | ||
| Err(e) => format!("Error: {:?}", e), | ||
| }; |
There was a problem hiding this comment.
Optimize Error Handling and Response Decoding
The error messages use debug formatting ({:?}) which might not be suitable for production logs. Also, String::from_utf8 does not handle potential invalid UTF-8 data in the response.
- response_str = match stargate_response {
- Ok(binary) => String::from_utf8(binary.to_vec()).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)),
- Err(e) => format!("Error: {:?}", e),
+ response_str = match stargate_response {
+ Ok(binary) => String::from_utf8_lossy(&binary).into_owned(),
+ Err(e) => format!("Error: {}", e),
};This change ensures that:
- Error messages are formatted properly for production logs.
- Potential invalid UTF-8 data in the response is handled gracefully using
String::from_utf8_lossy.
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.
| response_str = match stargate_response { | |
| Ok(binary) => String::from_utf8(binary.to_vec()).unwrap_or_else(|e| format!("Failed to decode binary to string: {:?}", e)), | |
| Err(e) => format!("Error: {:?}", e), | |
| }; | |
| response_str = match stargate_response { | |
| Ok(binary) => String::from_utf8_lossy(&binary).into_owned(), | |
| Err(e) => format!("Error: {}", e), | |
| }; |
| fn test_query_trader_spot_market_order() { | ||
| let env = Setup::new(ExchangeType::Spot); | ||
| let wasm = Wasm::new(&env.app); | ||
| let market_id = env.market_id.unwrap(); | ||
|
|
||
| let subaccount_id = checked_address_to_subaccount_id(&Addr::unchecked(env.contract_address.to_owned()), 0u32); | ||
|
|
||
| execute_all_authorizations(&env.app, &env.users[0].account, env.contract_address.clone()); | ||
| add_spot_initial_liquidity(&env.app, market_id.clone()); | ||
|
|
||
| let (scale_price, scale_quantity) = scale_price_quantity_spot_market("9.8", "1", &BASE_DECIMALS, "E_DECIMALS); | ||
|
|
||
| wasm.execute( | ||
| &env.contract_address, | ||
| &ExecuteMsg::TestMarketOrderStargate { | ||
| market_id: MarketId::new(market_id).unwrap(), | ||
| subaccount_id: subaccount_id.clone(), | ||
| price: scale_price.to_string(), | ||
| quantity: scale_quantity.to_string(), | ||
| }, | ||
| &[coin(1000000000000000000000u128, BASE_DENOM)], | ||
| &env.users[0].account, | ||
| ) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
Add assertions to validate the spot market order execution.
The test_query_trader_spot_market_order function executes a spot market order using the correct message and parameters. However, it lacks assertions to validate if the order was executed successfully.
Consider adding assertions to check:
- The response events for order execution details
- The updated balances of the trader's subaccount
- The updated orderbook state
| pub fn new(exchange_type: ExchangeType) -> Self { | ||
| let app = InjectiveTestApp::new(); | ||
|
|
||
| let wasm = Wasm::new(&app); | ||
| let exchange = Exchange::new(&app); | ||
|
|
||
| let mut market_id = None; | ||
|
|
||
| let mut denoms = HashMap::new(); | ||
| denoms.insert("atom".to_string(), ATOM_DENOM.to_string()); | ||
| denoms.insert("quote".to_string(), QUOTE_DENOM.to_string()); | ||
| denoms.insert("base".to_string(), BASE_DENOM.to_string()); | ||
|
|
||
| let signer = app.init_account(&[str_coin("1000000", BASE_DENOM, BASE_DECIMALS)]).unwrap(); | ||
|
|
||
| let validator = app.get_first_validator_signing_account(BASE_DENOM.to_string(), 1.2f64).unwrap(); | ||
|
|
||
| let owner = app | ||
| .init_account(&[ | ||
| str_coin("1000000", ATOM_DENOM, ATOM_DECIMALS), | ||
| str_coin("1000000", BASE_DENOM, BASE_DECIMALS), | ||
| str_coin("1000000", QUOTE_DENOM, QUOTE_DECIMALS), | ||
| ]) | ||
| .unwrap(); | ||
|
|
||
| let mut users: Vec<UserInfo> = Vec::new(); | ||
| for _ in 0..10 { | ||
| let user = app | ||
| .init_account(&[ | ||
| str_coin("1000000", ATOM_DENOM, ATOM_DECIMALS), | ||
| str_coin("1000000", BASE_DENOM, BASE_DECIMALS), | ||
| str_coin("1000", QUOTE_DENOM, QUOTE_DECIMALS), | ||
| ]) | ||
| .unwrap(); | ||
|
|
||
| let user_subaccount_id = checked_address_to_subaccount_id(&Addr::unchecked(user.address()), 0u32); | ||
|
|
||
| users.push(UserInfo { | ||
| account: user, | ||
| subaccount_id: user_subaccount_id, | ||
| }); | ||
| } | ||
|
|
||
| let wasm_byte_code = std::fs::read(wasm_file("injective_cosmwasm_stargate_example".to_string())).unwrap(); | ||
| let code_id = wasm.store_code(&wasm_byte_code, None, &owner).unwrap().data.code_id; | ||
|
|
||
| // Instantiate contract | ||
| let contract_address: String = wasm | ||
| .instantiate(code_id, &InstantiateMsg {}, Some(&owner.address()), Some("mock-contract"), &[], &owner) | ||
| .unwrap() | ||
| .data | ||
| .address; | ||
|
|
||
| assert!(!contract_address.is_empty(), "Contract address is empty"); | ||
|
|
||
| send(&Bank::new(&app), "1000000000000000000000", BASE_DENOM, &owner, &validator); | ||
|
|
||
| add_exchange_admin(&app, &validator, owner.address().to_string()); | ||
|
|
||
| launch_insurance_fund( | ||
| &app, | ||
| &owner, | ||
| "INJ/USDT", | ||
| denoms["quote"].as_str(), | ||
| denoms["base"].as_str(), | ||
| denoms["quote"].as_str(), | ||
| OracleType::PriceFeed, | ||
| ); | ||
|
|
||
| launch_price_feed_oracle( | ||
| &app, | ||
| &signer, | ||
| &validator, | ||
| denoms["base"].as_str(), | ||
| denoms["quote"].as_str(), | ||
| human_to_dec("10.01", BASE_DECIMALS).to_string(), | ||
| ); | ||
|
|
||
| match exchange_type { | ||
| ExchangeType::Spot => { | ||
| market_id = Some(launch_spot_market(&exchange, &owner, "INJ/USDT".to_string())); | ||
| } | ||
| ExchangeType::Derivative => { | ||
| market_id = Some(launch_perp_market(&exchange, &owner, "INJ/USDT".to_string())); | ||
| } | ||
| ExchangeType::None => {} | ||
| } | ||
|
|
||
| Self { | ||
| app, | ||
| owner, | ||
| signer, | ||
| validator, | ||
| users, | ||
| denoms, | ||
| contract_address, | ||
| code_id, | ||
| market_id, | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider breaking down the function into smaller, more focused functions.
The Setup::new function is quite large and performs many initialization steps. To improve readability and maintainability, consider breaking it down into smaller, more focused functions. For example, you could extract the initialization of accounts, contract details, and market details into separate functions.
…-ext Feat/stargate tester authz ext
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
contracts/injective-cosmwasm-stargate-example/src/testing/authz.rs (1)
94-138: Minor suggestion: Remove commented out lines.The test function
test_query_grantsis well-structured and correctly tests the querying of specific grants. The setup, authorization execution, query message construction, and verification logic for both users are all implemented correctly.However, there are a couple of commented out lines (114-115) that can be removed to improve code cleanliness.
Apply this diff to remove the commented out lines:
- // let query_result = get_stargate_query_result::<GranteeGrantsResponse>(wasm.query(&env.contract_address, &query_msg)).unwrap(); - // println!("{:?}", query_result);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- contracts/atomic-order-example/.circleci/config.yml (0 hunks)
- contracts/atomic-order-example/Cargo.toml (1 hunks)
- contracts/dummy/Cargo.toml (1 hunks)
- contracts/injective-cosmwasm-mock/Cargo.toml (1 hunks)
- contracts/injective-cosmwasm-stargate-example/Cargo.toml (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/authz.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/mod.rs (1 hunks)
- contracts/injective-cosmwasm-stargate-example/src/testing/type_helpers.rs (1 hunks)
- packages/injective-std/Cargo.toml (1 hunks)
- packages/injective-testing/Cargo.toml (1 hunks)
Files not reviewed due to no reviewable changes (1)
- contracts/atomic-order-example/.circleci/config.yml
Files skipped from review due to trivial changes (1)
- contracts/injective-cosmwasm-mock/Cargo.toml
Files skipped from review as they are similar to previous changes (2)
- contracts/injective-cosmwasm-stargate-example/Cargo.toml
- contracts/injective-cosmwasm-stargate-example/src/testing/type_helpers.rs
Additional comments not posted (10)
contracts/injective-cosmwasm-stargate-example/src/testing/mod.rs (1)
1-8: LGTM!The modular structure of the testing directory is well-organized and follows best practices. Each submodule focuses on testing specific functionalities, which enhances code maintainability and ensures thorough testing coverage.
This approach promotes robustness and reliability in the overall system.
packages/injective-std/Cargo.toml (1)
15-15: Verify the impact of removing theworkspace = trueattribute.The removal of the
workspace = trueattribute from theinjective-std-derivedependency suggests a significant change in the dependency management strategy for theinjective-stdpackage.This change may affect how dependencies are resolved and could imply a shift in how the project is structured or how the
injective-std-derivepackage is intended to be used within the overall project context.Please ensure that this change aligns with the intended dependency management approach and verify that it does not introduce any unintended consequences for the project's build process or dependency resolution.
packages/injective-testing/Cargo.toml (1)
15-17: Verify the intentional removal ofworkspace = truefrom the dependencies.The changes involve removing the
workspace = trueattribute from the following dependencies:
injective-cosmwasminjective-mathinjective-stdThis change indicates a shift in how these packages are managed within the project. Please ensure that:
- The removal of
workspace = trueis intentional.- The change aligns with the project's dependency management strategy.
- There are no potential build issues or version conflicts introduced by this change.
contracts/dummy/Cargo.toml (1)
33-33: Verify the implications of removingworkspace = true.The change looks good from a correctness perspective, as the relative
pathattribute is retained. However, please ensure that removingworkspace = truedoes not introduce any unintended consequences for the build process or dependency management.contracts/atomic-order-example/Cargo.toml (3)
35-35: Verify the impact of the dependency declaration change.The
workspace = trueattribute has been removed from theinjective-cosmwasmdependency declaration. This change suggests a shift in how the package is managed within the project.Please ensure that this modification does not adversely affect the project's build process or workspace management. Verify that the dependency can still be resolved correctly and that the removal of the workspace configuration is intentional.
36-36: Verify the impact of the dependency declaration change.The
workspace = trueattribute has been removed from theinjective-mathdependency declaration. This change suggests a shift in how the package is managed within the project.Please ensure that this modification does not adversely affect the project's build process or workspace management. Verify that the dependency can still be resolved correctly and that the removal of the workspace configuration is intentional.
37-37: Verify the impact of the dependency declaration change.The
workspace = trueattribute has been removed from theinjective-stddependency declaration. This change suggests a shift in how the package is managed within the project.Please ensure that this modification does not adversely affect the project's build process or workspace management. Verify that the dependency can still be resolved correctly and that the removal of the workspace configuration is intentional.
contracts/injective-cosmwasm-stargate-example/src/testing/authz.rs (3)
13-61: LGTM!The test function
test_query_grantee_grantsis well-structured and correctly tests the querying of grantee grants. The setup, authorization execution, query message construction, and verification logic are all implemented correctly.
63-92: LGTM!The test function
test_query_granter_grantsis well-structured and correctly tests the querying of granter grants. The setup, authorization execution, query message construction, and verification logic for both users are all implemented correctly.
140-154: LGTM!The helper function
create_stargate_responseis well-implemented and correctly constructs aStargateQueryGranteeGrantsResponsebased on the provided messages, granter, and grantee. The logic for mapping messages to grants is accurate.
|
Just fix merge conflicts |
Stargate queries contract example and test.
Summary by CodeRabbit
Release Notes
New Features
Enhancements
min_notionalin market responses to clarify transaction constraints.Refactor
Documentation
min_notionalfield inMsgInstantSpotMarketLaunchfor better understanding.Style
displaymodule to be publicly accessible in theFPDecimalmodule.