kucoin futures#104
Conversation
added kucoinfutures stubs
added kucoin_futures section
fixing typo bug
fixing typo bug
|
FEE could actually be used for loss. I think we just need to add another transaction type called LOSS that is an exact copy of FEE. GIFT is unrealized gain. Yet GAIN needs to be realized gain, so I got stuck on this part. I actually haven't gone through the rp2 codebase extensively, so I am not sure how much major change it would require for rp2 to incorporate GAIN transaction type. |
change transaction types GIFT -> INCOME, and FEE -> SELL
|
@macanudo527, could you help review this PR (it has a subclass of |
macanudo527
left a comment
There was a problem hiding this comment.
Let me know if you need help implementing the pagination details. It looks like the data fetching can be handled that way, and then the code will be easier to maintain.
code review changes
code review changes
macanudo527
left a comment
There was a problem hiding this comment.
Just one last nitpick from me. Otherwise, this looks great!
eprbell
left a comment
There was a problem hiding this comment.
Add documentation here (remember to update the table of contents): https://github.com/eprbell/dali-rp2/blob/main/docs/configuration_file.md#data-loader-plugin-sections
If you want you can also add a test to avoid regressions, but that's up to you. Here's an example: https://github.com/eprbell/dali-rp2/blob/main/tests/test_plugin_binance_com.py
| exchange=self.exchange_name(), | ||
| holder=self.account_holder, | ||
| transaction_type=Keyword.SELL.value, | ||
| spot_price="0", |
There was a problem hiding this comment.
Why is spot price 0? This would cause a 0 fiat loss: in other words it would be a taxable event with no impact.
There was a problem hiding this comment.
I could be wrong, but this is my thought process.
If we set spot price to 0, it would incur ((acquisition price - spot price) * quantity) loss and will be considered as taxable event. If this is not how it works, at least that is my intention and I would like to know how to code that as I am not sure how to do that.
There was a problem hiding this comment.
I don't have Kucoin so I'm just going by the example JSON above: does the amount field contain the fiat amount of the loss (in other words is -66 the fiat difference between the price at acquisition of the future contract and the price at expiration)?
There was a problem hiding this comment.
-66 would be the amount of USDT realized loss (it isn't fiat amount). So if you had 3000 USDT before, you would be now left with 2934 USDT after closing your future position.
If the amount is positive (e.g 66 not -66), then it would be the amount of USDT realized gain. If the currency was XBT, then it would be BTC realized gain. -66 amount with XBT currency would be, realized loss of 66 Bitcoins (66 * $ 17000 in fiat values at current price)
There was a problem hiding this comment.
Oh, I see, thanks for the explanation. What would happen if the loss is -66 USDT at expiration but the user never had any USDT?
There was a problem hiding this comment.
before that happens, the position will be liquidated.
There was a problem hiding this comment.
Could we just represent the differential loss as a SELL transaction of 66 USDT with the actual spot price (or UNKNOWN if the exchange doesn't provide it)? This way it would be treated as a normal loss by the tax engine.
There was a problem hiding this comment.
I am kind of confused as to why the spot price needs to be set to market price.
ex)
1 30 USDT bought with USD at $ 1.00 spot price
2. Bought a future contract.
3. Contract got liquidated. 30 USDT realized loss reported.
So, are you suggesting step 3 to be represented as a sell (OutTransaction) with spot price of $1.00 ? Wouldn't this generate a tax report of 0 capital loss?
My assumption was to represent the above as the following.
step 1 -> Buy InTransaction with amount 30, currency USDT, spot price $1.00
step 3-> Sell OutTransaction with amount 30, currency USDT, spot price $0.00
There was a problem hiding this comment.
OK, I see what you mean: you want to ensure the 30 USDT are modeled as lost, not sold and so by setting the spot price to 0 you achieve that. Sorry it took me a while to get it: it looks good to me. @macanudo527, do you have any more thoughts on this? If not I think we can move on.
code review fixes
added kucoin futures section
| thread_count = <em><thread_count></em> | ||
| </pre> | ||
|
|
||
| Note: the `thread_count` parameter is optional and denotes the number of parallel threads used to by the plugin to connect to the endpoint. Kucoin Futures is rate limited by account/userid, so using `thread_count` of 1 is recommended (https://docs.kucoin.com/futures/#request-rate-limit). |
There was a problem hiding this comment.
used to by -> used by
macanudo527
left a comment
There was a problem hiding this comment.
Nitpicks for the docs. Otherwise, this is good.
It would be nice if you could post tests to keep it from regressing.
The failing test is due to a server timeout and doesn't affect the validity of this pull. Sorry about that.
| for sells, please [open an issue](https://github.com/eprbell/dali-rp2/issues). | ||
| * The Coincheck REST API only supports BTC trades, withdrawals, and deposits and there are currently no plans to implement it. | ||
|
|
||
| ### Kucoin Futures Section (REST) |
There was a problem hiding this comment.
This should be moved to line 157 (at the end of the REST section in alphabetical order).
There was a problem hiding this comment.
Also, add an entry to the TOC up top.
|
What is the status of this review? Kucoin is an exchange I'm interested in. Is Kucoin futures same as Kucoin spot or would a new plugin need to be written for that API? |
|
Kucoin and Kucoin Futures are two different CCXT classes and basically behave like separate exchanges, Albeit two well-connected exchanges. I think it is best to have separate plugins for them, so it is best to write another one that inherits from |
There are a few comments that still need to be addressed, otherwise it looks almost ready. |
No description provided.