Skip to content

Ccxt pair converter#53

Merged
eprbell merged 91 commits into
eprbell:mainfrom
macanudo527:ccxt_pair_converter
Aug 25, 2022
Merged

Ccxt pair converter#53
eprbell merged 91 commits into
eprbell:mainfrom
macanudo527:ccxt_pair_converter

Conversation

@macanudo527

Copy link
Copy Markdown
Collaborator

This is the first draft of the CCXT pair converter. I hope all the features make sense. I tried my best to have the logic make sense, but the 'bridge pricing' (using bridge fiat and bridge crypto assets) might be a little confusing. Things might need to be refactored a little. Let me know if you have questions.

@macanudo527

Copy link
Copy Markdown
Collaborator Author

The best pricing path starts with the ETHGUSD path on Gemini. Then, from there the plugin can grab ETHUSD from another exchange. This is our best route. It's a little odd to price a stable coin through ETH, though.

The ETHGUSD market, like most of the other markets for GUSD, is only on Gemini. The price shown on coinmarketcap, etc... Is probably derived from an average of that and the other major coins traded on Gemini.

So, any unpegging is basically caused by a liquidity shortage on Gemini.

If Gemini fails to redeem 1:1, a user doesn't have a viable market to turn to. There is one on CEX but it lacks enough liquidity to be viable. Gemini would have to freeze withdrawals anyway, and would probably freeze trading, which would close that pricing path.

All that is to say, I can see why we would want a derived price from ETH, but one could make a valid case that, in the particular case of GUSD, locking it to $1 might be ok for some users.

The user-defined pricing could prove useful for that. The one caveat is that it makes it easier to manipulate prices. I know on most of the online crypto tax services I've used, they don't allow you to directly edit things like this. So, I would propose the JSON include a mandatory note field that users would need to fill out that explains the need for the pricing. This would later help with the auditing reporting I proposed before. Also, it would force the user to think about why changing the pricing is a good idea.

They can of course just put some nonsense reason in there, but at least we've warned them. After all, this is open source and if someone was really determined they could add in code to help them manipulate prices as well, so we can't completely prevent someone from cheating.

@eprbell

eprbell commented Aug 15, 2022

Copy link
Copy Markdown
Owner

A few questions:

  • why does ETHUSD have to come from another exchange: is it not available on Gemini?
  • how imprecise is the price if we let the routing mechanism determine it (ETHGUSD, then ETHUSD)?
  • are any other concerns in using the routing algorithm, other than it's "odd to price a stable via ETH"?

I do see your point about the danger of letting the user set price, but on the other end they can also already enter transactions (via the manual plugin).

Also, if we go for the manual file solution, let's use .ini, not .json (to be consistent with DaLI user config files).

@iloveitaly

Copy link
Copy Markdown
Contributor

I know on most of the online crypto tax services I've used

Do you know how they handle this sort of thing? I'm guessing part of the value-add they offer is historical pricing DBs for stuff like this?

So, I would propose the JSON include a mandatory note field that users would need to fill out that explains the need for the pricing

I like this idea.

let's use .ini, not .json

IMHO I think json would be a better option here, although it does diverge from the existing ini style.

  • It's hard to create complex data structures with ini. To my knowledge, you can't create subhashes, right?
  • Everyone knows JSON, only the python community really heavily uses ini. This will make dali/rp2 more accessible to others.
  • It's more portable if other systems needed to generate/read/manipulate this data for any reason.

@macanudo527

Copy link
Copy Markdown
Collaborator Author

A few questions:

  • why does ETHUSD have to come from another exchange: is it not available on Gemini?

For some reason, I thought all the markets on the Gemini exchange had GUSD as the quote asset. It seems there is a ETHUSD market there we can make use of.

  • how imprecise is the price if we let the routing mechanism determine it (ETHGUSD, then ETHUSD)?

How can we gauge precision? Basically, the ETHGUSD market has limited liquidity and that is where the slight unpegging comes from. During times of high volatility like the recent surge in ETH price because of the merge, the GUSD becomes unpegged because liquidity can't keep up. So, is that a precise gauge of the price? Especially, when a user can just redeem GUSD for USD 1:1 on the same exchange where there is a liquidity issue?

  • are any other concerns in using the routing algorithm, other than it's "odd to price a stable via ETH"?

I can't think of any at the moment.

I do see your point about the danger of letting the user set price, but on the other end they can also already enter transactions (via the manual plugin).

Yeah, like I said it is open source so they can just edit it any way they like. I just thought it would be wise to have the user stop and think about why before doing it.

If they use the manual plugin, they are just sorting the data to run through dali-rp2 (under normal circumstances), but the manual price override is purposely editing the price from the default (under the normal use-case). What we provide by default should be correct, except under special circumstances, thus it would be good to provide a reason in case the user is audited.

Also, if we go for the manual file solution, let's use .ini, not .json (to be consistent with DaLI user config files).

I can't really comment either way since I'm new to Python, but consistency sounds good to me unless there is a need for JSON.

@macanudo527

Copy link
Copy Markdown
Collaborator Author

Unrelated, I'm running into a weird KeyError when attempting to get a spot price for BSV:

2022-08-14 06:50:03,078/CCXT-converter/high/DEBUG: Converting BSV to USD
2022-08-14 06:50:03,078/CCXT-converter/high/DEBUG: Found path - ['BSV', 'USDT', 'USD']

The BSVUSDT key doesn't exist in the hash. I'll try to get the exact line numbers. I don't understand how the graph stuff is working and how this could occur—any pointers?

This is a bug in the code, I was using the ID to name the market, but the ID had BSV's old ticker BCHSV. I changed it to concatenate the base and the quote asset, so it should work now.

@eprbell

eprbell commented Aug 15, 2022

Copy link
Copy Markdown
Owner

A few questions:

  • why does ETHUSD have to come from another exchange: is it not available on Gemini?

For some reason, I thought all the markets on the Gemini exchange had GUSD as the quote asset. It seems there is a ETHUSD market there we can make use of.

Nice, I wonder if this would solve this specific problem: If all quotes (ETHGUSD and ETHUSD) come from the same exchange perhaps they would be internally consistent and we would get less imprecision.

  • how imprecise is the price if we let the routing mechanism determine it (ETHGUSD, then ETHUSD)?

How can we gauge precision? Basically, the ETHGUSD market has limited liquidity and that is where the slight unpegging comes from. During times of high volatility like the recent surge in ETH price because of the merge, the GUSD becomes unpegged because liquidity can't keep up. So, is that a precise gauge of the price? Especially, when a user can just redeem GUSD for USD 1:1 on the same exchange where there is a liquidity issue?

Fair point.

I do see your point about the danger of letting the user set price, but on the other end they can also already enter transactions (via the manual plugin).

Yeah, like I said it is open source so they can just edit it any way they like. I just thought it would be wise to have the user stop and think about why before doing it.

Sure, agreed: let's have the user fill in a description field that we can add to the notes.

If they use the manual plugin, they are just sorting the data to run through dali-rp2 (under normal circumstances), but the manual price override is purposely editing the price from the default (under the normal use-case). What we provide by default should be correct, except under special circumstances, thus it would be good to provide a reason in case the user is audited.

I'm not sure I understand this concern about default and override (can you expand on it?): the way I imagine this will work is that the manual plugin is a pair converter plugin like the others. It will be placed by the user in the pair converter section of the configuration file and the order of plugins in that list is what determines which plugin is used first or last. In this sense there is no "default" and no "overriding": the value is given by the first plugin that has a match in that user-provided list (whether it's the manual plugin or any other).

@eprbell

eprbell commented Aug 15, 2022

Copy link
Copy Markdown
Owner

let's use .ini, not .json

IMHO I think json would be a better option here, although it does diverge from the existing ini style.

* It's hard to create complex data structures with ini. To my knowledge, you can't create subhashes, right?
* Everyone knows JSON, only the python community really heavily uses ini. This will make dali/rp2 more accessible to others.
* It's more portable if other systems needed to generate/read/manipulate this data for any reason.

All of the above is true in general, however in this specific case:

  • we don't need complex data structures, just key-value pairs;
  • JSON is well-known to programmers, but the configuration file is user-facing and .ini files are friendlier to users than .json ones;
  • portability isn't really a concern for us: Python runs on all OSes we are reasonably interested in supporting.

In RP2 (which was written before DaLI) I used JSON for the configuration file, but now I regret that decision for the reasons outlined above.

@iloveitaly

Copy link
Copy Markdown
Contributor

Re: manual price conversion, here's a PR with the WIP converter I have running locally:

#77

There's some TODOs in the source with some of the questions/issues I ran into. If we need to indicate time ranges for a particular symbol (i.e. timestamp and timerange on each symbol entry) how could we structure that in the ini format? Or are you thinking of pointing to an CSV? Either way, we can continue the conversation there.


Re: this PR, here's another round of changes I needed to get around some of the missing symbols: macanudo527#5. At this point the dali loader is working for me, crossing my fingers rp2 runs fairly easily :)

@eprbell

eprbell commented Aug 16, 2022

Copy link
Copy Markdown
Owner

Thanks! I think the .ini format would simply mimic HistoricalBar:

  • duration
  • timestamp
  • open
  • high
  • low
  • close
  • volume
  • notes

Actually, as I wrote the above I realized that CSV would be a valid, user-friendly alternative to .ini in this case: the user could just fill a CSV file with the above columns.

To capture a range value in .ini you can have a start and an end field or a start and a duration field (as above).

Let us know how it goes with RP2!

@macanudo527

Copy link
Copy Markdown
Collaborator Author

I added the ability for users to define a default exchange if the exchange attached to the transaction is not currently supported.

@eprbell

eprbell commented Aug 21, 2022

Copy link
Copy Markdown
Owner

I started refactoring classic data structures and algorithms in a separate library called "prezzemolo", which is now a dependency of both RP2 and DaLI. Currently it contains AVLTree and Graph/Vertex and it will be expanded with new components as needed. The reason I mention this here is that the Graph class can be useful in this PR to replace ad-hoc graph logic: it has a find_shortest_path() method which is based on breadth-first search. See example here: https://github.com/eprbell/prezzemolo/blob/main/tests/test_graph.py. I hope it can be helpful.

@macanudo527

Copy link
Copy Markdown
Collaborator Author

I started refactoring classic data structures and algorithms in a separate library called "prezzemolo", which is now a dependency of both RP2 and DaLI. Currently it contains AVLTree and Graph/Vertex and it will be expanded with new components as needed. The reason I mention this here is that the Graph class can be useful in this PR to replace ad-hoc graph logic: it has a find_shortest_path() method which is based on breadth-first search. See example here: https://github.com/eprbell/prezzemolo/blob/main/tests/test_graph.py. I hope it can be helpful.

Ok, thanks for that! I want to get this working, merge it, and then implement that.

@macanudo527

Copy link
Copy Markdown
Collaborator Author

Everything should be working on everything. I'm okay with rolling everything through (Binance rest/csv, docs, and the CCXT pair converter)

Then, I'll be focusing on abstracting the CCXT plugin.

After that, this is my rough Todo:
Build coincheck and bitbank plugins based on CCXT.
Build Pionex Csv plugin
Build JP tax plugins

I'd like to get an MVP on those since I need them for taxes and it's getting close to the end of the year. Then, I'd like to focus on refactoring and cleaning.

@eprbell

eprbell commented Aug 24, 2022

Copy link
Copy Markdown
Owner

Great! I'll take one last look at the pair converter and then we'll merge everything. Your plan for future activities is fantastic: thanks for taking on all of this good stuff :)

@eprbell eprbell left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't have any more feedback on this: ready to merge.

@eprbell
eprbell merged commit c9d3406 into eprbell:main Aug 25, 2022
@eprbell eprbell mentioned this pull request Sep 24, 2023
@github-staff github-staff deleted a comment May 12, 2024
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.

3 participants