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

Modify RESTClient post() to add CB-2FA-Token for SIWC transaction endpoints #43

Closed
wants to merge 1 commit into from

Conversation

brianddk
Copy link

With the April 2nd expansion of the CDP api keys to support SIWC urls, there is extra support needed for the /v2/accounts/:account_id/transactions endpoints. This appears to be a holdover from the OAuth2 support, and may actually be an error in the API. If not, support is needed for the transactions endpoints. Specifically, posts this endpoint requires the CB-2FA-Token header. This PR adds a cb_2fa_token argument to the RESTClient constructor to facilitate that.

Example

from requests.exceptions import HTTPError
from coinbase.rest import RESTClient

params = {
  "type": "send",
  "currency": "BTC",
  "to": "bc1qwc2203uym96u0nmq04pcgqfs9ldqz9l3mz8fpj",
  "amount": "0.00045678",
}

api_key = "organizations/{org_id}/apiKeys/{key_id}"
api_secret = "-----BEGIN EC PRIVATE KEY-----\nYOUR PRIVATE KEY\n-----END EC PRIVATE KEY-----\n"
client = RESTClient(
    api_key = api_key,
    api_secret = api_secret,
)
try:
    client.post("/v2/accounts/456789ab-cdef-0123-4567-89abcdef0123/transactions", params)
except HTTPError as e:
    if "two_factor_required" in e.args[0]:
        cb_2fa_token=input("CB-2FA-Token: ").strip()
        client = RESTClient(
            api_key = api_key,
            api_secret = api_secret,
            cb_2fa_token = cb_2fa_token,
        )
        client.post("/v2/accounts/456789ab-cdef-0123-4567-89abcdef0123/transactions", params)
    else:
        raise e

Changes

  • Add CB-2FA-TOKEN header when cb_2fa_token is found in instance
  • Add CB_2FA_TOKEN_ENV_KEY constant
  • Add cb_2fa_token to APIBase constructor
  • Add cb_2fa_token to RESTClient constructor

…oints

  - Add `CB-2FA-TOKEN` header when `cb_2fa_token` is found in instance
  - Add `CB_2FA_TOKEN_ENV_KEY` constant
  - Add `cb_2fa_token` to APIBase constructor
  - Add `cb_2fa_token` to RESTClient constructor
@brianddk brianddk changed the title Modify RESTClient get() to add CB-2FA-Token for SIWC transaction endpoints Modify RESTClient post() to add CB-2FA-Token for SIWC transaction endpoints Apr 21, 2024
@urischwartz-cb
Copy link
Contributor

Thanks for reporting @brianddk ! I checked with our team and they said a fix was pushed for this. Could you check if you are able to hit this endpoint without the 2FA header now?

@brianddk
Copy link
Author

I checked with our team and they said a fix was pushed for this. Could you check if you are able to hit this endpoint without the 2FA header now?

still requesting the token:

{
  "errors": [
    {
      "id": "two_factor_required",
      "message": "Two-step verification code required to complete this request. Re-play the request with CB-2FA-Token header."
    }
  ]
}

@urischwartz-cb
Copy link
Contributor

urischwartz-cb commented Apr 24, 2024

@brianddk Looking at the SIWC docs, it seems that it is expected for 2FA to be required for this endpoint.
Closing out this PR since we do not plan on supporting SIWC in this SDK for now.

Have you been able to access this endpoint before without 2FA token, or has this always been the case?

@urischwartz-cb
Copy link
Contributor

@brianddk thank you again for flagging. This was indeed a bug in our service. A fix has now been pushed and you should not need the 2FA token header for this endpoint anymore.

@brianddk
Copy link
Author

A fix has now been pushed and you should not need the 2FA token header for this endpoint anymore.

Confirmed... Thanks.

You might want to let your contact in the API-dev team know that the update bugged the /v2/user/auth endpoint. It now shows "method": null in the response JSON when CDP keys are used on it.

@urischwartz-cb
Copy link
Contributor

Thanks for that. That is now fixed as well.

@brianddk
Copy link
Author

Thanks for that. That is now fixed as well.

Confirmed. Thx again.

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.

None yet

2 participants