leoschweizer / moneymoney-coinbase-pro

Inofficial MoneyMoney extension to list Coinbase Pro balances as securities
MIT License
6 stars 4 forks source link

Bad Request Error - USDC #7

Closed tommodore closed 5 years ago

tommodore commented 5 years ago

Got a Bad Request Error since adding USDC-Coin

Log says:

16:57:08 1> Refreshing account Coinbase Pro. 1> Web Banking Engine: Using user-supplied extension Coinbase Pro.lua version 1.00. 1> Protocol version: Web Scraping 1> Server address: https://api.pro.coinbase.com 1> Getting portfolio for Coinbase Pro... 1> Sending: GET https://api.pro.coinbase.com/accounts 1> HTTPS response: Bad Request 1> The server responded with the error message: Bad Request. Retrying... 1> Sending: GET https://api.pro.coinbase.com/accounts 1> HTTPS response: Bad Request 1> The API server of your bank responded with the error message: »Bad Request« Please try again later or contact the support of your bank with this error message.

TheSerapher commented 5 years ago

A new coin was added, this was causing the issues. Now that the coin is active, there is another issue:

          1> Sending: GET https://api.pro.coinbase.com/products/USDC-EUR/ticker
          1> The server responded with the error message: Not Found. Retrying...
          1> Sending: GET https://api.pro.coinbase.com/products/USDC-EUR/ticker

Apparently by default the plugin will try to fetch the EUR value of this coin. There will never be a EUR value since it's a simple exchange token for USD to USDC. It can be used to buy BAT (Basic Attention Token). There is no FIAT trade available for either BAT or USDC.

leoschweizer commented 5 years ago

There will never be a EUR value since it's a simple exchange token for USD to USDC.

Says who? Even Coinbase itself trades USDC for EUR. Until there is an equivalent for that in Coinbase Pro (and in the ticker API) I don't think there is much I can do...

As you can see, this works: https://api.coinbase.com/v2/prices/USDC-EUR/spot

TheSerapher commented 5 years ago

Probably true, in the meantime I have installed the BETA and disabled extension signature checks. For I am just skipping both USDC and BAT in the RefreshAccount function. Works again that way at least.

tommodore commented 5 years ago

Thank you for the workaround. Unfortunately I'm not able to install the BETA app because I bought it at the MacAppStore.

TheSerapher commented 5 years ago

@tommodore See last paragraph on this comment, worked for me too: https://github.com/leoschweizer/moneymoney-coinbase-pro/issues/4#issuecomment-411718269

leoschweizer commented 5 years ago

I'm also wondering why you two are experiencing this issue, while I am not? USDC and BAT do not even show up in my balance, and thus this still does not crash for me.

TheSerapher commented 5 years ago

I really can't tell. For me, the ticker request for USDC failed and trying it now it still doesn't serve any data:

https://api.pro.coinbase.com/products/USDC-EUR/ticker

Maybe it makes sense to be more specific on that request and check for anything non-200 or skip specifically any 404s? That would allow to fetch data but not any amount/value to it.

I don't really know what the best approach would be, especially since you don't even seem to be affected for some reason.

leoschweizer commented 5 years ago

Well in theory it could be built such that if some kind of error occurs, the respective position in your balance is not returned at all. But I doubt that this does the right thing for moneymoney. Imagine that there is some kind of general API outage (and this happens quite often), then the extension would not return any balances to moneymoney - your balance would thus show up empty. Not what we want.

We could also only not return any fiat value if that part fails. But then the position in your balance shows up as 0EUR, and thus messes up your history. Also not what we want.

We could also revert back to using the Coinbase (not Pro) API to do the fiat conversion - but you probably remember that we switched from there since we had the problem the other way around ;)

So, this is not really easy to fix.

qoomon commented 5 years ago

I think we should look at the response json if the response is {"message":"NotFound"} then we should ignore that fiat, cause it means it's not supported right now.

TheSerapher commented 5 years ago

@qoomon I gave that a try but I am not too familiar with LUA. Somehow the HTTP call is simply aborted and doesn’t return anything if the status code is not 200. Maybe it’s possible to enforce returning this but I haven’t found out how yet.

leoschweizer commented 5 years ago

@TheSerapher you can find the documentation of the Connection stuff here https://moneymoney-app.com/api/webbanking/

The request method does return a lot more (content, charset, mimeType, filename, headers) than is currently being evaluated (only content). I could imagine that an HTTP status code could be determined from the headers result.

TheSerapher commented 5 years ago

@leoschweizer That's what I tried out before simply skipping entries. It didn't work since it throws an exception and never gets to parse the headers of the request:

          1> Sending: GET https://api.pro.coinbase.com/products/USDC-EUR/ticker
          1> The server responded with the error message: Not Found. Retrying...
          1> Sending: GET https://api.pro.coinbase.com/products/USDC-EUR/ticker
21:15:49  1> HTTPS response: Not Found
          1> The server of your bank responded with an internal error. Please try again later.

I'd think this may be an issue with the implementation in MoneyMoney. The log also records an internal error, which isn't really true since the actual response is a 404:

$ curl -I https://api.pro.coinbase.com/products/USDC-EUR/ticker
HTTP/2 404

So unless we can get this patched and check for response codes other than 200 ourselves, I don't see a clean way to catch lacking API implementations :-/

TheSerapher commented 5 years ago

What a funny coincidence .. I was playing around with it trying to get the infos above, they just added ZCASH with code ZEC which is of course not working fully yet 😃 One more I had to add to my skip list.

qoomon commented 5 years ago

I think a have an idea, you can just ship your own http lib by doing the following

in conclusion you could ship arbitrary dependencies for example this module https://github.com/daurnimator/lua-http

TheSerapher commented 5 years ago

Found a way to check if a coin is even supported without going through the headers. See the linked PR.

It also adds a feature to the plugin, maybe you can check my pull request @leoschweizer :)

TheSerapher commented 5 years ago

Closed the old PR, made a new one specifically fixing the issue for the original code. Feel free to review, merge and hopefully close this issue here!

qoomon commented 5 years ago

I just got an answer from the moneymoney support how to do it the right way you have to set the Accept header to application/json then no exception is thrown, even if the api response code is 404. e.g.

local headers = {}
headers["Accept"] = "application/json"
local content = Connection():request("GET", url, nil, nil, headers)
return JSON(content):dictionary()
leoschweizer commented 5 years ago

The updated and signed extension is now available. Thanks @TheSerapher for the good work!

TheSerapher commented 5 years ago

No problem, my pleasure!