Updated to official api #400

ckhatri · 2017-11-14T23:41:57Z

Just uploading this as mockup of a solution to issue #398

ckhatri · 2017-11-15T00:16:23Z

I can clean it up! But wanted to see if y'all thought if this was a good solution before doing so.

Basically this uses the official coinmarketapi, and then uses another api to get currency conversion rates from USD -> x currency.

fix · 2017-11-15T08:50:32Z

sounds good to me, yet again the northpole api is frozen...

ckhatri · 2017-11-15T19:42:34Z

@fix Great!

@j-a-m-l last commit was done to fix dashboard.html since I use the available supply in the ng-if. Used this cause market cap is dependent on the supply * price. Seem good?

spresnal · 2017-11-16T11:52:47Z

client/app/src/filters/filters.js

@@ -33,7 +33,7 @@
        var localeVersion

        if (currencyName === 'btc') {
-          localeVersion = 'Ƀ ' + val
+          localeVersion = 'Ƀ ' + Number(val).toFixed(5)


What's the purpose of doing toFixed(5) here?

Just wanted to cap the bitcoin value to some amount of digits. Chose five just because that was the value slightly above.

I'll be reverting this back to 8 digits though.

spresnal · 2017-11-16T11:57:17Z

client/app/src/services/network.service.js

-          if (res.data.price && res.data.price.btc) {
-            res.data.price.btc = Number(res.data.price.btc).toFixed(8) // store BTC price in satoshi
+          if (res.data[0] && res.data[0].price_btc) {
+            res.data[0].price_btc = Number(res.data[0].price_btc).toFixed(8) // store BTC price in satoshi


Nitpick: I always prefer descriptive method names over comments if possible -> ex: toSatoshis(res.data[0].price_btc)

Agreed, I just left that as is since it was already there. But in cleanup was planning on fixing some styling stuff up.

spresnal · 2017-11-16T12:04:55Z

client/app/src/services/network.service.js

-          peer.market = res.data
+          storageService.set('lastPrice', { market: res.data[0], date: new Date() }, true)
+          peer.market = res.data[0]
+          $http.get('https://api.fixer.io/latest?base=USD', { timeout: 2000}).then( function (result) {


Do we care about storing the value in every different one of the currencies below? Or do we just care about what the user has active?

If we just care about what the user has active, then maybe it'd be a good idea to consider passing in the convert param during the API call to coinmarketcap. That'd save us from doing the additional API call to api.fixer.io.

I wrote it this way to mimic the old api functionality, which gave us the value in every different currency. We simply store all the currency values and toggle the right one.

It is a bit wasteful on space since we don't need all the currencies, only the toggled active one. Only downside I could see to that is that we'd have to have a new api call each time the user toggles another currency (which probably will be rarely).

I'd be down to work on implementing that, just trying to get this change out asap since the old api is frozen and none of the wallets are showing the right price.

spresnal · 2017-11-16T12:11:19Z

client/app/src/components/dashboard/account-box.controller.js

@@ -34,7 +34,7 @@
    this.myAccountsCurrencyBalance = () => {
      const market = this.accountCtrl.connectedPeer.market
      const currencyName = this.accountCtrl.currency.name
-      const price = market ? market.price[currencyName] : 0
+      const price = market && market.price ? market.price[currencyName] : 0


If market or market.price is falsey does the user see 0 for their balance? If that's the case, I'd double check that we throw up a toast error message.

I just hit a similar issue to this on my PR. market is undefined until a connection to a peer is established. It resolves itself as soon as that connection happens (which already has toasts related to it) 🤔 I thought about showing zero initially too. Perhaps we should cache the market data so we at least show something?

I like the idea of caching the most recently retrieved value. That still leaves the case where the user doesn't have the cache built yet, but that'd be a big improvement over displaying 0 for returning users.

Yeh probably the best way to go!I think I'll do the same on my PR, default to zero. The caching / loading of cache should be done either here or another PR I think. As long as it's done before the next release of the desktop client, no one will notice too much! Perhaps if there's no market data, we show a hyphen (-) which could be included in the new way of doing it?

I guess the process would be along the lines of:

Check cache for market data (load if available)

Fetch market data from API

Update live and cache data

Does that sound okay?

If the get request for the coinmarket api is faulty, then the rejection function uses the cached value by setting market to the cached value. If there is no cached value, then we return zero. But peer.market will be defined eventually, even if it's a dummy value.

I've gone ahead and added a toast error message in that case. Saying we're unable to get market data.

I think the only time this would happen is if the api fails on the very first time the user opens the wallet since there will be no cached value.

We could add the hyphen! I like that idea.

I think if we could also cache the data within updatePeerWithCurrencies too, then load that as early as possible on client launch by default. The user may not then experience seeing zero at all, once that cache has been populated. You could override the cache assuming the coinmarketcap process is available.

I've commented possible caching changes in this PR.

I hope that makes sense?

alexbarnsley

A few comments but really only around the caching side of things

alexbarnsley · 2017-11-17T02:29:46Z

client/app/src/services/network.service.js

+            const USD_PRICE = Number(res.data[0].price_usd)
+            var currencies = ["aud", "brl", "cad", "chf", "cny", "eur", "gbp", "hkd", "idr", "inr", "jpy", "krw", "mxn", "rub"]
+            var prices = {}
+            currencies.forEach(function(currency) {


There's a shorter way of doing a function (not a massive difference or required, but just an alternative):

currencies.forEach(currency => { prices[currency] = result.data.rates[currency.toUpperCase()] * USD_PRICE })

alexbarnsley · 2017-11-17T02:36:49Z

client/app/src/components/dashboard/account-box.controller.js

@@ -34,7 +34,7 @@
    this.myAccountsCurrencyBalance = () => {
      const market = this.accountCtrl.connectedPeer.market
      const currencyName = this.accountCtrl.currency.name
-      const price = market ? market.price[currencyName] : 0
+      const price = market && market.price ? market.price[currencyName] : 0


I think if we could also cache the data within updatePeerWithCurrencies too, then load that as early as possible on client launch by default. The user may not then experience seeing zero at all, once that cache has been populated. You could override the cache assuming the coinmarketcap process is available.

I've commented possible caching changes in this PR.

I hope that makes sense?

alexbarnsley · 2017-11-17T02:39:37Z

client/app/src/services/network.service.js

+            })
+            prices["btc"] = res.data[0].price_btc
+            prices["usd"] = res.data[0].price_usd
+            peer.market.price = prices


Cache the response here:

storageService.set('peerCurrencies', prices, true)

Then roughly Line 22 of NetworkService could become:

var peer = { ip: network.peerseed, network: storageService.getContext(), isConnected: false, height: 0, lastConnection: null, price: storageService.get('peerCurrencies') || { btc: '0.0' } }

This will need testing. Also worth seeing what the other guys think of this approach

setting true in storageService.set('peerCurrencies', prices, true) force a full save of the storage into localstorage. If it happens too often the app can be unresponsive. Otherwise the storage is getting flushed to localstorage every 10s or so.

unless this is critical (like before shutting down the app) i would remove this flag.

@fix that's good to know! I think this is only called once per network connection but I'll check and make the necessary changes regardless as this isn't critical to force a flush for the prices. This has been merged already as a separate PR so I'll make a quick fix for it. Thanks!

alexbarnsley · 2017-11-17T14:50:18Z

I hope you don't mind. I have created a new branch & PR to do additional tweaks, along with a CMC ticker for individual networks (with fixes)

You did great work on this, just wanted to get it merged into master :)

+5! 👍

ckhatri · 2017-11-17T17:42:06Z

Totally understand! I'd want it done asap and we're in different time zones so it works pretty well!

alexbarnsley · 2017-11-17T18:41:37Z

Indeed! Thanks @ckhatri 😁

* fix: convert V1 `totalCount` of transactions to `Number` * test: the `totalCount` of fetched transactions

ckhatri added 3 commits November 14, 2017 17:39

usd and btc updated and working

d128b90

had to get rid of a console.log

f9ddd2a

added currency conversion

fb9ab69

had to update the navbar as well

fb485d0

ckhatri mentioned this pull request Nov 15, 2017

Price doesn't seem to update #398

Closed

ckhatri added 3 commits November 15, 2017 12:59

Merge branch 'master' into update_api

751ab5f

Merge branch 'master' into update_api

da3c4d3

fixing conflicts

0a5bc72

spresnal reviewed Nov 16, 2017

View reviewed changes

ckhatri added 5 commits November 16, 2017 12:19

have to cache the prices, so we cache at the end of the request

12c6cfb

add toast error message if no market data is available

f6da984

reverted so bitcoin is fixed to 8 digits (original)

ce01a6e

added convertToSatoshi method to clean up code

e355941

modularized currency conversion to another function

efceee2

alexbarnsley reviewed Nov 17, 2017

View reviewed changes

alexbarnsley closed this Nov 17, 2017

alexbarnsley mentioned this pull request Nov 17, 2017

Update API & CoinMarketCap ticker for Networks #408

Merged

ckhatri deleted the update_api branch November 17, 2017 17:42

alexbarnsley pushed a commit that referenced this pull request Dec 3, 2018

Fix the totalCount of the fetched transactions (#400)

b5e8e9a

* fix: convert V1 `totalCount` of transactions to `Number` * test: the `totalCount` of fetched transactions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated to official api #400

Updated to official api #400

ckhatri commented Nov 14, 2017

ckhatri commented Nov 15, 2017 •

edited

Loading

fix commented Nov 15, 2017

ckhatri commented Nov 15, 2017

spresnal Nov 16, 2017

ckhatri Nov 16, 2017 •

edited

Loading

spresnal Nov 16, 2017

ckhatri Nov 16, 2017 •

edited

Loading

spresnal Nov 16, 2017

ckhatri Nov 16, 2017 •

edited

Loading

spresnal Nov 16, 2017

alexbarnsley Nov 16, 2017 •

edited

Loading

spresnal Nov 16, 2017

alexbarnsley Nov 16, 2017 •

edited

Loading

ckhatri Nov 16, 2017 •

edited

Loading

alexbarnsley Nov 17, 2017 •

edited

Loading

alexbarnsley left a comment

alexbarnsley Nov 17, 2017

alexbarnsley Nov 17, 2017 •

edited

Loading

alexbarnsley Nov 17, 2017

fix Nov 17, 2017

alexbarnsley Nov 17, 2017

alexbarnsley commented Nov 17, 2017

ckhatri commented Nov 17, 2017

alexbarnsley commented Nov 17, 2017

Updated to official api #400

Updated to official api #400

Conversation

ckhatri commented Nov 14, 2017

ckhatri commented Nov 15, 2017 • edited Loading

fix commented Nov 15, 2017

ckhatri commented Nov 15, 2017

Choose a reason for hiding this comment

ckhatri Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckhatri Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckhatri Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbarnsley Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbarnsley Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

ckhatri Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

alexbarnsley Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

alexbarnsley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbarnsley Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbarnsley commented Nov 17, 2017

ckhatri commented Nov 17, 2017

alexbarnsley commented Nov 17, 2017

ckhatri commented Nov 15, 2017 •

edited

Loading

ckhatri Nov 16, 2017 •

edited

Loading

ckhatri Nov 16, 2017 •

edited

Loading

ckhatri Nov 16, 2017 •

edited

Loading

alexbarnsley Nov 16, 2017 •

edited

Loading

alexbarnsley Nov 16, 2017 •

edited

Loading

ckhatri Nov 16, 2017 •

edited

Loading

alexbarnsley Nov 17, 2017 •

edited

Loading

alexbarnsley Nov 17, 2017 •

edited

Loading