Skip to content

[BFP-1144 Deposit Call Decimal Updates#38

Merged
MemoAlfa merged 1 commit intomainfrom
myym/deposit-decimal-changes
Mar 22, 2025
Merged

[BFP-1144 Deposit Call Decimal Updates#38
MemoAlfa merged 1 commit intomainfrom
myym/deposit-decimal-changes

Conversation

@YameenMalik
Copy link
Copy Markdown
Contributor

@YameenMalik YameenMalik commented Mar 22, 2025

User description

Added code comments to make it clear that the input amount to deposit_to_asset_bank is in decimals of the coin being deposited.


PR Type

Documentation, Tests


Description

  • Clarified the unit of amount in deposit_to_asset_bank method.

  • Updated test comments to specify decimal places for USDC.

  • Removed unnecessary debug print statement in get_coin_having_balance.


Changes walkthrough 📝

Relevant files
Documentation
rpc.py
Clarified `amount` units in `deposit_to_asset_bank` docstring

python/sdk/src/crypto_helpers/rpc.py

  • Updated the docstring for deposit_to_asset_bank to specify that the
    amount is in the smallest units of the coin.
  • +1/-1     
    Miscellaneous
    utils.py
    Removed debug print statement in utility function               

    python/sdk/src/crypto_helpers/utils.py

    • Removed a debug print statement from get_coin_having_balance.
    +0/-1     
    Tests
    test_rpc_calls.py
    Updated test comments for USDC decimal places                       

    python/sdk/tests/test_rpc_calls.py

  • Updated test comments to specify that USDC uses 6 decimal places.
  • Added an example for 0.15 USDC in the test comment.
  • +3/-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.
  • @YameenMalik YameenMalik requested a review from a team March 22, 2025 15:42
    @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: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Documentation Inconsistency

    The docstring comment for the amount parameter states it's in "units of the coin" with example "1 USDC == 1000000", but this is actually describing decimals, not units. This could be confusing as it contradicts the PR description which correctly states the input is in decimals.

    amount (uint): The amount to be deposited in the units of the coin being deposited (for example, 1 USDC == 1000000)

    @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
    Fix misleading documentation

    The docstring is incorrect about the units. It should clarify that the amount is
    in the smallest units (e.g., 1 USDC is represented as 1000000 units), not "in
    the units of the coin".

    python/sdk/src/crypto_helpers/rpc.py [48]

    -amount (uint): The amount to be deposited in the units of the coin being deposited (for example, 1 USDC == 1000000)
    +amount (uint): The amount to be deposited in the smallest units of the coin (for example, 1 USDC == 1000000 units)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that the docstring wording is ambiguous. Changing "in the units of the coin" to "in the smallest units of the coin" provides clearer documentation about how amounts should be specified, which is important for correct API usage.

    Medium
    • More

    @YameenMalik YameenMalik changed the title Decimal Updates [BFP-1144 Deposit Call Decimal Updates Mar 22, 2025
    Copy link
    Copy Markdown
    Contributor

    @MemoAlfa MemoAlfa left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    @MemoAlfa MemoAlfa merged commit d8d7a2c into main Mar 22, 2025
    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