Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: transfer amount less than or equal to the balance. #36

Merged
merged 3 commits into from
Oct 29, 2024

Conversation

riflecode
Copy link
Contributor

What changed? Why?

The user should be able to transfer the entire balance from their account.

image

Qualified Impact

@cb-heimdall
Copy link

cb-heimdall commented Oct 29, 2024

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@John-peterson-coinbase
Copy link
Contributor

Thank you for your contribution @riflecode . Can you please add a test to tests/test_wallet_address.py for ensure balance on the full amount? Thanks!

Something like:

@patch("cdp.Cdp.api_clients")
def test_ensure_sufficient_balance_sufficient_full_amount(
    mock_api_clients, wallet_address_factory, balance_model_factory
):
    """Test the ensure_sufficient_balance method with sufficient full amount balance."""
    wallet_address = wallet_address_factory()
    balance_model = balance_model_factory(
        amount="1500000000000000000", network_id="base-sepolia", asset_id="eth", decimals=18
    )

    mock_get_balance = Mock()
    mock_get_balance.return_value = balance_model
    mock_api_clients.external_addresses.get_external_address_balance = mock_get_balance

    wallet_address._ensure_sufficient_balance(Decimal("1.5"), "eth")

    mock_get_balance.assert_called_once_with(
        network_id=wallet_address.network_id, address_id=wallet_address.address_id, asset_id="eth"
    )

@riflecode
Copy link
Contributor Author

Thank you for your contribution @riflecode . Can you please add a test to tests/test_wallet_address.py for ensure balance on the full amount? Thanks!

Something like:

@patch("cdp.Cdp.api_clients")
def test_ensure_sufficient_balance_sufficient_full_amount(
    mock_api_clients, wallet_address_factory, balance_model_factory
):
    """Test the ensure_sufficient_balance method with sufficient full amount balance."""
    wallet_address = wallet_address_factory()
    balance_model = balance_model_factory(
        amount="1500000000000000000", network_id="base-sepolia", asset_id="eth", decimals=18
    )

    mock_get_balance = Mock()
    mock_get_balance.return_value = balance_model
    mock_api_clients.external_addresses.get_external_address_balance = mock_get_balance

    wallet_address._ensure_sufficient_balance(Decimal("1.5"), "eth")

    mock_get_balance.assert_called_once_with(
        network_id=wallet_address.network_id, address_id=wallet_address.address_id, asset_id="eth"
    )

Thank you. I will improve this test case

@John-peterson-coinbase
Copy link
Contributor

John-peterson-coinbase commented Oct 29, 2024

@riflecode - The CodeQL CI check did not get triggered since this is your first contribution to the repo. Can you push up a fix to #36 (comment) formatting issue and that should trigger the CodeQL CI run now that you have access to the CI actions so that we can merge.

Thanks!

@jazz-cb jazz-cb closed this Oct 29, 2024
@jazz-cb jazz-cb reopened this Oct 29, 2024
@John-peterson-coinbase John-peterson-coinbase merged commit 0b10f03 into coinbase:master Oct 29, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants