Multi wallet #1563
Multi wallet #1563
Conversation
js/start.js
Outdated
| return; | ||
| } | ||
|
|
||
| app.serverConfig.walletsHelp = 'CAUTION! - Rather than reading the wallet curs directly ' + |
There was a problem hiding this comment.
Is this helpful at all? If you were exploring app.serverConfig in the JS console, would you notice this warning?
There was a problem hiding this comment.
When would you see this in the console?
There was a problem hiding this comment.
if a developer is examining app.serverConfig in the console. Rather than them write code that directly references app.serverConfig.wallets, the preference is for them to get the curs from a utility function
| // that currency, then exchange rate data is not needed. | ||
| // TODO: | ||
| // TODO: | ||
| // TODO: ugh - how to handle that case? |
There was a problem hiding this comment.
This is a pretty interesting scenario, albeit quite the edge case, so we should probably take a lower scope path.
Anyhow, the idea is that if a listing is priced in a certain currency we don't have exchange rate data for then the Buy Now button is disabled. But, if the listing happens to be priced in one of the accepted currencies (and the accepted currency is supported by the user's wallet), then as long as the user pays in that currency, the purchase would work. So maybe in this case, we leave the button enabled:
And then in the Checkout, maybe we just disable all options other than the one in the pricing currency? Maybe with a little 'exchange rate unavailabletext, similar to howNot available` is used when your wallet doesn't support one of the accepted currencies?
(just want to stress to not put too much effort into this because most listings are probably priced in fiat AND the exchange rate API shouldn't be down too often (hopefully))
There was a problem hiding this comment.
I think it would be nice to allow like to like crypto purchases (ie if the listing is priced in a crypto coin, I can buy it with that crypto coin), since that would allow the network to keep working if the ticker was down, but not if it adds a lot of complexity.
Your idea seems right to me, we'd just disable all the currencies that won't work with a note next to each explaining why.
js/data/walletCurrencies.js
Outdated
| `https://blockchair.com/bitcoin/transaction/${txid}` | ||
| ), | ||
| canShapeShiftIntoWallet: true, | ||
| canShapeShiftIntoPurchase: false, |
There was a problem hiding this comment.
Just a reminder here that the shapeshift code can be removed since we don't use shapeshift any more.
| /** | ||
| * Returns a list of the crypto currencies supported by the wallet. | ||
| * | ||
| * @param {object} [options={}] - Function options |
There was a problem hiding this comment.
Are the brackets around the object needed? It looks like jsDoc uses them if the object is in an array, the options aren't part of an array passed to the supportedWalletCurs function here are they? http://usejsdoc.org/tags-param.html
There was a problem hiding this comment.
@jjeffryes The bracket notation on the variable name signifies that it's optional:
http://usejsdoc.org/tags-param.html#optional-parameters-and-default-values
The type is driven by the preceding fragment ({object} in this case).
| @@ -316,23 +321,23 @@ function fetchVerifiedMods() { | |||
| const fetchStartupDataDeferred = $.Deferred(); | |||
| let ownFollowingFetch; | |||
| let exchangeRatesFetch; | |||
| let walletBalanceFetch; | |||
| let walletBalancesFetch; | |||
There was a problem hiding this comment.
Good thought renaming this. We should probably take care across the app to make sure names still make sense with the multi wallet.
| <div class="flex snipKids gutterH tx5 detailsRow"> | ||
| <% if (ob.hasValidCurrency) { %> | ||
| <div class="flex gutterH tx5 detailsRow"> | ||
| <% if (ob.hasValidCurrency && false) { %> |
There was a problem hiding this comment.
Is the && false just test code?
| <div class="statsWrap flexColRows clrBr"> | ||
| <div class="js-walletStatsContainer"></div> | ||
| <div class="buttonWrap"> | ||
| <a class="btn clrP clrBr clrSh2 btnReceiveMoney js-toggleSendReceive"><%= ob.polyT('wallet.btnReceiveMoney') %></a> |
There was a problem hiding this comment.
You might want to use the top nav button classes I made for the donations modal here.
I assume this will replace the sendReceiveNav? Or is it the other way around?
| initialState: { | ||
| active: false, | ||
| balance: 0, | ||
| displayCur: app && app.settings && app.settings.get('localCurrency') || 'PLN', |
|
|
||
| render() { | ||
| const state = this.getState(); | ||
| this.$el.toggleClass('active', state.active); |
There was a problem hiding this comment.
Would it be better to handle the active state styling inside the coinNavItem template, so there isn't an extra DOM change here?
There was a problem hiding this comment.
Maybe, but that then gives you an extra wrapping element which, not only is kind of ugly, but also doesn't play nice with stuff like padMdKids borderStacked.
I'll keep this in mind though and maybe circle back to it when I make some other optimizations.
Get Balances for Wallet Nav Coins
Co-Authored-By: rmisio <rmisio@users.noreply.github.com>
removing polyTfallback and using native functionality
Fix Vertical Centering on Mod Message
Migrate pre mw
Ignore temp folder when building binaries
Update Debian dependency for libgconf2.4
Update package.json
This is the base branch for the multi-wallet client integration. It should be run with the
multiwalletserver branch.closes #1585
closes #1425