Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/ars pricenode #15

Merged
merged 2 commits into from
Aug 13, 2023
Merged

Conversation

rodvar
Copy link
Contributor

@rodvar rodvar commented Aug 3, 2023

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

  • A passing test is provided in this PR

** 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
Copy link

ghost commented Aug 3, 2023

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.

@rodvar
Copy link
Contributor Author

rodvar commented Aug 3, 2023

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 rodvar force-pushed the feature/ars_pricenode branch 2 times, most recently from 45cad71 to ab079fd Compare August 4, 2023 02:03
@rodvar
Copy link
Contributor Author

rodvar commented Aug 4, 2023

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

@rodvar rodvar force-pushed the feature/ars_pricenode branch from ab079fd to ba6f205 Compare August 4, 2023 04:50
- Enforce bisq module Guava version
@rodvar rodvar force-pushed the feature/ars_pricenode branch 4 times, most recently from 9685e93 to 9e09d79 Compare August 4, 2023 05:34
@ghost
Copy link

ghost commented Aug 4, 2023

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.

Copy link

@HenrikJannsen HenrikJannsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

 - setup cryptoya api price provider
 - add cryptoya pricenode provider
 - algorithm to decide rate based on fetched tickers
 - add test for new exchange rate provider
 - code review suggestions
@rodvar rodvar force-pushed the feature/ars_pricenode branch from 9e09d79 to a3cd9a1 Compare August 4, 2023 23:16
@rodvar
Copy link
Contributor Author

rodvar commented Aug 4, 2023

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
Copy link
Contributor Author

rodvar commented Aug 5, 2023

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
Copy link

ghost commented Aug 5, 2023

☝️ 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
Copy link

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
Copy link

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
Copy link
Contributor Author

rodvar commented Aug 6, 2023

@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 rodvar force-pushed the feature/ars_pricenode branch from ac6d11c to a3cd9a1 Compare August 6, 2023 23:02
@rodvar
Copy link
Contributor Author

rodvar commented Aug 6, 2023

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

#17

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

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

Copy link
Contributor

@gabernard gabernard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants