Skip to content

Adjust SDKs for new WebSocket openapi definitions#55

Merged
nikita-seedlabs merged 5 commits intomainfrom
nikita/adjust_sdk_post_ws_changes
Apr 3, 2025
Merged

Adjust SDKs for new WebSocket openapi definitions#55
nikita-seedlabs merged 5 commits intomainfrom
nikita/adjust_sdk_post_ws_changes

Conversation

@nikita-seedlabs
Copy link
Copy Markdown
Contributor

@nikita-seedlabs nikita-seedlabs commented Apr 3, 2025

PR Type

Enhancement, Bug fix, Documentation, Tests


Description

  • Introduced AccountAggregatedTradeUpdate model for aggregated trade details.

    • Added support for AccountAggregatedTradeUpdate in TypeScript, Rust, and Python SDKs.
    • Updated WebSocket event handling to include AccountAggregatedTradeUpdate.
  • Replaced Side with PositionSide and TradeSide for better clarity.

    • Updated related models and schemas in TypeScript, Rust, and Python SDKs.
  • Removed redundant models like RecentTradesUpdate and Side.

    • Consolidated trade-related data into the Trade model.
  • Updated documentation to reflect schema and model changes.

    • Added new documentation for AccountAggregatedTradeUpdate.
  • Added unit tests for AccountAggregatedTradeUpdate in Python SDK.


Changes walkthrough 📝

Relevant files
Enhancement
13 files
api.ts
Added AccountAggregatedTradeUpdate and updated trade-related models.
+25/-182
account_aggregated_trade_update.rs
Added `AccountAggregatedTradeUpdate` model in Rust.           
+29/-0   
account_data_stream.rs
Added `AccountAggregatedTradeUpdate` to `AccountDataStream` enum.
+3/-0     
account_event_type.rs
Added `AccountAggregatedTradeUpdate` to `AccountEventType` enum.
+3/-0     
account_stream_message.rs
Added `AccountAggregatedTradeUpdate` to `AccountStreamMessage`.
+8/-1     
account_stream_message_payload.rs
Added AccountAggregatedTradeUpdate to AccountStreamMessagePayload.
+1/-0     
account_trade_update.rs
Refactored `AccountTradeUpdate` to use `Trade` model.       
+4/-58   
recent_trades_updates.rs
Replaced `RecentTradesUpdate` with `Trade` in `RecentTradesUpdates`.
+4/-4     
trade.rs
Simplified `Trade` model by removing redundant enums.       
+1/-17   
account_aggregated_trade_update.py
Added `AccountAggregatedTradeUpdate` model in Python SDK.
+91/-0   
account_trade_update.py
Refactored `AccountTradeUpdate` to use `Trade` model in Python SDK.
+9/-35   
listener.py
Added WebSocket handling for `AccountAggregatedTradeUpdate`.
+2/-0     
common.yaml
Simplified `Trade` schema by removing redundant enums.     
+1/-5     
Bug fix
2 files
account_position_update.rs
Replaced `Side` with `PositionSide` in `AccountPositionUpdate`.
+2/-2     
active_order_update.rs
Replaced `Side` with `TradeSide` in `ActiveOrderUpdate`. 
+2/-2     
Configuration changes
2 files
mod.rs
Registered `AccountAggregatedTradeUpdate` model in Rust module.
+2/-6     
__init__.py
Added `AccountAggregatedTradeUpdate` to Python SDK imports.
+1/-3     
Tests
1 files
test_account_aggregated_trade_update.py
Added unit tests for `AccountAggregatedTradeUpdate`.         
+86/-0   
Documentation
2 files
bluefin-api.yaml
Updated OpenAPI spec to include `AccountAggregatedTradeUpdate`.
+0/-48   
websocket-api.yaml
Updated WebSocket API spec for `AccountAggregatedTradeUpdate`.
+21/-105
Additional files
30 files
FILES +3/-6     
AccountAggregatedTradeUpdate.md +30/-0   
AccountDataStream.md +2/-0     
AccountEventType.md +2/-0     
AccountOrderUpdate.md +1/-1     
AccountPositionUpdate.md +1/-1     
AccountStreamMessagePayload.md +7/-13   
AccountTradeUpdate.md +1/-15   
ActiveOrderUpdate.md +1/-1     
MarketStreamMessagePayload.md +1/-1     
RecentTradesUpdates.md +1/-1     
__init__.py +1/-3     
account_data_stream.py +1/-0     
account_event_type.py +1/-0     
account_position_update.py +2/-2     
account_stream_message_payload.py +26/-12 
active_order_update.py +2/-2     
recent_trades_updates.py +11/-11 
trade.py +1/-11   
openapi_client_README.md +1/-3     
FILES +2/-6     
README.md +1/-3     
AccountAggregatedTradeUpdate.md +11/-0   
AccountDataStream.md +1/-0     
AccountEventType.md +1/-0     
AccountPositionUpdate.md +1/-1     
AccountStreamMessagePayload.md +1/-0     
AccountTradeUpdate.md +1/-15   
ActiveOrderUpdate.md +1/-1     
RecentTradesUpdates.md +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.
  • @nikita-seedlabs nikita-seedlabs requested a review from a team April 3, 2025 04:49
    @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: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Validation

    The validation logic for AccountStreamMessagePayload has multiple type checks that will always fail because they're checking if an object is multiple types simultaneously. This could cause runtime errors when processing WebSocket messages.

    # validate data type: AccountAggregatedTradeUpdate
    if not isinstance(v, AccountAggregatedTradeUpdate):
        error_messages.append(f"Error! Input type `{type(v)}` is not `AccountAggregatedTradeUpdate`")
    else:
        match += 1
    # validate data type: AccountOrderUpdate
    if not isinstance(v, AccountOrderUpdate):
        error_messages.append(f"Error! Input type `{type(v)}` is not `AccountOrderUpdate`")
    Documentation Gap

    The new AccountAggregatedTradeUpdate interface is added but its trade property lacks detailed documentation compared to the removed AccountTradeUpdate fields, which might make it harder for developers to understand the data structure.

    export interface AccountAggregatedTradeUpdate {
        /**
         * 
         * @type {Trade}
         * @memberof AccountAggregatedTradeUpdate
         */
        'trade': Trade;

    @qodo-code-review
    Copy link
    Copy Markdown

    qodo-code-review Bot commented Apr 3, 2025

    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
    Possible issue
    Restore original default implementation

    The default implementation was changed from AccountCommandFailureUpdate to
    AccountAggregatedTradeUpdate, which might cause unexpected behavior in existing
    code that relies on the previous default. Consider keeping the original default
    or ensuring all code that depends on this default is updated.

    rust/gen/bluefin_api/src/models/account_stream_message.rs [71-74]

     impl Default for AccountStreamMessage {
         fn default() -> Self {
    -        Self::AccountAggregatedTradeUpdate {
    +        Self::AccountCommandFailureUpdate {
                 reason: Default::default(),
                 payload: Default::default(),
             }
         }
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: This is an important catch. Changing the default implementation from AccountCommandFailureUpdate to AccountAggregatedTradeUpdate could break existing code that relies on the previous default behavior, potentially causing runtime errors or unexpected behavior.

    Medium
    Align type order with validation

    The order of types in the union type doesn't match the order used in the
    validation code elsewhere. For consistency and to avoid potential issues with
    type checking, align the order of types in this union with the order used in the
    validation logic.

    ts/sdk/src/api.ts [591]

    -export type AccountStreamMessagePayload = AccountAggregatedTradeUpdate | AccountCommandFailureUpdate | AccountOrderUpdate | AccountPositionUpdate | AccountTradeUpdate | AccountTransactionUpdate | AccountUpdate;
    +export type AccountStreamMessagePayload = AccountUpdate | AccountTradeUpdate | AccountAggregatedTradeUpdate | AccountOrderUpdate | AccountPositionUpdate | AccountTransactionUpdate | AccountCommandFailureUpdate;

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly identifies a potential consistency issue where the order of types in the union doesn't match the validation code elsewhere. While this won't cause functional bugs, aligning the order improves code maintainability and reduces confusion.

    Low
    • Update

    $ref: './websocket-api.yaml#/components/schemas/RecentTradesUpdates'
    RecentTradesUpdate:
    $ref: './websocket-api.yaml#/components/schemas/RecentTradesUpdate'
    OrderbookDiffDepthUpdate:
    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I'm confused, what happened to all of these?

    Copy link
    Copy Markdown
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Context: we use bluefin-api as our "main" file, which contains all the endpoint routes. Openapi generator will generate message types according to the requests and responses used by the endpoint paths in the bluefin-api.yaml file.

    BUT for web sockets, it's a special case, since the endpoint routes return multiple types for the same route, so I initially added all the response types into this file, so that openapi generator picks them up.

    HOWEVER, while working on it last night, I realized that I only need to add the top most level message types, and since we define all our response types in the top level messages through enums, openapi generator will pick all the needed types up. So we only really need to include the top level messages, which we only have 4-5: subscriptions, subscription response and data stream message main enum

    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Heck yes! Getting out of the way and letting the tool do work for us! 🎉

    description: "The filled quantity of the order in scientific notation with 9 decimal places."
    side:
    $ref: '#/components/schemas/Side'
    $ref: './common.yaml#/components/schemas/TradeSide'
    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Guess we'll merge TradeSide and PositionSide some other time

    type: object
    description: "Details about a trade in the account."
    properties:
    tradeId:
    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Nice to merge them all into one

    @nikita-seedlabs nikita-seedlabs merged commit 9e02a9b into main Apr 3, 2025
    @kevin-ip kevin-ip deleted the nikita/adjust_sdk_post_ws_changes branch April 3, 2025 19:37
    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.

    3 participants