Skip to content

Better deposit interface#119

Merged
MemoAlfa merged 3 commits intomainfrom
draft-better-deposit-interface
Jun 5, 2025
Merged

Better deposit interface#119
MemoAlfa merged 3 commits intomainfrom
draft-better-deposit-interface

Conversation

@MemoAlfa
Copy link
Copy Markdown
Contributor

@MemoAlfa MemoAlfa commented Jun 5, 2025

User description

Quick solution to better deposit interface


PR Type

Enhancement


Description

  • Enhanced Environment enum to include RPC URLs and properties

  • Refactored SDK initialization for dynamic contract and RPC setup

  • Added deposit_to_asset_bank method for asset deposits

  • Improved code structure and removed redundant parameters


Changes walkthrough 📝

Relevant files
Enhancement
bluefin_pro_sdk.py
Refactor environment handling and add deposit method         

python/sdk/src/bluefin_pro_sdk.py

  • Enhanced Environment enum to store both environment name and RPC URL,
    with accessor properties.
  • Refactored BluefinProSdk.__init__ to remove redundant parameters and
    dynamically configure contracts and RPC calls.
  • Added deposit_to_asset_bank async method for depositing assets to the
    external asset bank.
  • Improved contract configuration setup in init for dynamic asset and
    contract info.
  • Removed unused RpcUrl enum and streamlined environment handling.
  • +60/-27 

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @MemoAlfa MemoAlfa requested a review from a team June 5, 2025 12:38
    @qodo-code-review
    Copy link
    Copy Markdown

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit 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

    Error Handling

    The new deposit_to_asset_bank method lacks error handling for failed transactions or invalid inputs. Consider adding try/except blocks to handle potential RPC errors or validation for asset_symbol and amount_e9.

    async def deposit_to_asset_bank(self, asset_symbol: str, amount_e9: str, destination_address: str = None):
        """
        Deposits the provided asset of provided amount into the external asset bank.
        :param asset_symbol: The symbol of the asset being deposited (e.g. "USDC")
        :param amount_e9: The amount to be deposited in 9 decimal places (e.g. 1 USDC == 1000000)
        :param destination_address: Optional destination account address to which funds are being deposited.
                                    By default, funds are always deposited to the depositor's account
        """
        if destination_address is None:
            destination_address = self.current_account_address or self._sui_wallet.sui_address
    
        await self.__rpc_calls.deposit_to_asset_bank(asset_symbol, amount_e9, destination_address)
    Hardcoded Config

    The contracts_config setup in init() has a hardcoded comment "todo: dynamic supoorted assets once we support multi collat" and only handles the first asset from exchange_info.assets[0]. This may cause issues when multiple assets are supported.

    exchange_info = await self.exchange_data_api.get_exchange_info()
    # todo: dynamic supoorted assets once we support multi collat
    contracts_config = {
        "ExternalDataStore": exchange_info.contracts_config.eds_id,
        "InternalDataStore": exchange_info.contracts_config.ids_id,
        "Package": exchange_info.contracts_config.current_contract_address,
        "Operators": exchange_info.contracts_config.operators,
        "SupportedAssets": {
            "USDC": {
                "coinType": exchange_info.assets[0].asset_type,
                "decimals": exchange_info.assets[0].decimals,
                "symbol": exchange_info.assets[0].symbol
            }
        }
    }

    @qodo-code-review
    Copy link
    Copy Markdown

    qodo-code-review Bot commented Jun 5, 2025

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add empty assets check

    The code assumes exchange_info.assets list has at least one element without
    checking, which could cause an IndexError if the list is empty. Add a validation
    check to ensure the assets list is not empty before accessing its elements.

    python/sdk/src/bluefin_pro_sdk.py [142-163]

     async def init(self):
         await self.__login_and_update_token()
         self.__update_token_task = asyncio.create_task(self.__refresh_token())
         self.__is_connected = True
         # set contracts and rpc calls
         exchange_info = await self.exchange_data_api.get_exchange_info()
         # todo: dynamic supoorted assets once we support multi collat
    +    
    +    if not exchange_info.assets:
    +        raise ValueError("No assets found in exchange info")
    +        
         contracts_config = {
             "ExternalDataStore": exchange_info.contracts_config.eds_id,
             "InternalDataStore": exchange_info.contracts_config.ids_id,
             "Package": exchange_info.contracts_config.current_contract_address,
             "Operators": exchange_info.contracts_config.operators,
             "SupportedAssets": {
                 "USDC": {
                     "coinType": exchange_info.assets[0].asset_type,
                     "decimals": exchange_info.assets[0].decimals,
                     "symbol": exchange_info.assets[0].symbol
                 }
             }
         }
         # set RpcUrl enum from
         self.__rpc_calls = ProRpcCalls(self._sui_wallet, ProContracts(contracts_config), url=self.env.rpc_url)
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: This adds important error handling to prevent an IndexError when accessing exchange_info.assets[0] if the assets list is empty, improving code robustness.

    Medium
    Fix decimal precision documentation

    The docstring incorrectly states that 1 USDC equals 1000000 (6 decimal places),
    but the parameter is named amount_e9 suggesting 9 decimal places. This
    inconsistency could lead to incorrect deposit amounts. Update the docstring to
    reflect the correct decimal precision.

    python/sdk/src/bluefin_pro_sdk.py [165-176]

     async def deposit_to_asset_bank(self, asset_symbol: str, amount_e9: str, destination_address: str = None):
         """
         Deposits the provided asset of provided amount into the external asset bank.
         :param asset_symbol: The symbol of the asset being deposited (e.g. "USDC")
    -    :param amount_e9: The amount to be deposited in 9 decimal places (e.g. 1 USDC == 1000000)
    +    :param amount_e9: The amount to be deposited in 9 decimal places (e.g. 1 USDC == 1000000000)
         :param destination_address: Optional destination account address to which funds are being deposited.
                                     By default, funds are always deposited to the depositor's account
         """
         if destination_address is None:
             destination_address = self.current_account_address or self._sui_wallet.sui_address
     
         await self.__rpc_calls.deposit_to_asset_bank(asset_symbol, amount_e9, destination_address)
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The docstring contains an inconsistency between the parameter name amount_e9 (suggesting 9 decimal places) and the example showing 1000000 (only 6 zeros). This could mislead users about the expected input format.

    Low
    • Update


    # enum Environment with sui-prod sui-staging sui-dev
    # enum Environment with environment name and RPC URL
    class Environment(str, Enum):
    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.

    avoid double specification of environment, from a single enum we have both values

    self.__update_token_task = asyncio.create_task(self.__refresh_token())
    self.__is_connected = True
    self.__contracts_config = (await self.exchange_data_api.get_exchange_info()).contracts_config
    # set contracts and rpc calls
    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.

    on init we construct the RPC call class and settings dyncamically.

    @MemoAlfa MemoAlfa merged commit 6c65a69 into main Jun 5, 2025
    @MemoAlfa MemoAlfa deleted the draft-better-deposit-interface branch June 5, 2025 12:49
    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