Skip to content

Change enabled gating to available for currencies and assets in wrappers & friends#2183

Draft
gloriousCode wants to merge 7 commits intothrasher-corp:masterfrom
gloriousCode:r-u-available-bb
Draft

Change enabled gating to available for currencies and assets in wrappers & friends#2183
gloriousCode wants to merge 7 commits intothrasher-corp:masterfrom
gloriousCode:r-u-available-bb

Conversation

@gloriousCode
Copy link
Collaborator

PR Description

image

This is a more thorough version of #2159. If this is to be done, its to be done in a single PR with minimal changes.

Changes

  • Replace currency enabled checks in various forms with available checks
  • Replace asset enabled checks in various forms with available checks
  • Removes ClassificationError because it should go
  • Moves ticker processing to {{exchange}}_websocket.go
  • Minimises some work within websocketroutine_manager.go to spend less time processing
  • RPC server is more lenient for get requests only. Anything like submitorder will continue to be for enabled only
    • This is an optional change
  • Fixes an old engine data race on shutdown

What stays

  • subscription generation is based on enabled pairs. There are already subscription hooks and other ways to generate subscriptions. I do not believe there is value in auto-generating subscriptions and subscribing to them if you haven't enabled them
  • engine sync_manager uses enabled pairs
  • engine order_manager uses enabled pairs
  • engine helpers uses enabled pairs

Type of change

  • New feature (non-breaking change which adds functionality)

Changes enabled gating and pushes it to `/engine`
Some areas such as subscription generation still require enabled
to not blow out websockets when undesired
Updates websocketroutine_manager to move ticker processing
to exchange websockets
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request is a significant refactoring that changes the gating for many data retrieval functions from checking for "enabled" currencies and assets to "available". This makes the system more flexible for querying data. The PR also includes important fixes for data races in engine/exchange_manager and engine/sync_manager, and refactors ticker processing to be more modular.

My review focuses on the correctness and potential side effects of these widespread changes. While most changes look good and are consistent with the PR's goal, I've identified a potential issue where CancelAllOrders in several exchange wrappers now iterates over all available pairs instead of enabled pairs. This could lead to unexpected behavior and a high volume of API requests. This seems to contradict the PR's description that write operations should remain strict.

case asset.CoinMarginedFutures:
if req.Pair.IsEmpty() {
enabledPairs, err := e.GetEnabledPairs(asset.CoinMarginedFutures)
availablePairs, err := e.GetAvailablePairs(asset.CoinMarginedFutures)

Choose a reason for hiding this comment

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

high

This change from GetEnabledPairs to GetAvailablePairs means that calling CancelAllOrders for an asset will now attempt to cancel orders for all pairs available on the exchange, not just the ones enabled in the user's configuration. This could be unexpected and lead to a large number of API requests, potentially causing rate limiting. The PR description states that write operations should remain strict ('for enabled only'). Should this use GetEnabledPairs instead?

Suggested change
availablePairs, err := e.GetAvailablePairs(asset.CoinMarginedFutures)
availablePairs, err := e.GetEnabledPairs(asset.CoinMarginedFutures)

case asset.USDTMarginedFutures:
if req.Pair.IsEmpty() {
enabledPairs, err := e.GetEnabledPairs(asset.USDTMarginedFutures)
availablePairs, err := e.GetAvailablePairs(asset.USDTMarginedFutures)

Choose a reason for hiding this comment

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

high

This change from GetEnabledPairs to GetAvailablePairs means that calling CancelAllOrders for an asset will now attempt to cancel orders for all pairs available on the exchange, not just the ones enabled in the user's configuration. This could be unexpected and lead to a large number of API requests, potentially causing rate limiting. The PR description states that write operations should remain strict ('for enabled only'). Should this use GetEnabledPairs instead?

Suggested change
availablePairs, err := e.GetAvailablePairs(asset.USDTMarginedFutures)
availablePairs, err := e.GetEnabledPairs(asset.USDTMarginedFutures)


var allOrders []OrderData
currs, err := e.GetEnabledPairs(asset.Spot)
currs, err := e.GetAvailablePairs(asset.Spot)

Choose a reason for hiding this comment

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

high

This change from GetEnabledPairs to GetAvailablePairs means that calling CancelAllOrders will now attempt to cancel orders for all pairs available on the exchange, not just the ones enabled in the user's configuration. This could be unexpected and lead to a large number of API requests, potentially causing rate limiting. The PR description states that write operations should remain strict ('for enabled only'). Should this use GetEnabledPairs instead?

Suggested change
currs, err := e.GetAvailablePairs(asset.Spot)
currs, err := e.GetEnabledPairs(asset.Spot)

switch orderCancellation.AssetType {
case asset.Spot:
enabledPairs, err := e.GetEnabledPairs(asset.Spot)
availablePairs, err := e.GetAvailablePairs(asset.Spot)

Choose a reason for hiding this comment

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

high

This change from GetEnabledPairs to GetAvailablePairs means that calling CancelAllOrders for an asset will now attempt to cancel orders for all pairs available on the exchange, not just the ones enabled in the user's configuration. This could be unexpected and lead to a large number of API requests, potentially causing rate limiting. The PR description states that write operations should remain strict ('for enabled only'). Should this use GetEnabledPairs instead?

Suggested change
availablePairs, err := e.GetAvailablePairs(asset.Spot)
availablePairs, err := e.GetEnabledPairs(asset.Spot)

// getAllOpenOrderID returns all open orders by currency pairs
func (e *Exchange) getAllOpenOrderID(ctx context.Context) (map[string][]string, error) {
allPairs, err := e.GetEnabledPairs(asset.Spot)
allPairs, err := e.GetAvailablePairs(asset.Spot)

Choose a reason for hiding this comment

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

high

This change from GetEnabledPairs to GetAvailablePairs means that getAllOpenOrderID (and by extension CancelAllOrders) will now operate on all pairs available on the exchange, not just the ones enabled in the user's configuration. This could be unexpected and lead to a large number of API requests, potentially causing rate limiting. The PR description states that write operations should remain strict ('for enabled only'). Should this use GetEnabledPairs instead?

Suggested change
allPairs, err := e.GetAvailablePairs(asset.Spot)
allPairs, err := e.GetEnabledPairs(asset.Spot)

}

enabledPairs, err := e.GetEnabledPairs(asset.Spot)
availablePairs, err := e.GetAvailablePairs(asset.Spot)

Choose a reason for hiding this comment

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

high

This change from GetEnabledPairs to GetAvailablePairs means that calling CancelAllOrders will now attempt to cancel orders for all pairs available on the exchange, not just the ones enabled in the user's configuration. This could be unexpected and lead to a large number of API requests, potentially causing rate limiting. The PR description states that write operations should remain strict ('for enabled only'). Should this use GetEnabledPairs instead?

Suggested change
availablePairs, err := e.GetAvailablePairs(asset.Spot)
availablePairs, err := e.GetEnabledPairs(asset.Spot)

}
if len(req.Pairs) == 0 {
allPairs, err := e.GetEnabledPairs(asset.Spot)
allPairs, err := e.GetAvailablePairs(asset.Spot)

Choose a reason for hiding this comment

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

medium

This change from GetEnabledPairs to GetAvailablePairs means that calling GetActiveOrders without specifying pairs will now fetch orders for all pairs available on the exchange, not just the ones enabled in the user's configuration. While this is a 'get' request, it could still result in a large number of API requests. Is this the intended behavior?

@thrasher-
Copy link
Collaborator

@codex please review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e74f6a9c53

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +338 to 339
if b.Config.CurrencyPairs.IsAssetAvailable(a) == nil {
enabledAsset = true

Choose a reason for hiding this comment

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

P1 Badge Preserve disabled-asset state when applying config pairs

In SetConfigPairs, enabledAsset is now derived from Config.CurrencyPairs.IsAssetAvailable(a) while iterating assetTypes := Config.CurrencyPairs.GetAssetTypes(false), so this condition is always true for every configured asset. As a result, SetAssetEnabled(a, enabledAsset) re-enables assets that were intentionally disabled in config, which breaks asset-level disablement after bootstrap/reload.

Useful? React with 👍 / 👎.

Comment on lines 222 to +225
case []ticker.Price:
if !m.syncer.IsRunning() {
return nil
}

Choose a reason for hiding this comment

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

P2 Badge Keep ticker cache updates for websocket ticker batches

The websocket routine no longer runs ticker.ProcessTicker for incoming *ticker.Price/[]ticker.Price, and now only performs sync bookkeeping here. That regression affects feeds that still emit raw ticker structs without pre-processing (notably GateIO option/futures websocket handlers), so those websocket ticker updates no longer refresh the global ticker cache and downstream GetTicker consumers can observe stale values.

Useful? React with 👍 / 👎.

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.

2 participants