feat: made small changes to the refactor #241
Conversation
WalkthroughThe updates involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Exchange
participant Subaccount
participant Market
User->>Exchange: Query total value
Exchange->>Subaccount: Retrieve trade deposits
Subaccount->>Market: Query effective position
Market-->>Subaccount: Return balance and margin
Subaccount-->>Exchange: Return total value
Exchange-->>User: Provide total value
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 (
|
| / Uint128::one(); | ||
|
|
||
| let effective_position = exchange | ||
| .query_subaccount_effective_position_in_market(&QuerySubaccountEffectivePositionInMarketRequest { market_id, subaccount_id }) |
There was a problem hiding this comment.
why dont a wrapper fuction for query_subaccount_effective_position_in_market ?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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 (14)
- Cargo.toml (1 hunks)
- build_release.sh (1 hunks)
- contracts/injective-cosmwasm-mock/src/utils.rs (1 hunks)
- packages/injective-testing/CHANGELOG.md (1 hunks)
- packages/injective-testing/Cargo.toml (1 hunks)
- packages/injective-testing/src/lib.rs (1 hunks)
- packages/injective-testing/src/multi_test/chain_mock.rs (1 hunks)
- packages/injective-testing/src/multi_test/mod.rs (1 hunks)
- packages/injective-testing/src/test_tube/authz.rs (1 hunks)
- packages/injective-testing/src/test_tube/bank.rs (1 hunks)
- packages/injective-testing/src/test_tube/exchange.rs (3 hunks)
- packages/injective-testing/src/test_tube/insurance.rs (1 hunks)
- packages/injective-testing/src/test_tube/mod.rs (1 hunks)
- packages/injective-testing/src/test_tube/oracle.rs (1 hunks)
Files skipped from review due to trivial changes (9)
- contracts/injective-cosmwasm-mock/src/utils.rs
- packages/injective-testing/CHANGELOG.md
- packages/injective-testing/Cargo.toml
- packages/injective-testing/src/multi_test/chain_mock.rs
- packages/injective-testing/src/test_tube/authz.rs
- packages/injective-testing/src/test_tube/bank.rs
- packages/injective-testing/src/test_tube/insurance.rs
- packages/injective-testing/src/test_tube/mod.rs
- packages/injective-testing/src/test_tube/oracle.rs
Additional comments not posted (12)
packages/injective-testing/src/multi_test/mod.rs (2)
1-1: LGTM!Making the
address_generatormodule public increases its accessibility.The code changes are approved.
2-2: LGTM!Making the
chain_mockmodule public increases its accessibility.The code changes are approved.
packages/injective-testing/src/lib.rs (3)
1-1: LGTM!Making the
mocksmodule public increases its accessibility.The code changes are approved.
2-2: LGTM!Making the
multi_testmodule public increases its accessibility.The code changes are approved.
3-3: LGTM!Making the
test_tubemodule public increases its accessibility.The code changes are approved.
build_release.sh (1)
11-11: LGTM!Updating the Docker image version may include enhancements, bug fixes, or new features.
The code changes are approved.
Cargo.toml (6)
23-23: LGTM!The dependency
injective-cosmwasmhas been updated to include a local path, indicating a shift towards local development.
24-24: LGTM!The dependency
injective-mathhas been updated to include a local path, indicating a shift towards local development.
25-25: LGTM!The dependency
injective-stdhas been updated to include a local path, indicating a shift towards local development.
26-26: LGTM!The dependency
injective-std-derivehas been updated to include a local path, indicating a shift towards local development.
27-27: LGTM!The dependency
injective-test-tubehas been updated to a newer version, ensuring compatibility with newer versions of the dependency.
28-28: LGTM!The dependency
injective-testinghas been updated to include a local path and a newer version, indicating a shift towards local development and ensuring compatibility with newer versions of the dependency.
| pub fn get_spot_market_id(exchange: &Exchange<InjectiveTestApp>, ticker: String) -> String { | ||
| let spot_markets = exchange | ||
| .query_spot_markets(&QuerySpotMarketsRequest { | ||
| status: "Active".to_string(), | ||
| market_ids: vec![], | ||
| }) | ||
| .unwrap() | ||
| .markets; | ||
|
|
||
| let market = spot_markets.iter().find(|m| m.ticker == ticker).unwrap(); | ||
|
|
||
| market.market_id.to_string() |
There was a problem hiding this comment.
Enhance error handling.
The function is correctly implemented but can be improved for error handling. Apply this diff to enhance the function:
pub fn get_spot_market_id(exchange: &Exchange<InjectiveTestApp>, ticker: String) -> String {
let spot_markets = exchange
.query_spot_markets(&QuerySpotMarketsRequest {
status: "Active".to_string(),
market_ids: vec![],
})
.unwrap()
.markets;
let market = spot_markets.iter().find(|m| m.ticker == ticker).unwrap_or_else(|| {
panic!("Failed to find spot market with ticker: {}", ticker);
});
market.market_id.to_string()
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn get_spot_market_id(exchange: &Exchange<InjectiveTestApp>, ticker: String) -> String { | |
| let spot_markets = exchange | |
| .query_spot_markets(&QuerySpotMarketsRequest { | |
| status: "Active".to_string(), | |
| market_ids: vec![], | |
| }) | |
| .unwrap() | |
| .markets; | |
| let market = spot_markets.iter().find(|m| m.ticker == ticker).unwrap(); | |
| market.market_id.to_string() | |
| pub fn get_spot_market_id(exchange: &Exchange<InjectiveTestApp>, ticker: String) -> String { | |
| let spot_markets = exchange | |
| .query_spot_markets(&QuerySpotMarketsRequest { | |
| status: "Active".to_string(), | |
| market_ids: vec![], | |
| }) | |
| .unwrap() | |
| .markets; | |
| let market = spot_markets.iter().find(|m| m.ticker == ticker).unwrap_or_else(|| { | |
| panic!("Failed to find spot market with ticker: {}", ticker); | |
| }); | |
| market.market_id.to_string() | |
| } |
| pub fn get_subaccount_total_value(exchange: &Exchange<InjectiveTestApp>, market_id: String, subaccount_id: String, denom: String) -> Uint128 { | ||
| let trade_deposits_during = exchange | ||
| .query_subaccount_deposits(&QuerySubaccountDepositsRequest { | ||
| subaccount_id: subaccount_id.clone(), | ||
| subaccount: None, | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| let total_balance = Uint128::from_str(&trade_deposits_during.deposits[&denom].total_balance) | ||
| .unwrap_or(Uint128::zero()) // Use zero if the result is an Err | ||
| / Uint128::one(); | ||
|
|
||
| let effective_position = exchange | ||
| .query_subaccount_effective_position_in_market(&QuerySubaccountEffectivePositionInMarketRequest { market_id, subaccount_id }) | ||
| .unwrap(); | ||
|
|
||
| let effective_margin = effective_position.state.as_ref().map_or(Uint128::zero(), |state| { | ||
| Uint128::from_str(&state.effective_margin).unwrap_or(Uint128::zero()) | ||
| }) / Uint128::one(); | ||
|
|
||
| total_balance + effective_margin |
There was a problem hiding this comment.
Improve readability and error handling.
The function is correctly implemented but can be improved for readability and error handling. Apply this diff to enhance the function:
pub fn get_subaccount_total_value(exchange: &Exchange<InjectiveTestApp>, market_id: String, subaccount_id: String, denom: String) -> Uint128 {
let trade_deposits_during = exchange
.query_subaccount_deposits(&QuerySubaccountDepositsRequest {
subaccount_id: subaccount_id.clone(),
subaccount: None,
})
.unwrap();
let total_balance = Uint128::from_str(&trade_deposits_during.deposits[&denom].total_balance)
.unwrap_or_else(|_| {
eprintln!("Failed to parse total balance for denom: {}", denom);
Uint128::zero()
});
let effective_position = exchange
.query_subaccount_effective_position_in_market(&QuerySubaccountEffectivePositionInMarketRequest { market_id, subaccount_id })
.unwrap();
let effective_margin = effective_position.state.as_ref().map_or(Uint128::zero(), |state| {
Uint128::from_str(&state.effective_margin).unwrap_or_else(|_| {
eprintln!("Failed to parse effective margin for market_id: {}", market_id);
Uint128::zero()
})
});
total_balance + effective_margin
}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 get_subaccount_total_value(exchange: &Exchange<InjectiveTestApp>, market_id: String, subaccount_id: String, denom: String) -> Uint128 { | |
| let trade_deposits_during = exchange | |
| .query_subaccount_deposits(&QuerySubaccountDepositsRequest { | |
| subaccount_id: subaccount_id.clone(), | |
| subaccount: None, | |
| }) | |
| .unwrap(); | |
| let total_balance = Uint128::from_str(&trade_deposits_during.deposits[&denom].total_balance) | |
| .unwrap_or(Uint128::zero()) // Use zero if the result is an Err | |
| / Uint128::one(); | |
| let effective_position = exchange | |
| .query_subaccount_effective_position_in_market(&QuerySubaccountEffectivePositionInMarketRequest { market_id, subaccount_id }) | |
| .unwrap(); | |
| let effective_margin = effective_position.state.as_ref().map_or(Uint128::zero(), |state| { | |
| Uint128::from_str(&state.effective_margin).unwrap_or(Uint128::zero()) | |
| }) / Uint128::one(); | |
| total_balance + effective_margin | |
| pub fn get_subaccount_total_value(exchange: &Exchange<InjectiveTestApp>, market_id: String, subaccount_id: String, denom: String) -> Uint128 { | |
| let trade_deposits_during = exchange | |
| .query_subaccount_deposits(&QuerySubaccountDepositsRequest { | |
| subaccount_id: subaccount_id.clone(), | |
| subaccount: None, | |
| }) | |
| .unwrap(); | |
| let total_balance = Uint128::from_str(&trade_deposits_during.deposits[&denom].total_balance) | |
| .unwrap_or_else(|_| { | |
| eprintln!("Failed to parse total balance for denom: {}", denom); | |
| Uint128::zero() | |
| }); | |
| let effective_position = exchange | |
| .query_subaccount_effective_position_in_market(&QuerySubaccountEffectivePositionInMarketRequest { market_id, subaccount_id }) | |
| .unwrap(); | |
| let effective_margin = effective_position.state.as_ref().map_or(Uint128::zero(), |state| { | |
| Uint128::from_str(&state.effective_margin).unwrap_or_else(|_| { | |
| eprintln!("Failed to parse effective margin for market_id: {}", market_id); | |
| Uint128::zero() | |
| }) | |
| }); | |
| total_balance + effective_margin | |
| } |
Summary by CodeRabbit
New Features
get_subaccount_total_value, for calculating the total value of a subaccount in a specific market.get_spot_market_idfunction for querying active spot markets.Improvements
Bug Fixes
Documentation