Implement CCXT pair converter plugin about dali-rp2 HOT 35 CLOSED

Thanks for promptly getting that in. I think that will be a huge help.

For some reason I was thinking the Fiat plugin #50 would be a separate plugin, but it actually should be a part of a pair converter plugin since it will need to convert the available fiat amount into the one being requested by the transaction resolver.

Which of the following should I do?

1. make it a part of just the CCXT pair converter
2. make it a function of abstract_pair_converter_plugin.py so it can be used by any pair converter that would need it.
3. make it a separate class that can be imported and used in any pair converter plugin as needed.

My guess is 2), but what do you think?

from dali-rp2.

No problems with explaining all the details. My day job is as an English teacher and I've had to explain that 'I go to shopping' is incorrect well over 1000 times already. I'm used to explaining things. Besides, it's probably a good idea to rethink and reanalyze anything to do with taxes and accounting to make sure everything is accurate. There are plenty of half-baked solutions out there now, I'd like to make one that is robust and works well.

I still have to finish up a few fringe cases for the converter, but it is starting to shape up. One thing I noticed was that the cache key NamedTuple for the abstract class doesn't contain the exchange name. Should it be added?

from dali-rp2.

I don't think anybody is a git guru. Every time I use it, I seem to screw something else up about it.

I think I got it. Just for future reference:

Basically, I merged changes from the main repository (via Github). Then, made two commits locally, tried to push them, and failed because I was behind the remote. I pulled changes and then something happened where I couldn't push the changes and I got that error. Maybe I changed one of the merged files? Or maybe it is a very bad idea to pull a remote commit while you have two local commits?

Resetting the head to before the merge, pulling changes, remaking my local changes (from a backup), and then pushing to remote fixed it.

Another day, another way to mess up git commits. Thanks for the help!

from dali-rp2.

It doesn't look like CCXT has a forex exchange, yet.

There is another option with exchangerate.host

But, it seems a little suspicious that they have a pricing API that is completely free with no limits. It seems easy enough to implement though as a fallback plugin that can fill in any missing prices.

Other services offer pricing/conversion to ~200 fiat currencies but are often limited to 250 transactions a month, which we would probably use fairly quickly.

from dali-rp2.

Ok, let's do whatever works. I gave a quick look at exchangerate.host and it seems fine: we won't even need an extra Python library because it's just JSON and network calls.

from dali-rp2.

Is it possible to rewrite transaction_resolver.py and abstract_pair_converter_plugin.py to pass the exchange name to _get_historic_bar_from_native_source?

If we do that, the CCXT pair converter plugin can just automatically pull exchange-specific pricing information and the user wouldn't have to explicitly declare it. We also wouldn't have to abstract out the CCXT pair converter plugin and write a new pair converter plugin for every CCXT-supported exchange.

from dali-rp2.

The way I would do that is to add an exchange parameter (and whatever other parameters you need) to the __init__() function of the CCXT plugin: then you can store the exchange in an instance variable and use it as needed. This means that when the user adds a CCXT pair_converter to the .ini file they'll have to pass the exchange parameter as well (the .ini section for the pair_converter plugin automatically reflects the signature of the __init__() function). This doesn't require any changes to the existing code and in my mind it's preferable, because it forces the user to declare explicitly what exchange they want to use (whereas implicit choices can cause unintended, hard-to-explain side-effects).

from dali-rp2.

from dali-rp2.

Here's how I imagined this would work:

• create a new pair converter plugin: a subclass of AbstractPairConverter living in src/dali/plugin/pair_converter (as described here). Let's call this class CCXT and imagine it is defined in ccxt.py.
• CCXTPairConverter has an __init__() method that looks like this:

class CCXT(AbstractPairConverterPlugin):
    def __init__(self, historical_price_type: str, exchange: str) -> None:
        super().__init__(historical_price_type=historical_price_type)
        self.__exchange = exchange
        ...

• CCXT has a get_historic_bar_from_native_source() method that looks like this:

    def get_historic_bar_from_native_source(self, timestamp: datetime, from_asset: str, to_asset: str) -> Optional[HistoricalBar]:
        # use CCXT API and self.__exchange as needed to create and return the HistoricalBar
        ...

• CCXT also has name() and cache_key() methods:

    def name(self) -> str:
        return f"ccxt_{self.__exchange}_pair_converter"

    def cache_key(self) -> str:
        return self.name()

The user would need to specify one or more pair converter sections in their .ini file, which contain the parameters to pass to ini. E.g. if the user wants to use CCXT/Binance first and CCXT/Coinbase as a fallback for price conversions, they would add the following to their .ini file:

[dali.plugin.pair_converter.ccxt binance]
historical_price_type = high
exchange = binance
[dali.plugin.pair_converter.ccxt coinbase]
historical_price_type = high
exchange = coinbase

This will ensure that when converting prices DaLI will try to use the ccxt/binance plugin first and the ccxt/coinbase plugin if the previous one failed.

Does this answer your question? Let me know if you think I'm missing something we're not on the same page.

from dali-rp2.

This will unfortunately provide inaccurate results. I'll give you a real world example.

Currently, I own BETH on Pionex. It is trading at 0.931 ETH to 1 BETH. If I were to sell BETH for ETH, the pair converter would record the sale at the current rate from Binance (if I list that first in the ini), which is 0.9748. This is obviously inaccurate pricing data.

The pricing of crypto assets on Japanese exchanges is almost always cheaper in a bear, and more expensive in a bull. So, if I buy XLM in Japan and transfer it to another exchange it is actually cheaper than converting JPY to USD and then buying the asset (during the current bear market).

For example, if the exchange rate is 120 JPY to USD. I can buy 120 Yen worth of XLM, send it, convert it to a stable and sometimes have 1.02 in stable. There is a profit there that wouldn't be recorded if we just took the price of XLM from Binance.

Considering these cases, I think it would be helpful to either have the CCXT plugin choose the exchange based on where the transaction took place (my original proposal) or allow the declaration of a pair converter plugin per exchange.

from dali-rp2.

Got it, thanks for the additional explanation: I hadn't understood the use case correctly. I think your initial proposal is OK, then. We'll add an exchange parameter to get_historic_bar_from_native_source(): the plugin is free to use or ignore this new parameter (the CCXT plugin will use it, the Historic-Crypto plugin will ignore it). I'll work on this.

from dali-rp2.

The transaction resolver will read the exchange from the transaction it is processing and pass it to the pair converter. However, how should it deal with intra-transactions (which include two exchanges)?

from dali-rp2.

I think it would carry over the spot price when it was purchased from the exchange it was sent from. According to Coinbase, the cost basis should be carried over.

It is my understanding that you should keep the cost basis and date and time of purchase and that a transfer doesn't really affect anything tax wise other than recording the transfer fee as disposal of the asset (similar to making a purchase with crypto assets). The IRS has some answers to questions that deal with this (Q38~Q41).

from dali-rp2.

Agreed, cost basis is carried over, but I should have specified more clearly what I meant. I was thinking of the moving fee, which is probably not taxable, but affects the in/out flow, so RP2 keeps track of it. Is there ever a situation where you move some crypto between two exchanges which denominate that transaction in two different fiats? In that case what should the transaction resolver do? Currently DaLI has no ability to model an intra-transaction with two different fiats.

from dali-rp2.

To clarify further: I'm thinking of one intra-transaction between, say, Binance.com and Coinbase, which is denominated in JPY on Binance.com and EUR on Coinbase. Can this happen?

from dali-rp2.

It is possible. Not Binance.com, since it doesn't really have a base currency, but I could transfer an amount from Bitbank which only deals in JPY to Coinbase US, which is denominated in USD. And the transaction fee would be denominated in the corresponding fiat.

In that case, I would just take the fee denominated in the sending exchange's fiat. I feel like that is where it is 'spent'. Or use the fiat that is the base fiat for the country plugin if that makes sense.

As for the exchange fee, my understanding is that the transaction fee is taxable as 'disposal' (i.e. as if you used the crypto to make a purchase of, say, a Tesla.) You would basically calculate it as a sale of the crypto and have to pay capital gains on it (according to US tax law). Koinly has a pretty good explanation and example, but I've also heard other reputable sources echo this.

from dali-rp2.

OK, using the sending exchange fiat sounds reasonable: I may have to fix the transaction resolver to deal with the specific case of an intra-transaction between two exchange that denominate in different fiats, because I hadn't considered it before.

As for transfer fees, thanks for the link: I had read a different interpretation that considers them non-deductible investment expenses.

In any case this interpretation doesn't affect this particular discussion because RP2 is agnostic to how to treat transfer fees: it collects them and computes their fiat impact and then it reports them in the "Investment Expenses" tab. Then it's up to the user to decide which interpretation to use for transfer fees.

from dali-rp2.

I fixed and pushed what we discussed above:

• AbstractPairConverter.plugin.get_historic_bar_from_native_source() now has a new exchange parameter, which the plugin is free to use or ignore
• the transaction resolver and a few more files have been updated to use the new parameter. For in and out-transactions exchange is used, for intra-transactions from_exchange is used.

There was no need to add any logic to deal with the case of intra-transaction denominated in two different fiats on two different exchanges: the existing code is already doing the right thing. The two unresolved halves of the intra-transaction (coming from different exchanges/plugins and denominated in different fiats) have their fiat fields converted to the default fiat before being resolved (merged) into one transaction.

If you have any questions or problems with this new code let me know.

from dali-rp2.

In my mind a pair converter plugin should convert any pair (as long as the information is available in its native historical database), without needing to distinguish fiat vs crypto. So I guess I was thinking 1 plus a reasonable default. By "reasonable default" I mean that if the user doesn't specify any pair converters in the .ini file, the code should use a default chain of converters that covers most reasonable cases (so defining pair converters in the .ini file should be limited to special situations): e.g. the default chain could look like CCXT, Historic-Crypto (for now, because we don't have anything else, but later it could include also others), so that most situations are handled. However you're more familiar than I am with the international use case: do you see problems with the solution I described and do you see any specific advantage to doing 2?

from dali-rp2.

Well, CCXT can't return spot prices in JPY without a conversion from USD to JPY. Binance.com doesn't provide any estimates in any fiat. It only provides fiat spot prices for the markets it has (e.g. BTC/GBP). So, the CCXT will have to hand off the USD amount to the fx converter before it can return an amount in JPY.

Here is the pseudo code:

def get_historic_bar_from_native_source(self, timestamp, from_asset="BTC", to_asset="JPY", exchange="binance"):
       if to_asset not in _EXCHANGEFIATLISTS[exchange]:
              stable = _DEFAULTSTABLE[exchange]

              # OPTIONAL iso code - allows support for exchanges that don't have high volume in a USD stable coin
              # I don't think we need it at present
              # For now it will be USD for most users, but could be another currency for an international exchange
              stable_iso = _DEFAULTSTABLEISO[exchange] 
              historical_raw = binance.fetchOHLCV(f"BTC/{stable}", "1m", timestamp, 1)
              historical_data = _forex_converter(historical_raw, from_fiat=stable_iso, to_fiat=to_asset)
      else:
            # no need to pull stable coin, just use fiat data from exchange
      
      result = HistoricalBar( .... )
      return result

I guess the best design, now that I think about it, is to extend the abstract class with a class that does the forex converting. Then, CCXT inherits from that. That way anyone who wants to write a plugin in the future can explicitly inherit from the base class with the forex conversion:

forex_pair_converter_plugin.py:

class ForexPairConverterPlugin(AbstractPairConverterPlugin):

ccxt_converter.py:

class PairConverterPlugin(ForexPairConverterPlugin):

I hope all that makes sense.

from dali-rp2.

So, let's take the example of Binance.com, which doesn't have a conversion USD->JPY. Let's say for sake of discussion we keep the design as it is today, so the binance.com-based pair converter isn't able to convert dollars to yen. However let's say we add another pair converter that has that pair (maybe based on exchangerate.host). Then we make the pair converter chain as follows:

• CCXT / Binance.com
• exchangerate.host

Note that the above could be the default chain or it could be user-defined (it's immaterial to this specific discussion).

So when the transaction resolver needs to convert USD->JPY it tries with the first pair converter (CCXT/Binance.com) and fails. Then it tries with the second (exchangerate.host) and succeds.

Is this enough to cover the use case? Based on my understanding so far, it should be: does this design break somewhere?

from dali-rp2.

Let's say I trade USDT to BTC on Binance.com.

My understanding is that the transaction_resolver.py will try to get the spot price in JPY (if that is what is configured in global). So it makes the request to CCXT converter -> get_pair_conversion_rate(timestamp, "BTC", "JPY", "Binance.com", global_configuration)

That gets rejected since Binance.com doesn't have the pair in JPY. The transaction resolver moves on to the Forex converter (based on exchangerate.host) and makes the same call 'get_pair_conversion_rate(timestamp, "BTC", "JPY", "Binance.com", global_configuration)'. It also gets rejected because we want exchangerate.host to only handle fiat and not crypto assets, since it is more accurate to get crypto asset prices from the exchange where the transaction took place (based on the conversations we had above).

The transaction resolver doesn't know to convert USD -> JPY unless we can return the USD spot price in the first call and put the ISO code with it.

from dali-rp2.

To clarify further, the _convert_fiat_fields_to_native_fiat function will convert the fiat amount for any normal purchase. But for conversions, the fiat_in is not known and will have to be pulled from the web, then converted to the fiat declared in the global config. The transaction resolver doesn't do that currently does it?

from dali-rp2.

Ok, I now understand your reasoning for BTC->JPY conversion failing, so your proposal of adding forex conversion capability to the superclass makes sense. Sorry, not having direct experience with the use case slows me down a little, but my intention is to make sure:

• we support the foreign exchange use case fully,
• I understand all its nuances.

So thanks for being patient and going over the details (more than once :-) ): this is very educational for me.

Regarding the comment on _convert_fiat_fields_to_native_fiat, check the code between line 239 and 260: https://github.com/eprbell/dali-rp2/blob/main/src/dali/transaction_resolver.py#L239

If fiat_ticker is not the default fiat, it tries to convert fiat fields (if any) to the default fiat, then if the spot price is unknown it tries to read it from the web. Note that if any fiat fields are None then they are computed later by RP2 as crypto_field * spot_price.

from dali-rp2.

I still have to finish up a few fringe cases for the converter, but it is starting to shape up. One thing I noticed was that the cache key NamedTuple for the abstract class doesn't contain the exchange name. Should it be added?

Can you point me to the exact place in the code? The cache_key() method is implemented in the subclass and can be whatever makes sense for plugin.

from dali-rp2.

I think the NamedTuple needs to include exchange. And then when the key is created the initializer call needs to include exchange.

Or maybe I'm just reading it wrong. I haven't done much work with a cache.

I've actually already added it to my code so that I can test caching on line 27:

class AssetPairAndTimestamp(NamedTuple):
    timestamp: datetime
    from_asset: str
    to_asset: str
    exchange: str

and line 65:

        key: AssetPairAndTimestamp = AssetPairAndTimestamp(timestamp, from_asset, to_asset, exchange)

from dali-rp2.

Yes, that looks correct: the new exchange parameter should be reflected in the key, otherwise we run the risk of overwriting some values in the cache.

from dali-rp2.

I'm having a serious issue with github not accepting a push. I keep getting this error:

Enumerating objects: 7, done.
Counting objects: 100% (7/7), done.
Delta compression using up to 4 threads
Compressing objects: 100% (4/4), done.
Writing objects: 100% (4/4), 350 bytes | 350.00 KiB/s, done.
Total 4 (delta 3), reused 0 (delta 0)
remote: Resolving deltas: 100% (3/3), completed with 3 local objects.
remote: fatal: did not receive expected object 74149a535ef3322dc4d58f25d22cff4ed37bd0e3
To github.com:macanudo527/dali-rp2.git
 ! [remote rejected] ccxt_converter -> ccxt_converter (failed)
error: failed to push some refs to '[email protected]:macanudo527/dali-rp2.git'

I've searched high and low to figure out what this is and how to resolve it and can't find anything that helps. I re-cloned to a different directory. I've re-init the local git repository. I've tried everything I can to fix it and it just won't push. Have you ever seen anything like this?

I'm afraid I might have to re-fork the whole project at this point. Nothing seems to be working.

from dali-rp2.

Unfortunately I'm not a git guru, so I'm not exactly sure what may be going on. A couple of things you may already have tried:

• git pull or git pull --rebase
• then git push

If that doesn't work, it may be easier to just start anew (refork and reapply your changes).

from dali-rp2.

It looks like the rate converter plugin is re-instantiated for every transaction. Is it possible to keep the instantiation across each of the calls?

The plugin would work more efficiently if that were the case. Basically, if you make a call for the BTC to USD rate, some exchanges will not have that pairing, so I've allowed for a default stable coin to be used per exchange. But, it would be more efficient if the plugin could check against a cached list of markets from the exchange instead of making a failed call to the API each time.

I could just ask it to replace the fiat with a stable coin each time per exchange, but then if the exchange ever does list the fiat market, it wouldn't reflect that. I would rather it check every time for a fiat market than fall back on a stable coin market.

Also, the fiat converter can cache a list of fiat currencies that are pulled dynamically and identify those conversions and send them off to that function instead of trying and failing to pull the conversion from a crypto exchange.

If this is too much of an issue I can have it try and fail each time. I hope that all makes sense. Let me know if you have questions.

from dali-rp2.

Can you point me to the exact place in the code? Pair converter plugins should be instantiated only once per DaLI run, see here.

Also I'm not sure I follow the rest of the description you gave: could you provide an example?

from dali-rp2.

Can you point me to the exact place in the code? Pair converter plugins should be instantiated only once per DaLI run, see here.

Oops, I misread the code. It should be ok for what I'm trying to do.

Also I'm not sure I follow the rest of the description you gave: could you provide an example?

It's probably just better to show you the code at this point. Let me finish up writing the "no fiat pair" conversion code and I'll push a commit and link to it. I might be able to get to it in 12 hours or it might be in 3ish days (having a short vacay).

from dali-rp2.

Sounds good. Enjoy the time off!

from dali-rp2.

Okay, I created a pull request #53 for it. I tried to document everything as best I could. I'll let you look over it. I'm sure you'll have questions.

from dali-rp2.

I think this can be closed for now. Let's open a new issue for the improvements.

from dali-rp2.