[Feat] add forex

[tanut32039]

Implementation details

• wrap up forex config to compatible with Bothan

Please ensure the following requirements are met before submitting a pull request:

• The pull request is targeted against the correct target branch
• The pull request is linked to an issue with appropriate discussion and an accepted design OR is linked to a spec that describes the work.
• The pull request includes a description of the implementation/work done in detail.
• The pull request includes any appropriate unit/integration tests
• You have added a relevant changelog entry to CHANGELOG.md
• You have re-reviewed the files affected by the pull request (e.g. using the Files changed tab in the GitHub PR explorer)

[Copilot]

Pull request overview

Adds initial forex support by generalizing the existing crypto-only asset info manager into a combined AssetInfoManager, introducing forex worker plumbing and API/server configuration to enable forex sources.

Changes:

• Refactors CryptoAssetInfoManager into AssetInfoManager with separate crypto/forex worker opts and worker pools.
• Introduces AssetType and updates signal-id batching to filter by asset type (crypto vs forex).
• Adds forex configuration to the API server (config structs + CLI wiring + example config) and creates new asset_info::price module/error types.

Reviewed changes

Copilot reviewed 21 out of 26 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
bothan-core/src/manager/asset_info/types.rs	Adds AssetType and renames manager info struct for generalized asset manager.
bothan-core/src/manager/asset_info/signal_ids.rs	Adds asset-type filtering to batched query-id mapping and updates tests.
bothan-core/src/manager/asset_info/price/tasks.rs	Updates imports to new generalized asset_info::price module paths.
bothan-core/src/manager/asset_info/price/error.rs	Adds shared price computation error types under asset_info::price.
bothan-core/src/manager/asset_info/price/cache.rs	Updates PriceState import to generalized module path.
bothan-core/src/manager/asset_info/price.rs	Adds asset_info::price module surface.
bothan-core/src/manager/asset_info/manager.rs	Generalizes manager to handle both crypto and forex workers; rebuild logic updated.
bothan-core/src/manager/asset_info/forex/worker/opts.rs	Adds forex worker options enum.
bothan-core/src/manager/asset_info/forex/worker.rs	Adds forex worker wrapper and worker builder using asset-type batching.
bothan-core/src/manager/asset_info/forex.rs	Exposes forex module.
bothan-core/src/manager/asset_info/error.rs	Adds generalized manager error types (registry/monitoring/heartbeat).
bothan-core/src/manager/asset_info/crypto/worker/opts.rs	Moves crypto worker opts under new module layout.
bothan-core/src/manager/asset_info/crypto/worker.rs	Updates crypto worker builder to use asset-type batching.
bothan-core/src/manager/asset_info/crypto.rs	Exposes crypto module.
bothan-core/src/manager/asset_info.rs	Updates public exports to new module layout and manager name.
bothan-core/src/manager.rs	Re-exports AssetInfoManager and replaces old module name.
bothan-core/src/ipfs/client.rs	Makes IpfsClient clonable.
bothan-core/Cargo.toml	Adds strum_macros dependency for AssetType string prefixing.
bothan-api/server/src/config/manager/forex_info/sources.rs	Adds forex source config for Band kiwi2.
bothan-api/server/src/config/manager/forex_info.rs	Adds forex manager config (sources + staleness threshold).
bothan-api/server/src/config/manager.rs	Adds forex manager config to aggregated manager config.
bothan-api/server/src/api/utils.rs	Updates PriceState import to new generalized module path.
bothan-api/server/src/api/server.rs	Switches server to use AssetInfoManager and new error module paths.
bothan-api/server-cli/src/commands/start.rs	Wires forex config into manager build and adds forex worker opts init.
bothan-api/server-cli/config.example.toml	Adds an example forex source config section.
Cargo.lock	Updates lockfile for strum_macros dependency/version.

Comments suppressed due to low confidence (4)

bothan-core/src/manager/asset_info/signal_ids.rs:107

• get_signal_ids_by_asset_type is new behavior but the unit test only covers the crypto case (and doesn't assert that non-matching asset types are excluded). Add a test registry containing both CS: and FS: IDs and assert filtering works for both AssetType::Crypto and AssetType::Forex.
  bothan-core/src/manager/asset_info/manager.rs:296
• set_registry_from_ipfs holds the Tokio Mutex guard across multiple .await points (sleep and build_crypto_workers). Holding an async mutex across awaits can block other tasks (and risks deadlocks/latency spikes). Drop the lock before awaiting (e.g., clear in a short scope, then rebuild, then reacquire to replace the vector).
  bothan-core/src/manager/asset_info/manager.rs:306
• Same issue for forex workers: the forex_workers mutex guard is held across sleep(..).await and build_forex_workers(..).await. Release the lock before awaiting and reacquire only to swap in the rebuilt workers.
  bothan-api/server-cli/src/commands/start.rs:220
• ForexInfoManagerConfig defines its own stale_threshold, but the server initialization always reads config.manager.crypto.stale_threshold and passes that single value into AssetInfoManager::build. Either wire up the forex stale threshold too (e.g., separate thresholds in AssetInfoManager) or remove the unused forex field to avoid a configuration setting that has no effect.

    let stale_threshold = config.manager.crypto.stale_threshold;
    let bothan_version =
        Version::from_str(VERSION).with_context(|| "Failed to parse bothan version")?;
    let registry_version_requirement = VersionReq::from_str(REGISTRY_REQUIREMENT)
        .with_context(|| "Failed to parse registry version requirement")?;

    let crypto_opts = match init_crypto_opts(&config.manager.crypto.source).await {
        Ok(workers) => workers,
        Err(e) => {
            bail!("failed to initialize workers: {:?}", e);
        }
    };

    let forex_opts = match init_forex_opts(&config.manager.forex.source).await {
        Ok(workers) => workers,
        Err(e) => {
            bail!("failed to initialize workers: {:?}", e);
        }
    };

    let manager = match AssetInfoManager::build(
        store,
        crypto_opts,
        forex_opts,
        ipfs_client,
        stale_threshold,
        bothan_version,
        registry_version_requirement,
        monitoring_client,

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 26 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

bothan-core/src/manager/asset_info/manager.rs:296

• set_registry_from_ipfs holds crypto_workers mutex while sleeping and while awaiting build_crypto_workers(...). This can block other tasks that need the mutex for an extended period. Consider dropping the lock before sleep/await (e.g., clear under lock, release, build workers, then re-lock briefly to swap in the new vector).
  bothan-core/src/manager/asset_info/manager.rs:306
• Same issue for forex_workers: the mutex is held across sleep and the await on build_forex_workers(...), increasing lock contention and potentially stalling unrelated operations. Clear under lock, drop the guard, build outside the lock, then re-lock to assign.
  bothan-core/src/manager/asset_info/signal_ids.rs:136
• get_source_batched_query_ids now filters by AssetType, but the test suite here only covers AssetType::Crypto. Please add a test that includes a mix of CS: and FS: signal IDs and asserts that only the selected asset type's signals (and their routed dependencies, if applicable) are included in the batching result.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 30 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (6)

bothan-core/src/manager/asset_info.rs:11

• This module-level documentation still says it manages crypto asset information, but the module now includes forex (pub mod forex) and exposes the renamed AssetInfoManager. Please update the doc comments to reflect the expanded scope.
  bothan-core/src/manager/asset_info/types.rs:31
• The top-level doc comment says this file contains types/constants for the crypto asset info manager, but it now introduces AssetType (Crypto/Forex) and AssetManagerInfo used by the unified asset manager. Please update the module docs to avoid misleading readers.
  bothan-core/src/manager/asset_info/price/tasks.rs:24
• The module header docs still refer to the crypto asset info manager, but this task code is now under asset_info::price and shared by the unified AssetInfoManager. Please update the doc comment to avoid implying this is crypto-only.
  bothan-core/src/manager/asset_info/manager.rs:296
• In set_registry_from_ipfs, the worker mutex is held across sleep() and the subsequent (potentially slow) build_*_workers calls. This blocks concurrent reads (e.g., price queries / info) for at least 1s + build time. Consider clearing under the lock, then dropping the guard before waiting/building, and finally re-locking only to swap in the newly built workers.
  bothan-core/src/manager/asset_info/signal_ids.rs:68
• The new asset_type filtering changes batching behavior, but the unit test only covers AssetType::Crypto. Please add a test case for AssetType::Forex (and ideally one where a forex signal routes to another forex signal) to ensure the prefix filter works as intended.
  bothan-core/src/manager/asset_info/manager.rs:44
• The module/file-level docs still refer to the old CryptoAssetInfoManager and only crypto assets, but this file now defines AssetInfoManager and also manages forex workers. Please update the top-level documentation to match the renamed struct and broadened scope.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 30 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (4)

bothan-core/src/manager/asset_info.rs:3

• Header comment still says this module manages "crypto asset information", but it now contains both crypto and forex submodules and re-exports AssetInfoManager. Updating the module docs will keep generated docs accurate.
  bothan-core/src/manager/asset_info/manager.rs:296
• set_registry_from_ipfs holds the crypto_workers mutex guard across sleep(...).await and build_crypto_workers(...).await. Holding a Tokio mutex across awaits can block other tasks from reading workers for an extended time (and worker builds may take significant time). Consider dropping the lock before awaiting (e.g., clear under lock, drop guard, wait/build, then re-lock briefly to swap in the new worker vec).
  bothan-core/src/manager/asset_info/manager.rs:306
• Same issue for forex_workers: the mutex guard is held while awaiting sleep and build_forex_workers. This can stall concurrent reads/updates and increases risk of contention. Clear under lock, release, then rebuild and swap under a short re-lock.
  bothan-core/src/manager/asset_info/signal_ids.rs:68
• get_source_batched_query_ids now filters registry signal IDs by asset_type, but the unit tests only exercise AssetType::Crypto and don’t validate the filtering behavior (e.g., that AssetType::Forex ignores CS: signals and vice versa). Add tests that include a mixed registry (both CS: and FS: entries) and assert the correct sources/queries are selected for each asset type.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 27 out of 32 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

bothan-core/src/manager/asset_info/price/tasks.rs:424

• In this test, prefix_stale_thresholds is populated with current_timestamp, but in production it’s treated as a threshold in seconds (see get_stale_cutoff: current_time - threshold). This makes the test validate a different contract (effectively passing a cutoff-like value) and can mask real staleness behavior; consider making mock AssetInfo.timestamp values relative to now and passing an actual threshold (e.g. 300) instead.
  bothan-core/src/manager/asset_info/price/tasks.rs:510
• Same issue as above: this test feeds prefix_stale_thresholds with an absolute timestamp, but get_stale_cutoff interprets the map values as thresholds (seconds). Consider setting mock AssetInfo.timestamp values near now and using a threshold duration to ensure the test reflects production semantics.
  bothan-core/src/manager/asset_info/price/tasks.rs:597
• This test sets the CS entry in prefix_stale_thresholds to current_timestamp - 10000, which again treats the value as an absolute cutoff rather than a threshold (seconds) as used by get_stale_cutoff. Updating the mock timestamps to be relative to now and passing an actual threshold would make the test match the production contract and avoid time-dependent behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 31 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (4)

bothan-core/src/manager/asset_info.rs:3

• asset_info is now the unified manager, but the module docs still describe only "crypto asset info". Please update the doc comment to reflect the expanded scope (crypto + forex) and the AssetInfoManager naming.
  bothan-core/src/manager/asset_info/price/tasks.rs:148
• Error::InvalidSignal is returned here for both (1) missing signals and (2) signals whose prefix isn’t present in prefix_stale_thresholds. That makes the error message ("Signal does not exist") inaccurate and can mark valid signals as Unsupported due to configuration. Consider introducing a distinct error for missing prefix thresholds (or treating it as Unavailable).
  bothan-core/src/manager/asset_info/price/tasks.rs:6
• The module-level docs still say "crypto asset info manager" even though this module now computes prices for the unified asset_info manager (crypto + forex). Please update the doc comment to avoid misleading documentation.
  bothan-core/src/manager/asset_info/price/tasks.rs:425
• These tests populate prefix_stale_thresholds with epoch timestamps (e.g., Utc::now().timestamp()), but production code treats the map values as stale thresholds (durations in seconds) that get subtracted from current_time. This makes the tests misleading and risks masking staleness bugs; consider using a real threshold value (e.g., 300) and setting AssetInfo.timestamp relative to current_time, or refactor get_signal_price_states to accept an injected current_time for deterministic tests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 31 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (8)

bothan-core/src/manager/asset_info/price/tasks.rs:511

• Same issue as above: prefix_stale_thresholds should contain threshold seconds, but the test inserts current_timestamp. This makes staleness checks effectively meaningless with the current hard-coded AssetInfo timestamps. Please adjust the test to use realistic timestamps (relative to now) and a real threshold value (seconds).
  bothan-core/src/manager/asset_info/manager.rs:16
• The file-level/module docs at the top of this file still refer to CryptoAssetInfoManager / "crypto asset information". Since this implementation is now AssetInfoManager and is used for both crypto + forex flows, please update the header docs to match the renamed type and expanded scope (otherwise rustdoc will be misleading).
  bothan-core/src/manager/asset_info/price/tasks.rs:11
• The module docs at the top of this file still say "Price computation tasks for the crypto asset info manager" even though the module now lives under asset_info and is used by the unified manager. Please update the top-level docs to match the current naming/scope.
  bothan-core/src/manager/asset_info.rs:3
• Module docs still refer to "crypto asset info manager" even though this module now appears to cover both crypto + forex and exports AssetInfoManager. Please update the top-level docs to match the renamed module and its broader scope to avoid misleading rustdoc output.
  bothan-core/src/manager/asset_info/price/tasks.rs:112
• compute_signal_result treats a missing stale-threshold entry (i.e., get_stale_cutoff returning None) the same as a missing signal, returning Error::InvalidSignal. This will silently mark signals as Unsupported if prefix_stale_thresholds is missing a prefix, and the error message ("Signal does not exist") becomes misleading. Consider either (1) providing a default threshold, (2) returning a dedicated configuration error, or (3) validating prefix_stale_thresholds up-front and failing fast.
  bothan-core/src/manager/asset_info/price/tasks.rs:425
• These tests populate prefix_stale_thresholds with current_timestamp, but the production config passes threshold seconds (e.g. 300/3600), not an epoch timestamp. Using an epoch timestamp here makes stale_cutoff ~0 and keeps the old hard-coded AssetInfo timestamps (11000, 11001, ...) artificially "fresh", so the test no longer validates the intended staleness behavior. Update the test data to use timestamps relative to now and set the map values to threshold seconds.
  bothan-core/src/manager/asset_info/price/tasks.rs:598
• This test inserts current_timestamp - 10000 into prefix_stale_thresholds, but the map is expected to hold a threshold duration in seconds (e.g. 10_000), not an epoch timestamp. With the current code, this produces a stale_cutoff around 10_000 (seconds since epoch) and will keep the hard-coded AssetInfo timestamps (~11_000) fresh. Please change the inserted value to the intended threshold seconds and update the mocked AssetInfo.timestamp values to be relative to now so the test actually exercises stale vs fresh behavior.
  bothan-core/src/manager/asset_info/types.rs:23
• Now that this type is AssetManagerInfo (not CryptoAssetManagerInfo) and the manager appears to handle more than crypto, the module-level docs at the top of this file should be updated to reflect the new naming/scope.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 24 out of 29 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

bothan-core/src/manager/asset_info/price/tasks.rs:148

• compute_signal_result now treats a missing prefix_stale_thresholds entry the same as a missing signal (both map to Error::InvalidSignal). This will mark valid signals as Unsupported if the prefix isn't configured, and the resulting logs/error message are misleading. Consider returning a dedicated error (e.g., missing staleness config for prefix) and handling it separately (likely as Unavailable/config error) rather than conflating it with an unknown signal.
  bothan-core/src/manager/asset_info/price/tasks.rs:425
• The prefix_stale_thresholds map is meant to hold thresholds (seconds), but this test populates it with an absolute timestamp (current_timestamp). That makes the test pass while exercising a different semantics than production. Prefer inserting a real threshold (e.g., 0/3600) and set AssetInfo.timestamp values relative to a deterministic current_time used by the test.
  bothan-core/src/manager/asset_info/price/tasks.rs:511
• Same issue as above: this test sets prefix_stale_thresholds["CS"] to an absolute timestamp (current_timestamp) even though the code interprets the value as a threshold duration in seconds. Update the test data to use an actual threshold value so the behavior under test matches production.
  bothan-core/src/manager/asset_info/price/tasks.rs:598
• This test appears to be trying to emulate the old stale_cutoff behavior, but prefix_stale_thresholds values are thresholds (seconds) and should not be set to current_timestamp - 10000. With the current code, that makes the computed cutoff depend on wall-clock time. Use a fixed threshold (e.g., 10000) and control current_time/AssetInfo.timestamp deterministically in the test.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 24 out of 29 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

bothan-core/src/manager/asset_info/manager.rs:14

• AssetWorker as AssetWorkerTrait is imported but not used anywhere in this module. CI runs clippy with RUSTFLAGS=-D warnings, so this unused import will fail the build; remove the import (or use it if intended).
  bothan-core/src/manager/asset_info/price/tasks.rs:160
• get_stale_cutoff() returns None when the signal id’s prefix isn’t in prefix_stale_thresholds (and also for valid unprefixed IDs like BTC-USD). That causes compute_signal_result to return Error::InvalidSignal even when registry.get(id) is Some(_), which is a behavioral regression and makes diagnosis difficult. Consider adding a fallback/default threshold and/or returning a distinct error for missing prefix threshold, and handle unprefixed IDs explicitly.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 30 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (7)

bothan-core/src/manager/asset_info.rs:3

• The module-level docs still describe this as a crypto asset info manager, but the PR expands this module to cover crypto + forex. Update the docs to reflect the broader scope so public API documentation stays accurate.
  bothan-core/src/manager/asset_info/price/tasks.rs:424
• In this test, prefix_stale_thresholds is intended to hold stale thresholds (seconds) but the test inserts current_timestamp (an absolute epoch time). This makes the computed cutoff ~0 and can mask staleness-related regressions. Consider using realistic epoch timestamps for AssetInfo.timestamp (e.g., near current_timestamp) and setting the threshold to a small value (e.g., 0/300) to exercise the intended behavior.
  bothan-core/src/manager/asset_info/types.rs:25
• The module-level docs at the top of this file still describe it as the crypto asset info manager, but AssetManagerInfo is now part of the combined asset (crypto + forex) manager API. Update the module doc comment to reflect the new scope/naming.
  bothan-core/src/manager/asset_info/price/tasks.rs:6
• The top-level doc comment for this module still refers to the “crypto asset info manager”, but this code now lives under asset_info and is used by the combined asset (crypto + forex) manager. Please update the doc wording to avoid misleading docs.
  bothan-core/src/manager/asset_info/manager.rs:16
• The file-level //! docs at the top still reference CryptoAssetInfoManager / “crypto asset info manager”, but the implementation is now AssetInfoManager and supports both crypto + forex. Please update the top-level doc comment to avoid referencing a non-existent type and to reflect the new scope.
  bothan-core/src/manager/asset_info/price/tasks.rs:510
• Same issue as above: this test sets the per-prefix threshold to current_timestamp (absolute time) rather than a duration in seconds, which makes the cutoff ~0 and reduces the test’s ability to catch staleness bugs. Prefer keeping thresholds as durations and adjusting mock AssetInfo.timestamp values accordingly.
  bothan-core/src/manager/asset_info/price/tasks.rs:597
• Same issue as above: prefix_stale_thresholds is documented/used as a duration map (seconds), but this test populates it with an absolute timestamp (current_timestamp - 10000). This makes the cutoff calculation hard to reason about and can hide issues. Prefer setting the threshold to a duration (e.g., 10_000) and using epoch-like AssetInfo.timestamp values.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.