Feature/ars pricenode redundancy

[rodvar]

Brief:

After merging pull request #15 we have a real market pricenode exhange rate provider of ARS/BTC, but its only one which is a liability for the system. This PR is meant to give redundancy to the ARS/BTC pricenode.

** Approach **

Use BlueLytics API to identify what's the gap multiplier ARS/USD and apply it to ARS/BTC since they are strictly correlated.
An example is provided from previous work but the approach is gonna be moved to the Service level to avoid tapping on individual exhange providers codes.

Related Issues:

This PR requires #14.

bisq-network/bisq#6601

Testing

Passing tests provided in this PR

Hope you enjoy reviewing this code, thanks!

[rodvar]

@jmacxx Copying your last comment here to continue the conversation:

Making a secondary unrelated HTTP request within coingecko provider is unclean and could break the coingecko feed at runtime. It could be done more cleanly by making the bluelytics feed an ExchangeRateProvider and doing the special rate conversion at the ExchangeRateService level.

From your experience what would be the best way of doing this?
Are you thinking on tapping into ExchangeRateService#getAllMarketPrices code doing something like this?

            Set<ExchangeRate> exchangeRates = new HashSet<>();
            Double bluelyticsGapMultipler = this.getBluelyticsGapMultiplier(); // if there are problems updating this value, 1.0 is return 

            // each provider would return false by default, except CryptoYa which already considers the special ARS case
            if (p.considersSpecialCases()) {
                exchangeRates.addAll(p.get());
            } else {
                // 
                p.get().forEach(er -> {
                    if (er.getCurrency().equals("ARS")) { // TODO read special currency cases from env and apply and apply change accordingly
                        exchangeRates.add(new ExchangeRate(
                                er.getCurrency(),
                                er.getPrice() * blueLyticsGapMultipler,
                                er.getTimestamp(),
                                er.getProvider()
                        ));
                    } else {
                        exchangeRates.add(er);
                    }
                });
            }

[ghost]

Yes, in general, something like that.

• It would keep the service less cluttered by delegating the special case if possible e.g. p.maybeApplyMultiplierForArs(bluelyticsGapMultiplier).
• I would expect that if there is a problem obtaining bluelyticsGapMultiplier, (i.e. the API becomes unavailable) that the providers which depend on it would revert to not providing a BTC/ARS price. (ref: if there are problems updating this value, 1.0 is return, perhaps NaN can mean there was an error).

[rodvar]

Yes, in general, something like that.

• It would keep the service less cluttered by delegating the special case if possible e.g. p.maybeApplyMultiplierForArs(bluelyticsGapMultiplier).
• I would expect that if there is a problem obtaining bluelyticsGapMultiplier, (i.e. the API becomes unavailable) that the providers which depend on it would revert to not providing a BTC/ARS price. (ref: if there are problems updating this value, 1.0 is return, perhaps NaN can mean there was an error).

Awesome, to your point:

1. I thought about delegating in the first place but from the exchange provider how can I modify the cached results? I think what we can do is pass the multiplier and make the exchange create a new Rate for ARS if it has one and then the service decides what to use
2. Yep thanks for the comment, 100% if there is a problem no ARS official rate should be shared
3. one last comment, implementing this PR would mean we can and need to remove ARS from the banned fiat list in the properties so that the exchanges fetch the "official" rate for ARS/BTC and then we can apply the multiplier

[rodvar]

@jmacxx think this one is ready to start reviewing and start working our way towards merging. Please have a look when you have some time, thanks! 🙏

[ghost]

@rodvar I've started testing:

• the initial request to get the bluelytics multiplier is made within the pricenode client's API call, and therefore blocks it. As mentioned earlier, I think that kind of blocking is wrong. The bluelytics request should be periodic and independent of what the client is doing.
• the subsequent async calls to update the bluelytics multiplier value are throwing repeated exceptions, log: pricenode_blue_error_log.txt

[rodvar]

@rodvar I've started testing:

• the initial request to get the bluelytics multiplier is made within the pricenode client's API call, and therefore blocks it. As mentioned earlier, I think that kind of blocking is wrong. The bluelytics request should be periodic and independent of what the client is doing.
• the subsequent async calls to update the bluelytics multiplier value are throwing repeated exceptions, log: pricenode_blue_error_log.txt

thanks for taking some time to test this @jmacxx ! Very helpful comments:

• I agree with this, we shouldn't block such a crucial API. I already put some code in place code to do stuff async after the initial call but can easily make all of it async which I'll do in my next change.
• on a first glance at the logs it looks like the exceptions could be linked to a jdk11 bug: https://stackoverflow.com/questions/52574050/javax-net-ssl-sslexception-no-psk-available-unable-to-resume - I'll dig into this soon

may I ask exactly which JDK are you using please? gonna try to repro this here

[ghost]

openjdk 11.0.2 2019-01-15
OpenJDK Runtime Environment 18.9 (build 11.0.2+9)
OpenJDK 64-Bit Server VM 18.9 (build 11.0.2+9, mixed mode)

[rodvar]

@jmacxx can you help me test this again, please? Do you still face the same exceptions?
are you able to try with a different JDK?
Thanks!

[ghost]

Testing: working OK now, I have not changed JDK. I checked out your branch directly instead of applying the patch.

• Please rename BlueLyticsUsdRates to BlueLyticsDto
• Please rename BlueLyticsUsdRate to BlueLyticsService
• Please remove needless this. references.
• Please include Bisq standard file header comment.

I'm curious as to why you choose to launch a BlueLytics reference data update triggered by the Bisq client querying the API, instead of periodically based on a timer. All the other reference data are periodic.

I'll probably have some more comments later, back to testing for now!

[ghost]

@rodvar I think your solution could be reconfigured to follow the Controller, Service, and Provider roles that are used in spot and mining portions of the pricenode. Then there can be multiple blue gap providers for currencies: ARS, LBP, etc. Another advantage of following those guidelines is that it provides infrastructure for component creation and periodic updates.

ExchangeRateController is the overall manager, containing an instance of each Service. It can pass the blue gaps to the ExchangeRateService before calling getAllMarketPrices.

[rodvar]

@jmacxx thanks for all the comments and greatly detailed suggestions, I'll get my hands on this ASAP.

[rodvar]

Hi @jmacxx ! I've done the requested changes (#17 (review)) now. I haven't had time to go through the last suggestion of changing the approach. On a surface level, it looks like the right thing to do though.
Do you think it would be useful to merge this as is and work on the refactor on another PR? Not sure when is the next release for bisq scheduled and might be good if we can get this in along with the currently unique ARS/BTC price scheduled to be released.
Let me know your thoughts, cheers!

[ghost]

Hi @rodvar I've requested another maintainer to review this PR. Regarding releases, pricenodes get upgraded when necessary, they are not tied to the Bisq client release schedule. So, no rush, and best to get the code right the first time.

[alvasw]

Hi @rodvar
I'll review your PR soon.

[rodvar]

Hi @jmacxx, thanks for all the clarifications! That makes sense why a friend of mine was telling me that the ARS/BTC price is already fixed in the current Bisq 1.9.12... 😄

Hi @alvasw, looking forward to your review!

[alvasw]

Hi @rodvar!

Thank you for taking your time to implement solution for all ARS users. Because of you, Bisq will be usable there! 😄

It was easier for me to play around with your code to find a good solution. Hence, I forked your PR branch and to make some changes. Unfortunately, the changes are hard to put into individual code review comments.

I wanted to set you as the commit author before pushing the changes. Sadly, Git won't let me set you as an author because of the GPG signature.

Please set yourself as the author and push the commit to your PR branch! 😄
https://github.com/alvasw/bisq-pricenode/tree/implement_ars_exchange_rate_transformer

Review

1. I think you updated the bisq submodule by accident.

2. You made cosmetic changes in the CoinGecko, CoinGeckoTest, and AbstractExchangeRateProviderTest classes. It's better to commit these changes separately: "Refactor CoinGecko's ExchangeRateProvider", "Cleanup AbstractExchangeRateProviderTest"

3. The majority of the price providers don't and won't support the blue rate. At the moment, only the CryptoYa provider supports it. Therefore, we should abstract away the blue rate concept and create a generic ExchangeRateTransformer that can be used for multiple use-cases in the future.

   Afterward, we can hide the ARS blue rate transformation in the ArsBlueRateTransformer. This transformer only supports the Argentine Peso and exludes ExchangeRates from BlueRateProviders (CryptoYa).

4. You created your threading logic for the API calls to BlueLytics and handled all possible edge cases. The Timer class from the JDK does most of that already. Moreover, we can reuse PriceProvider<T> which already implements caching and threading for us.

[alvasw]

I think you updated the submodule by accident.

[rodvar]

that's correct!

[rodvar]

Hi @rodvar!

Thank you for taking your time to implement solution for all ARS users. Because of you, Bisq will be usable there! 😄

It was easier for me to play around with your code to find a good solution. Hence, I forked your PR branch and to make some changes. Unfortunately, the changes are hard to put into individual code review comments.

I wanted to set you as the commit author before pushing the changes. Sadly, Git won't let me set you as an author because of the GPG signature.

Please set yourself as the author and push the commit to your PR branch! 😄 https://github.com/alvasw/bisq-pricenode/tree/implement_ars_exchange_rate_transformer

Review

1. I think you updated the bisq submodule by accident.
2. You made cosmetic changes in the CoinGecko, CoinGeckoTest, and AbstractExchangeRateProviderTest classes. It's better to commit these changes separately: "Refactor CoinGecko's ExchangeRateProvider", "Cleanup AbstractExchangeRateProviderTest"
3. The majority of the price providers don't and won't support the blue rate. At the moment, only the CryptoYa provider supports it. Therefore, we should abstract away the blue rate concept and create a generic ExchangeRateTransformer that can be used for multiple use-cases in the future.
   Afterward, we can hide the ARS blue rate transformation in the ArsBlueRateTransformer. This transformer only supports the Argentine Peso and exludes ExchangeRates from BlueRateProviders (CryptoYa).
4. You created your threading logic for the API calls to BlueLytics and handled all possible edge cases. The Timer class from the JDK does most of that already. Moreover, we can reuse PriceProvider<T> which already implements caching and threading for us.

Hi @alvasw , thanks so much for this effort. I don't really care who the author is as long as we achieve this goal 💪 , but thank you! I'll review your fork changes and bring them in! 😄

I'll get my hands on this soon!!!

[rodvar]

@alvasw I've integrated your changes in now.

Do you know why this is happening?

ExchangeRateServiceTest > getAllMarketPrices_withSingleExchangeRate() FAILED
org.opentest4j.AssertionFailedError at ExchangeRateServiceTest.java:267

26 tests completed, 1 failed

I'll dig into this ASAP and wrap up this PR, thanks!

[ghost]

@rodvar please compare your branch to Alva's. Looks like a lot of dead code in yours, for example BlueLyticsService is no longer needed etc.

While testing (both yours and Alva's branches) I noticed that the price feed is only providing ARS quotes because its only including transformed rates in getCurrencyCodeToExchangeRates(). I fixed it locally by this, but there's probably a cleaner way of doing it:

if (transformedExchangeRates.size() == 0) { // no transformation was done, use actual rate
   transformedExchangeRates.add(exchangeRate);
}

[rodvar]

@jmacxx I know I still have to clean up, but I did run it individually and the test was not passing (before my changes)
I can figure it out anyways just wondering if he is aware and might know why to speed me up :)

[rodvar]

@jmacxx @alvasw OK guys, finally had some time to finish this up. Found the issue with the test and fixed it - I think this is ready to go please have a look :D

[alvasw]

utACK

[gabernard]

utACK

[ghost]

I still have not finished testing this PR.

One question relates to the rate calculated from Bluelytics. It currently shows as 18052497. CryptoYa gives 19940160. Seems like a huge difference, has it been looked into?

Sep-12 21:31:39.482 [Timer-5] INFO  b.p.s.p.CoinGecko: BTC/ARS: 9075707.709
Sep-12 21:31:39.487 [Timer-13] INFO  b.p.s.p.CryptoYa: BTC/ARS: 1.9940160780500002E7 
Sep-12 21:32:12.965 [http-nio-8080-exec-5] INFO  b.p.s.ExchangeRateService: ARS transformed from 9075707.709 to 1.805249762280654E7 
Sep-12 21:32:12.965 [http-nio-8080-exec-5] INFO  b.p.s.ExchangeRateService: ARS transformed from 1.9940160780500002E7 to 1.9940160780500002E7 

Looking at the raw feed from https://criptoya.com/api/btc/ars/0.1 it shows most lineitems with around 19000000. But there is one that is very different, (ripioexchange) 34652283, so that inflates cryptoYa. Even if CryptoYa was correct, Bluelytics is way lower.

[rodvar]

I still have not finished testing this PR.

One question relates to the rate calculated from Bluelytics. It currently shows as 18052497. CryptoYa gives 19940160. Seems like a huge difference, has it been looked into?

Sep-12 21:31:39.482 [Timer-5] INFO  b.p.s.p.CoinGecko: BTC/ARS: 9075707.709
Sep-12 21:31:39.487 [Timer-13] INFO  b.p.s.p.CryptoYa: BTC/ARS: 1.9940160780500002E7 
Sep-12 21:32:12.965 [http-nio-8080-exec-5] INFO  b.p.s.ExchangeRateService: ARS transformed from 9075707.709 to 1.805249762280654E7 
Sep-12 21:32:12.965 [http-nio-8080-exec-5] INFO  b.p.s.ExchangeRateService: ARS transformed from 1.9940160780500002E7 to 1.9940160780500002E7 

Looking at the raw feed from https://criptoya.com/api/btc/ars/0.1 it shows most lineitems with around 19000000. But there is one that is very different, (ripioexchange) 34652283, so that inflates cryptoYa. Even if CryptoYa was correct, Bluelytics is way lower.

Yes, @jmacxx I've seen this and been thinking about it, as you correctly pointed out: cryptoya values are a bit higher (sometimes way too high like ripio). Hence it is why in Argentina among the 50+ rates for the dollar one of them is called the "dollar crypto" which is an even higher value than the free/blue price.

Crypto ya is sourcing from real CeFi exchanges that trade bitcoin in Argentina. I'm not an economist/financial person but I understand there is a fair bit of risk in these trades and some exchanges are more capable of handling those risks than others, that's why the big price is different. Our current algorithm is doing an average of all the exchange's sell price.

For the redundancy, we use the blue dollar factor to estimate a real market value for the BTC.

A fair question would be - Are we "arrogantly" adding these redundancy prices that in reality wouldn't be free traded in Argentina and therefore introducing some noise?

Tapping on your knowledge of the platform here, what would be the final market price used for market comparison in Bisq from all these new price points we are adding? One option to do a better estimation would be:

• When we calculate the average for crypto ya we could calculate what's the average gap factor from all the CeFi exchanges and save it somewhere.
• Then replace the Bluelytics gap factor with this one and that should give better redundancy estimations.

Happy to work on this, I want to hear your comments first.
Cheers!

[ghost]

We have 19 blue rates excluding ripio coming from cryptoYa and Binance, Average=19516121
The bluelytics/CoinGecko rate is at the lower bound, 18378773

Bisq Average=19459253. So the Bisq index is pulled down only 0.3% by the CoinGecko/bluelytics rate.
If CryptoYa was to go offline, we would be left with just 2 feeds and an average rate of 18870860.

[rodvar]

We have 19 blue rates excluding ripio coming from cryptoYa and Binance, Average=19516121 The bluelytics/CoinGecko rate is at the lower bound, 18378773

Bisq Average=19459253. So the Bisq index is pulled down only 0.3% by the CoinGecko/bluelytics rate. If CryptoYa was to go offline, we would be left with just 2 feeds and an average rate of 18870860.

@jmacxx Yeah not cool... how about we modify the averaging algorithm for Cryptoya to calculate the mean and the std deviation and exclude values out of the range [mean - 2sd, mean + 2sd ]?

That would definitely kick out that huge Ripio value!
And then we return the average of that final list.

I'm happy to work on this on another PR, just wanna know your thoughts first
Also maybe mean and sd are used somehwere else in bisq, please point if you know of any reusable code :)

[ghost]

Yes indeed, @alvasw has done that in #30 which removes the outlier rate. I was considering that and #29 & #31 in the calculations above. So I think we're good.

[rodvar]

awesome guys! @alvasw @jmacxx