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

9985 delist schedule #9989

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

jakubikan
Copy link

@jakubikan jakubikan commented Mar 22, 2024

Summary

This will implement exchange function to check whether a pair is delisting
If it will be delisting it will return the delisting time.

Solve the issue: 9985

Probably this need some more to have this working in the strategy with the confirm_trade_entry
or custom_trade_exit

but not sure yet - requesting some review if thats the right way to go

One thing I'm not sure of yet this will work in the end
Because essentially - what if binance does not activate have the delist schedule api entry with the announcements of the announcements page.
Because when there was the last announcement I looked that the pairs in the Binance app and they have not been marked to be delisted yet. Might als have been a caching issue or something - but not sure
i guess this can only be really tested with next announcement 😄

Quick changelog

  • <change log 1>
  • <change log 1>

What's new?

@jakubikan jakubikan marked this pull request as ready for review March 22, 2024 20:49
@@ -187,6 +187,11 @@ def __init__(self, config: Config, *, exchange_config: Optional[ExchangeConfig]
self._api_async = self._init_ccxt(
exchange_conf, ccxt_async, ccxt_kwargs=ccxt_async_config)

if exchange_conf.get('sandboxMode'):
self._api.set_sandbox_mode(True)
self._api_async.set_sandbox_mode(True)
Copy link
Author

Choose a reason for hiding this comment

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

it seems that the delist endpoint is not available on the the testnet

but not sure if we could keep this since there are quite some cex's out there that allow papertrading

Copy link
Contributor

Choose a reason for hiding this comment

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

why don't you use the actual server instead of the testnet? FT has been discouraging the use of testnet because of the limited features in it.

Copy link
Author

Choose a reason for hiding this comment

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

the problem that I'm having is that the delist endpoint is actually an authenticated endpoint

In dry_mode this will fail since we are not using the authentication there

Copy link
Member

Choose a reason for hiding this comment

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

it's not the first (and won't be the last) feature that ain't fully supported in dry-run mode.

it's also really a topic of "it's an authenticated endpoint on binance" - if you look at the same / corresponding endpoint on bybit - then it's not authenticated.

@xmatthias xmatthias linked an issue Mar 23, 2024 that may be closed by this pull request


@retrier
@cached(cache=TTLCache(maxsize=100, ttl=10), lock=Lock())
Copy link
Member

Choose a reason for hiding this comment

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

you should take your pick, really, when it comes to caching.

actual caching is applied below (in the get_spot_pair_delist_time().
applying caching twice can be dangerous here - as then you risk having the TTL applied twice in unfortunate cases.

That decorator is also something we're not using anywhere else - as it won't allow you to work around the cache and force a refresh - which can be necessary in some instances.

def get_deslist_schedule(self):
try:

delist_schedule = self._api.sapi_get_spot_delist_schedule()
Copy link
Member

Choose a reason for hiding this comment

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

considering that this is a private (direct) endpoint - i think we should have a form to unify it. Usually, that's taken care of by ccxt, but in this case, that unification / abstraction isn't available (yet?).

i know that for example bybit has a similar endpoint (we need to parse the text to make that work) - but i think what the exchange returns should be unified across exchanges (across the exchanges that support it, anyway) - so it's easier to use from a consumer side (not necessarily the strategy - anything outside of the exchange section should be seen as a consumer of this function).

cache = self._spot_delist_schedule_cache
lock = self._spot_delist_schedule_cache_lock

schedule_pair = pair.replace('/', '')
Copy link
Member

Choose a reason for hiding this comment

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

ccxt's .markets structure should have everything we need to convert symbol to pair (or viceversa).
i also think that the endpoint should be casting to pair already - so we get "DERP/BTC" out of the call - not DERPBTC. simply removing "/" is rather prone to errors in my experience.

it's often best to look at ccxt code on how they do it internally...

@jakubikan
Copy link
Author

Hey Guys thanks for the input - I will try to continue with it after the east holiday.
since I have just some limited time online.
Happy Easter 🐰 😄

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.

ExchangeDelistFilter Feature
3 participants