Skip to content

[BFP-1281] Update GET /account/trades symbol parameter to be optional#42

Merged
kevin-ip merged 3 commits intomainfrom
kevin/BFP-1281-update-get-account-trades-symbol-be-optional
Mar 27, 2025
Merged

[BFP-1281] Update GET /account/trades symbol parameter to be optional#42
kevin-ip merged 3 commits intomainfrom
kevin/BFP-1281-update-get-account-trades-symbol-be-optional

Conversation

@kevin-ip
Copy link
Copy Markdown
Contributor

@kevin-ip kevin-ip commented Mar 27, 2025

User description

so that we can fetch trades from all symbols


PR Type

Enhancement, Documentation


Description

  • Made the symbol parameter optional in GET /account/trades API.

    • Allows fetching trades across all markets when symbol is not specified.
  • Updated SDKs (TypeScript, Rust, Python) to reflect the optional symbol parameter.

  • Adjusted examples in SDKs to align with the updated API behavior.

  • Updated documentation to clarify the optional nature of the symbol parameter.


Changes walkthrough 📝

Relevant files
Enhancement
5 files
example.ts
Adjusted example to handle optional `symbol` parameter.   
+5/-8     
api.ts
Made `symbol` parameter optional in TypeScript SDK.           
+8/-10   
account-trades.rs
Updated Rust example to handle optional `symbol`.               
+2/-2     
account_data_api.rs
Made `symbol` parameter optional in Rust SDK.                       
+4/-2     
account_data_api.py
Made `symbol` parameter optional in Python SDK.                   
+6/-6     
Documentation
3 files
AccountDataApi.md
Updated Python SDK documentation for optional `symbol`.   
+4/-4     
account-data-api.yaml
Updated OpenAPI spec to make `symbol` optional.                   
+1/-2     
AccountDataApi.md
Updated Rust SDK documentation for optional `symbol`.       
+1/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @kevin-ip kevin-ip requested a review from a team March 27, 2025 18:09
    @qodo-code-review
    Copy link
    Copy Markdown

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Unused Import

    The send_request function is called with Some(symbols::perps::ETH) but the function doesn't use the Option wrapper in its implementation, which might cause type mismatch issues.

    let trades = send_request(&auth_token, Some(symbols::perps::ETH)).await?;
    Commented Code

    There are commented out lines related to deposit functionality. Consider removing these comments if they're not needed or adding a more descriptive comment explaining why they're disabled.

    // Disable for now as the account does not have enough coins
    // await client.deposit((0.03*1e9).toString());

    @qodo-code-review
    Copy link
    Copy Markdown

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Simplify error handling logic

    The IIFE (Immediately Invoked Function Expression) used to throw an error is
    unnecessarily complex. Use a simpler conditional expression or if/else statement
    for better readability.

    ts/sdk/example.ts [121-122]

     // Find the first market available if any
    -const perpMarket = exchangeInfo.markets.length > 0 ? exchangeInfo.markets[0] : (() => { throw new Error("No market is available"); })();
    +if (exchangeInfo.markets.length === 0) {
    +  throw new Error("No market is available");
    +}
    +const perpMarket = exchangeInfo.markets[0];
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion improves code readability by replacing the complex IIFE with a more straightforward if/else pattern, making the error handling logic clearer and easier to maintain.

    Medium
    Remove unnecessary string conversion

    The code is unnecessarily converting the string reference to a String with
    to_string() when it could just use the reference directly, which is more
    efficient.

    rust/gen/bluefin_api/src/apis/account_data_api.rs [180-182]

     if let Some(ref param_value) = p_symbol {
    -    req_builder = req_builder.query(&[("symbol", &param_value.to_string())]);
    +    req_builder = req_builder.query(&[("symbol", param_value)]);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly identifies an unnecessary string conversion that could be eliminated for better efficiency, as the param_value reference can be used directly in the query.

    Low
    • More

    @kevin-ip kevin-ip merged commit 1e74376 into main Mar 27, 2025
    @kevin-ip kevin-ip deleted the kevin/BFP-1281-update-get-account-trades-symbol-be-optional branch March 27, 2025 18:13
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants