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

kucoin futures #104

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

kucoin futures #104

wants to merge 13 commits into from

Conversation

topcoderasdf
Copy link

No description provided.

added kucoinfutures stubs
added kucoin_futures section
@topcoderasdf topcoderasdf changed the title kucoint futures kucoin futures Nov 5, 2022
@topcoderasdf
Copy link
Author

topcoderasdf commented Nov 5, 2022

#100 issue hasn't been solved. So I've used GIFT for GAIN and FEE for LOSS instead.

I am actually not sure how to implement GAIN/LOSS transaction type at the moment

Edit: also addresses this issue #81

@topcoderasdf
Copy link
Author

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
@eprbell
Copy link
Owner

eprbell commented Nov 6, 2022

@macanudo527, could you help review this PR (it has a subclass of AbstractCcxtInputPlugin)? Thanks!

Copy link
Collaborator

@macanudo527 macanudo527 left a 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.

src/dali/plugin/input/rest/kucoin_futures.py Outdated Show resolved Hide resolved
src/dali/plugin/input/rest/kucoin_futures.py Outdated Show resolved Hide resolved
src/dali/plugin/input/rest/kucoin_futures.py Show resolved Hide resolved
src/dali/plugin/input/rest/kucoin_futures.py Outdated Show resolved Hide resolved
src/dali/plugin/input/rest/kucoin_futures.py Outdated Show resolved Hide resolved
code review changes
code review changes
Copy link
Collaborator

@macanudo527 macanudo527 left a 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!

src/dali/plugin/input/rest/kucoin_futures.py Outdated Show resolved Hide resolved
Copy link
Owner

@eprbell eprbell left a 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

src/dali/plugin/input/rest/kucoin_futures.py Outdated Show resolved Hide resolved
src/dali/plugin/input/rest/kucoin_futures.py Outdated Show resolved Hide resolved
src/dali/plugin/input/rest/kucoin_futures.py Outdated Show resolved Hide resolved
exchange=self.exchange_name(),
holder=self.account_holder,
transaction_type=Keyword.SELL.value,
spot_price="0",
Copy link
Owner

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.

Copy link
Author

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.

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

Copy link
Author

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)

Copy link
Owner

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?

Copy link
Author

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.

Copy link
Owner

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.

Copy link
Author

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

Copy link
Owner

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.

thread_count = <em>&lt;thread_count&gt;</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).
Copy link
Collaborator

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

Copy link
Collaborator

@macanudo527 macanudo527 left a 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)
Copy link
Collaborator

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).

Copy link
Collaborator

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.

@ndopencode
Copy link
Contributor

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?

@macanudo527
Copy link
Collaborator

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 abstract_ccxt_input_plugin.py

@eprbell
Copy link
Owner

eprbell commented Feb 26, 2023

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?

There are a few comments that still need to be addressed, otherwise it looks almost ready.

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.

None yet

4 participants