[bisq-network/bisq] PriceNode: Add support for multiple ExchangeRateProviders (#4315)

[cd2357]

@cbeams Regarding what happens in case of failure (price difference > 1%) : this is covered in Test 4, where Alice and Bob use different pricenodes (one of them having an artificial 5% price difference). It behaves as in the status quo, which is, it fails gracefully (shows an error popup which describes why the trade cannot happen, please try again later; once popup is closed, Bisq can be used normally).

Regarding roll-out and an eventual roll-back in case of issues: will coordinate with @wiz, but basically this PR doesn't have any special config files or settings, from what I can tell it can be reverted with no issue.

Regarding testing and trying to break it: most of the tests I wrote are for "live" testing between two instances, Alice and Bob (on regtest). Also tried to simulate all edge cases and failure scenarios on the pricenode front, and between the pricenode and the exchange API endpoints. Let me know if there are other scenarios we should test, or other ways this can break.

@wiz to answer your four points:

Re: point 1 (losing support for hundreds of currencies) : not an issue anymore (see Tables 1 and 2 in the PR descriptioncurrencies).

To summarize, here's an overview of currencies supported by the BA pricenodes but not by this PR:

**Table X1: Currencies covered by BA, but not by this PR (as of 02.08.2020)**

```
CLF Chilean Unidad de Fomento
CNH Chinese Yuan (Offshore)
CUC Cuban Convertible Peso
ERN Eritrean Nakfa
GGP Guernsey pound
IMP Isle of Man pound
JEP Jersey pound
MRO Mauritanian ouguiya 
SSP South Sudanese pound
STD São Tomé and Príncipe dobra
XAG Silver
XAU Gold
XDR Special Drawing Rights (IMF)
XPD Palladium
XPT Platinum
```

Also, in the other direction:

**Table X2: Currencies covered by this PR, but not by BA (as of 02.08.2020)**

```
AEON
BEAM
BTM
DAI
EMC
FAIR
GRIN
MRU Mauritanian Ouguiya
NAV
PART
PIVX
STN Sao Tomean Dobra
USDC
XRC
XZC
ZEN
```

Re: your point 2 (compatibility with current Bisq nodes, handle evtl missing json elements in pricenode feed) : yes good point, there are some hardcoded pricenode elements that Bisq nodes expect. The PR handles that and it's and it's covered by Tests 1-4.

Re: your point 3 (testing between traders usign BA-pricendes and new pricenodes) : Not explicitly listed, but it's basically a variant of Tests 1-4. So Alice or Bob can be started with `--providers="<existing-live-pricenode-address>"` and this can be covered as well.

Re: your point 4 (roll-out strategy) : yes, ideally all pricenodes would upgrade as close to each-other in time as possible.

If there is **prolonged overlap** between BA pricenodes and new pricenodes (e.g. a long roll-out time-frame where both pricenode versions would coexist), these are the risks I see:

* Alice would try start a trade with a currency unknown to Bob's node
  * these can either be:
    * currencies supported by BA, but not in this PR (Table X1 above), or
    * currencies supported by this PR, but not in BA (Table X2 above)
   * in both cases, none of these currencies has an active markets, so very low risk
   * even if it happens, trade will graciously fail (error popup + Bisq node continues to function)

So overall the risks are low, and only there in case of a prolonged roll-out window, and even if they happen, Bisq graciously handles them. So these risks seem very low.

@sqrrm regarding your point on usability : From what I can see, there is no usability impact. This is transparent to the user, the change from BA to this PR wouldn't even be noticed. Only somewhat risky period is during the roll-out, where some currencies might have price differences >1% between BA and this PR.

Here are those currencies, as of right now:

```
ILS (2% delta between this PR's price and BA price)
NGN (9%)
PLN (6%)
RUB (1.3%)
SEK (3.8%)
UAH (1.3%)
ZAR (1.8%)
```

Also these are inactive markets, so the risk is low. And if such a trade is attempted during roll-out, Bisq would graciously handle the error and deny creating the trade. When roll-out is over, the risk is over.

Re: your point on loss of currencies (esp. latin american ones) : see above answer to wiz, shouldn't be a problem anymore.

------

So, this being said:

The PR is open for reviews / tests / and feedback.

Also, do let me know if you see other risks or other areas that should be tested.

P.S. I re-ran all tests above (0-5), all green.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/4315#issuecomment-667671916
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200802/2d7041ee/attachment.html>