search

bisq-network / bisq-pricenode

GNU General Public License v3.0
6 stars 12 forks source link

Feature/ars pricenode #15

Closed rodvar closed 1 year ago

rodvar commented 1 year ago

Hi there! This is my first PR ever in the project, please be kind to review it and I'll be kind and happy to review your comments/feedback :)

Brief:

Countries with currency controls declare exchange rates that cannot be trusted and therefore and extra effort is needed to source a price that is actually traded in the real/free market. This PR is the first change towards solving the issue for the currency ARS (Argentina Peso) -> it provides a price node exchange rate for ARS/BTC.

There might be more PRs coming to use this price node and/or solve the problems for other countries like Lebanon.

Approach

CryptoYa public API was used as suggested in the related issue. It is possible to get rates from different exchanges that already work with the blue rate in Argentina. An Average algorithm was implemented taking the asking price from each of them that have been updated at least a day ago. The API seems quite reliable, but if any data issues were to happen defaults have been provided (any price, then 0.0 meaning "do not include / no data").

Related Issues:

Testing

Other important notes

When I started this the project was not compiling in main so this PR includes some code that quick-fixes this. I'm more than happy to review feedback on this and strip those changes out into a different PR if needed.

Hope you enjoy reviewing this code, thanks!

ghost commented 1 year ago

I'm trying it out, and its working so far! :+1:

image

image

rodvar commented 1 year ago

I'm trying it out, and its working so far! 👍

image

image

  • Its a concern that it depends on a single API which could fail or block our requests. Unlike all other markets which request from multiple APIs for redundancy.
  • The 7 commits should be squashed into one especially since one reverts changes made by another. The only way I got this PR to apply on my system was to delete the patches that referenced bisq, gitmodules and gitignore.
  • The PR should be based on Fix tests. QuoineTest removed. #14
  • The change to ExchangeRateServiceTest looks like a cheap workaround to something that should perhaps be investigated.

Awesome, thanks @jmacxx I agree we should have at least another endpoint to compare but I struggle to find an open API that reports blue market prices. Crypto ya is consulting other APIs from different exchanges that operate in Argentina, eventually we could replace Crypto ya with our own individual API request to each marketplace. I think this is a good first contribution to solve the problem and we can grow from there. I'm gonna be sending more PRs related to the same topic soon.

What I am concerned though is your comment which could fail or block our requests. - isn't that handled in the client code that execute my exchange rate object code? Please let me know if I need to implement any extras to make sure that if cryptoya.com is down we don't block any othe functionality. I'll also make some tests on my end to make sure this won't happen.

I've got a question regarding the squashings, don't you guys squash before merge ? Anyways for this one I'm gonna squash after rebasing and adding some improvements you guys recommended. Thanks!!!

rodvar commented 1 year ago

@jmacxx @HenrikJannsen I think I covered everything, have a look and let me know! And thanks for taking the time in reviewing this :)

ghost commented 1 year ago

Please let me know if I need to implement any extras to make sure that if cryptoya.com is down we don't block any othe functionality. I'll also make some tests on my end to make sure this won't happen.

If there's just one API for a market and it goes down there will be no market price for the bisq client. In that case, the client has no alternative but to disable all market-based offers and users will get this message:

price_error

If there are multiple providers for a market the pricenode will still use the ones which are working, and as a result Bisq will continue uninterrupted. That's why we have redundancy in providers.

crypto-ya is fine but its just one provider. The problem of redundancy can be solved by adding another price provider for the ARS market, perhaps as @pazza83 suggested, obtaining a USD/ARS blue rate and converting it to BTC/ARS.

rodvar commented 1 year ago

Please let me know if I need to implement any extras to make sure that if cryptoya.com is down we don't block any othe functionality. I'll also make some tests on my end to make sure this won't happen.

If there's just one API for a market and it goes down there will be no market price for the bisq client. In that case, the client has no alternative but to disable all market-based offers and users will get this message:

price_error

If there are multiple providers for a market the pricenode will still use the ones which are working, and as a result Bisq will continue uninterrupted. That's why we have redundancy in providers.

crypto-ya is fine but its just one provider. The problem of redundancy can be solved by adding another price provider for the ARS market, perhaps as @pazza83 suggested, obtaining a USD/ARS blue rate and converting it to BTC/ARS.

@jmacxx I see what you meant now, thanks for the clarification. I saw @pazza83 suggestion in matrix and I think is a great idea. This is gonna take me a little while cause I don't have much availability at the moment. Do you think we can merge this as is since its already an improvement from having 0 ARS/BTC exchange rates or we rather wait till I have at least 1 redundant exchange price provider? Thanks!!!

PS. also if I go with the ARS -> USD -> BTC approach, what would be the best way to get the USD/BTC rate from my new ExchangeRateProvider? Thanks!

rodvar commented 1 year ago

I just had this idea. I could tap into for example into CoinGecko ExchangeRateProvider which already fetches the official ARS/BTC rate (~ 8.5 x 10e6) and using the https://api.bluelytics.com.ar/v2/latest calculate the gap multiplier BLUE_USD_RATE / OFFICIAL_USD_RATE (at the moment is ~2.07) and that can give us a realistic ARS/BTC of approx 1.7 x 10e7 :D

This could be a faster approach and also applicable to other exchanges that already fetch the official ARS/BTC but exclude them based on bisq.price.fiatcurrency.excluded (I think we should keep this exclusion and theat this as the specific scenario it is in each ExchangeRateProvider).

Let me know your thoughts!

ghost commented 1 year ago

:point_up: The idea for a second BTC/ARS provider based on https://api.bluelytics.com.ar/v2/latest sounds good @rodvar.

Perhaps @Emzy could give an opinion about whether this PR would be deployed to production, or wait for a second BTC/ARS provider. I expressed my thoughts above, and maybe the team wants forge ahead with the experiment, aware of the risks of having just one provider, to quickly see if an ARS market develops?

HenrikJannsen commented 1 year ago

Maybe we should display some info in the app about the usage of blue rate and reasons for it (probably not really needed for ppl who trade ARS) and a warning that the provided rate does not match Bisq's rate calculation standards (to have multiple providers). But I think its a big improvement over the current state where the rate is basically un-usable, even it carries a slight risk.

HenrikJannsen commented 1 year ago

Another idea an Argentinian trader brought up in a discussion was to use USD for the trade price but use the ARS in the payment method. Basically adding a "Transfer ARS via bank" payment method which use USD for the trade price.

The reason for that was that with 2 volatile markets ARS/USD, USD/BTC its getting complicate to assess risks. When they can trade BTC with USD price but do the payment then with the spot price for the ARS/USD rate, that price volatility is at least removed from the trade. Still it will be a problem how to define that rate. So not sure if it would really solve anything, but wanted to post that idea as maybe ppl more familiar with trading in multiple volatile currencies have some idea how to solve that problem.

rodvar commented 1 year ago

@HenrikJannsen @jmacxx have a look at my last commit. Ideally should refactor the bluelytics stuff out of CoinGecko Provider to be reused in other exchanges that provide the official ARS/BTC. This can be done if we wanna add more redundancy. Let me know your thoughts!

rodvar commented 1 year ago

@jmacxx @HenrikJannsen I've created a draft PR to work on the above and leave this one clean and ready to go.

https://github.com/bisq-network/bisq-pricenode/pull/17

Let me know you thoughts, thanks for all the help!