-
Notifications
You must be signed in to change notification settings - Fork 40
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
kucoin futures #104
base: main
Are you sure you want to change the base?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one last nitpick from me. Otherwise, this looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before that happens, the position will be liquidated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used to by -> used by
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -203,6 +203,25 @@ Notes: | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to line 157 (at the end of the REST section in alphabetical order).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.