Currecy/Asset: Environment variable to Ignore enabled currency/asset check/error#2171
Currecy/Asset: Environment variable to Ignore enabled currency/asset check/error#2171gloriousCode wants to merge 4 commits intothrasher-corp:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to bypass the enabled currency/asset checks via the IGNORE_ENABLED_CHECK environment variable. The implementation correctly applies this bypass logic across several functions in the currency package. My review focuses on a performance improvement opportunity and a bug in the accompanying test. Specifically, the function for checking the environment variable is inefficient and should be optimized to read the value only once. Additionally, the new test case has a case-sensitivity issue with the environment variable key that will cause it to fail on some operating systems.
|
@codex please review |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds an opt-in environment-variable bypass to allow wrapper/WebSocket paths to operate on supported-but-not-enabled assets/pairs by ignoring “enabled” checks at the currency/asset layer.
Changes:
- Introduces
IGNORE_ENABLED_CHECKenv var handling incurrency.PairsManagerto bypass asset/pair enabled gating. - Adjusts enabled/available checks (
GetAssetTypes,GetPairs,EnsureOnePairEnabled,IsPairAvailable,IsPairEnabled,IsAssetEnabled) to honor the bypass. - Adds a unit test covering the bypass behavior across several call paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
currency/manager.go |
Adds env-var driven bypass and updates multiple “enabled”/availability decision points accordingly. |
currency/manager_test.go |
Adds a test validating behavior with bypass off vs on. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8849809e52
ℹ️ 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".
|
So there is two competing philosophies here. PR #2159 (mine) — Remove the enabled-pairs filter from UpdateTickers wrapper functions entirely. Match against available pairs instead. The wrapper layer becomes agnostic — it fetches and stores everything the exchange supports, and filtering is left to higher layers (engine, strategy, CLI). This — Keep the existing enabled-pairs checks in place but add an IGNORE_ENABLED_CHECK env var that globally bypasses them when set to true. The wrapper layer keeps its filter but callers can opt out. From my perspective wrapper/exchange layer's job is to translate exchange API responses into internal types. Policy decisions about which pairs to surface belong in the application layer (engine, gRPC handlers, strategies). Embedding an "enabled" filter inside a REST-response-processing loop conflates data acquisition with business logic. #2159 pushes that boundary outward. Main isssues with this PR:
|
|
I agree this shouldn't be a PR. It was mostly to help you achieve your goals of not having filtering without requiring massive amounts of code changes and not needing to call "enable all pairs" every time a new pair is added |
PR Description
PRs like #2159 with messages like
Make me feel FEELINGS! I think it is totally the wrong direction to take. So rather than change every single wrapper function and every single websocket implementation, I thought this opt-in check would be a better way to address the issue.
Just add environment var
IGNORE_ENABLED_CHECK=true. It allows processing of non-enabled currencies and assets through wrapper and websocket functionsFixes # (issue)
Shazbert
New feature (non-breaking change which adds functionality)
How has this been tested
Feature on:
Config:

Feature off: