umbrel-dashboard icon indicating copy to clipboard operation
umbrel-dashboard copied to clipboard

Published 20 hours ago • verified publisher icon getumbrel

Balance USD amount sometimes display ~ $NaN

Open rubenhbaca opened this issue 4 years ago • 14 comments

image

rubenhbaca avatar Dec 19 '20 07:12 rubenhbaca

I will fix this later, I am just documenting my findings here for later reference.

rubenhbaca avatar Dec 19 '20 07:12 rubenhbaca

That is a known issue : the price is fetched via Tor. We talked about this in the Keybase dev chat with @lukechilds. Sometimes the request is refused by the API, and the price is not fetched until Tor changes its route (every ~10 min).

A workaround could be to save the last fetched price and return it in case of a request error.

louneskmt avatar Dec 19 '20 10:12 louneskmt

Yes, that would be a great workaround. Where do you suggest this hardening code should be, dashboard (client-side) or where we make the call to the API (server-side)? It can easily be done on the client side. Just not sure on your guyses philosophy on server vs client-side code (but ill get there).

I can always not display the amount it if it's not a number as another workaround?

rubenhbaca avatar Dec 19 '20 14:12 rubenhbaca

Here is the scheme to get the price:

Dashboard --API--> Manager --API via Tor--> Cryptocompare

I think the best way to do it could be to have the manager save the price when it is successfully retrieved. So when the request is unsuccessful (for example because of the tor current identity), the manager returns the saved price to the dashboard instead of nothing.

louneskmt avatar Dec 19 '20 14:12 louneskmt

Yea that sounds great.

rubenhbaca avatar Dec 19 '20 14:12 rubenhbaca

Here is the route: https://github.com/getumbrel/umbrel-manager/blob/master/routes/v1/external.js

louneskmt avatar Dec 19 '20 15:12 louneskmt

Yes that's exactly the place to put the fix. I'll make a PR later tonight.

rubenhbaca avatar Dec 19 '20 15:12 rubenhbaca

@louneskmt I have created a PR as promised. https://github.com/getumbrel/umbrel-manager/pull/62

rubenhbaca avatar Dec 20 '20 03:12 rubenhbaca

Any plans merging this?

bezysoftware avatar Mar 31 '21 08:03 bezysoftware

I hope so. poke @mayankchhabra @louneskmt

rubenhbaca avatar Mar 31 '21 14:03 rubenhbaca

Sorry for the delay, missed this, reviewed your PR @rubenhbaca!

lukechilds avatar Mar 31 '21 14:03 lukechilds

@lukechilds thank you, I will review and make those changes tonight.

rubenhbaca avatar Mar 31 '21 15:03 rubenhbaca

@rubenhbaca Any plans to finish the PR for this bug? If not, I would be happy to work on this issue. Thanks!

TannerR1776 avatar Jun 28 '21 16:06 TannerR1776

@TannerR1776 i will review this PR tonight and see if there is any further changes and pump luke for a merge.

rubenhbaca avatar Jun 28 '21 16:06 rubenhbaca