Exchange rate + update dependencies

[dewabisma]

Summary

• add exchange rate endpoint
• update dependencies qp and sp

[n13]

I now have a complete picture of the PR. Here's my review:

---

PR #68 review — Exchange rate endpoint + dependency bumps

Great little feature, PR #68 cleanly adds a GET /api/exchange-rate endpoint backed by exchangerate-api.com with snapshot caching, plus a sweep of qp-* / sp-* / subxt / rusx dep bumps. CI is green (49m41s) and the integration follows the existing RiskCheckerService pattern nicely. Below are concrete suggestions, ordered by importance.

1. Security: API key can leak through error messages [must fix]

The provider puts the API key in the URL path (https://v6.exchangerate-api.com/v6/{api_key}/latest/USD). reqwest::Error::Display frequently includes the request URL (especially for connect errors, redirects, timeouts), and we return that string straight to the HTTP client:

fn map_exchange_rate_error(err: ExchangeRateError) -> (StatusCode, String) {
    match err {
        ExchangeRateError::Api(err)  => (StatusCode::BAD_REQUEST, err),
        ExchangeRateError::Http(err) => (StatusCode::INTERNAL_SERVER_ERROR, err.to_string()),
        ExchangeRateError::Json(err) => (StatusCode::INTERNAL_SERVER_ERROR, err.to_string()),
    }
}

This can leak the key to any external caller who provokes a network error. Also, BAD_REQUEST is wrong — the client made a perfectly fine request, the upstream API failed. Suggestion (mirrors map_risk_checker_error):

fn map_exchange_rate_error(err: ExchangeRateError) -> (StatusCode, String) {
    tracing::error!("Exchange rate error: {}", err);
    match err {
        ExchangeRateError::Api(_)  => (StatusCode::BAD_GATEWAY,
            "Exchange rate provider returned an error".to_string()),
        ExchangeRateError::Http(_) => (StatusCode::BAD_GATEWAY,
            "Failed to fetch exchange rates".to_string()),
        ExchangeRateError::Json(_) => (StatusCode::BAD_GATEWAY,
            "Failed to parse exchange rate response".to_string()),
    }
}

Adding a Display sanitiser (reqwest::Error::without_url) before logging is also nice belt-and-braces.

2. Silent fallbacks violate the "fail early" rule [must fix]

Three spots in src/services/exchange_rate_service.rs swallow errors and substitute defaults:

let client = reqwest::Client::builder()
    .timeout(std::time::Duration::from_secs(30))
    .build()
    .unwrap_or_else(|_| reqwest::Client::new());        // (A) loses the timeout

time_next_update_unix: i64::try_from(time_next).unwrap_or(i64::MAX),  // (B) cache "never expires"

fn now_unix_seconds() -> i64 {
    SystemTime::now().duration_since(UNIX_EPOCH)
        .map(|d| d.as_secs() as i64)
        .unwrap_or(0)                                   // (C) cache always stale
}

RiskCheckerService::new already shows the right pattern for (A):

        let client = Client::builder()
            .timeout(Duration::from_secs(30))
            .build()
            .expect("TLS backend should be initialized, or the resolver should load the system configuration.");

For (B) and (C), at minimum log a warning; ideally surface them as ExchangeRateError::Api(...).

3. DRY: new_test duplicates new's client construction

#[cfg(test)]
impl ExchangeRateService {
    fn new_test(base_url: String) -> Self {
        let client = reqwest::Client::builder()
            .timeout(std::time::Duration::from_secs(30))
            .build()
            .unwrap_or_else(|_| reqwest::Client::new());   // <-- copy of new()
        Self { client, base_url, cache: Arc::new(Mutex::new(HashMap::new())), base_currency: "USD".to_string() }
    }
}

Extract a fn build_client() -> reqwest::Client (or just have new_test set base_url after calling new("test-key")).

4. Cache mutex is held across the network call [should fix]

pub async fn get_snapshot(&self) -> Result<ExchangeRateSnapshot, ExchangeRateError> {
    let base = normalize_currency_code(&self.base_currency);
    let mut guard = self.cache.lock().await;
    if let Some(s) = guard.get(&base) {
        if cache_is_fresh(s) { return Ok(s.clone()); }
    }
    let snapshot = self.fetch_latest(&base).await?;     // HTTP call WHILE holding the mutex
    guard.insert(base, snapshot.clone());
    Ok(snapshot)
}

While this does coalesce concurrent fetches (a feature), it also blocks fast cache reads whenever a fetch is in flight — up to the 30 s timeout. Two cleaner options:

• Arc<RwLock<Option<ExchangeRateSnapshot>>> — fast reads, brief write critical section
• tokio::sync::OnceCell per base currency for true single-flight semantics

Since base_currency is hardcoded to "USD" and never varies, even an Arc<RwLock<Option<ExchangeRateSnapshot>>> would be simpler than the current HashMap<String, _> (see point 6).

5. Handler drops the freshness signal

pub async fn handle_get_exchange_rate(
    State(state): State<AppState>,
) -> Result<Json<SuccessResponse<serde_json::Value>>, AppError> {
    let exchange_rate = state.exchange_rate_service.get_snapshot().await?;
    Ok(SuccessResponse::new(json!(exchange_rate.conversion_rates)))
}

The frontend gets no time_next_update_unix and no base_code, so it can't avoid hammering this endpoint. Adding #[derive(Serialize)] to ExchangeRateSnapshot and returning the whole snapshot (or a typed DTO) is one extra line and far more useful. Also avoids the serde_json::Value round-trip.

6. HashMap-keyed cache is YAGNI for a single hardcoded base

base_currency: "USD" is hardcoded, not config-driven, and there's no API to change it. The whole HashMap<String, ExchangeRateSnapshot> + normalize_currency_code machinery is dead weight today. Either:

• Wire base_currency through ExchangeRateConfig and accept it as a query param, or
• Simplify the cache to Arc<RwLock<Option<ExchangeRateSnapshot>>>

7. Scope check: wordlist re-curation is hidden in here

The PR title says "Exchange rate + update dependencies", but final_wordlist.txt is also a rename from crypto_checksum_bip39.txt plus a substantial vocabulary re-curation (drop "abandon"/"weapon"/"violent"/"zero", add "abundance"/"merkle"/"hodl"/"nft"/"rollup", etc.). It's almost certainly a side-effect of the qp-human-checkphrase bump to v2.0.1 (the API also changed: load_bip39_list → load_word_list), but it's worth a one-line note in the PR description so reviewers know to expect it. A few typos snuck in too, probably worth flagging upstream:

• benifits → benefits
• exellent → excellent
• supurb → superb
• undisputable → indisputable

Important call-out: any address whose referral code is regenerated after this lands will produce a different code than before, since the input vocabulary changed. Existing persisted codes are safe (they're stored in the DB), but any code that doesn't already exist in the DB will be drawn from the new vocabulary.

8. Test coverage [nice to have]

The three unit tests in exchange_rate_service.rs are clean and test the right things. One missing piece is a router-level smoke test — same shape as auth_challenge_and_verify_flow — that hits /api/exchange-rate end to end. The wiremock harness is already a dev-dep, so this should be straightforward.

9. Minor

• time_last_update_unix is #[allow(dead_code)] — drop it from the struct.
• ExchangeRateApiV6Response::result could be an enum (success | error) instead of a String comparison, but not blocking.
• auth.rs and signature_service.rs test changes (entropy → SensitiveBytes32, kp.sign(...).unwrap()) look like clean adaptations to the new dilithium 2.4.0 API. Nicely done.

What I'd merge today vs. block

Block until 1 (API key leak) and 2 (silent fallbacks, especially the unwrap_or_else(|_| reqwest::Client::new())) are fixed — both are quick changes and the first one is a real security concern.

Tackle in a follow-up PR: 4, 5, 6, 8 — they don't gate the feature.

Otherwise the structure (separate handler / route / service modules, ExchangeRateError plumbed through AppError, test config block, default fallback in Config::default) is exactly consistent with the existing codebase. Nice work overall.

[n13]

What is this word list change all about? Do we need this?

Also why is hodl and nft back in this list, that shouldn't be in there...

[dewabisma]

What is this word list change all about? Do we need this?

Also why is hodl and nft back in this list, that shouldn't be in there...

https://github.com/Quantus-Network/qp-human-checkphrase/blob/main/final_wordlist.txt

well it's there, in main branch

[n13]

The PR head hasn't moved since yesterday — still e1d2b9c from May 1 — but the upstream qp-human-checkphrase wordlist is now aligned with the PR's local copy, so the wordlist concern is resolved. The remaining code-level findings stand. Quick fresh pass:

Wordlist: now in sync with upstream

The PR's final_wordlist.txt is byte-identical to qp-human-checkphrase's main branch wordlist (just one trailing newline difference). Both have exactly 2048 words. Both contain hold/nautilus and exclude hodl/nft. Concern resolved — no maintenance divergence.

(Both still carry benifits / exellent / supurb / undisputable — purely cosmetic, but worth a one-line PR upstream when somebody has a spare minute.)

Code review: status unchanged since yesterday

The branch hasn't been updated, so the same picture as my prior re-review applies:

Still must-fix before merge

1. Silent failure in map_exchange_rate_error (src/errors.rs:209)

fn map_exchange_rate_error(err: ExchangeRateError) -> (StatusCode, String) {
    match err {
        ExchangeRateError::Api(_) | ExchangeRateError::Http(_) => {
            (StatusCode::BAD_GATEWAY, "Failed to fetch exchange rates".to_string())
        }
        ExchangeRateError::Json(_) => (
            StatusCode::INTERNAL_SERVER_ERROR,
            "Failed to parse exchange rate response".to_string(),
        ),
        ExchangeRateError::Cache(_) => (
            StatusCode::INTERNAL_SERVER_ERROR,
            "An internal server error occurred".to_string(),
        ),
    }
}

The inner error is dropped on the floor — no tracing::error!. When ops sees a 502 Failed to fetch exchange rates they'll have zero context (expired key? rate-limited? upstream down? JSON shape change?). Fix is one line at the top, mirroring map_risk_checker_error:

tracing::error!("Exchange rate error: {}", err);

2. DRY: new and new_test are byte-identical except base_url

    pub fn new(api_key: &str) -> Self {
        let base_url = format!("https://v6.exchangerate-api.com/v6/{}", api_key);
        let client = reqwest::Client::builder()
            .timeout(std::time::Duration::from_secs(30))
            .build()
            .expect("TLS backend should be initialized, or the resolver should load the system configuration.");

#[cfg(test)]
impl ExchangeRateService {
    fn new_test(base_url: String) -> Self {
        let client = reqwest::Client::builder()
            .timeout(std::time::Duration::from_secs(30))
            .build()
            .expect("TLS backend should be initialized, or the resolver should load the system configuration.");

Simplest fix: fn new_test(base_url: String) -> Self { let mut s = Self::new("test-key"); s.base_url = base_url; s }.

Still worth flagging (not blocking)

• Route was renamed /exchange-rate → /exchange-rates in the fix commit. Confirm the frontend agrees.
• time_last_update_unix: Option<u64> in ExchangeRateApiV6Response is #[allow(dead_code)] — drop it.

What I'd merge today

Once #1 lands the PR is mergeable in my book. #2 is a 30-second polish — would do it at the same time. Everything else (4, 5, 6, 8 from the original review) was follow-up material.

Want me to push those two fixes onto feat/exchange-rate-endpoint for you, or leave it for dewabisma?

Related context: Initial PR 68 review and yesterday's re-review.

[n13]

PR #68 — Updated review after de9397a

The two remaining must-fix items from the prior review are addressed in de9397a. Verified locally with cargo fmt --check, cargo clippy --all-targets --all-features -- -D warnings, and the three services::exchange_rate_service::tests::* (all green).

Resolved

#	Issue	Fix
1	Silent failure in map_exchange_rate_error	Each variant now logs via tracing::error! before returning a generic client message
1b	Potential API key leak in Http variant	reqwest::Error::without_url() strips the URL (which contains the key) before logging
2	DRY: new_test duplicated new's client construction	new_test now delegates to Self::new(\"test-key\") and overwrites base_url

fn map_exchange_rate_error(err: ExchangeRateError) -> (StatusCode, String) {
    match err {
        ExchangeRateError::Api(detail) => {
            tracing::error!(\"Exchange rate API error: {}\", detail);
            (StatusCode::BAD_GATEWAY, \"Failed to fetch exchange rates\".to_string())
        }
        ExchangeRateError::Http(e) => {
            tracing::error!(\"Exchange rate HTTP error: {}\", e.without_url());
            (StatusCode::BAD_GATEWAY, \"Failed to fetch exchange rates\".to_string())
        }
        // ...
    }
}

#[cfg(test)]
impl ExchangeRateService {
    fn new_test(base_url: String) -> Self {
        let mut service = Self::new(\"test-key\");
        service.base_url = base_url;
        service
    }
}

Status of the rest of the original review

#	Topic	Status
1	API key leak via error messages	Fixed in e1d2b9c, hardened further in de9397a
2	Silent fallbacks (unwrap_or_else, unwrap_or(i64::MAX), unwrap_or(0))	Fixed in e1d2b9c
3	DRY in new_test	Fixed in de9397a
4	Cache mutex held across HTTP call	Fixed in e1d2b9c (Arc<RwLock<Option<_>>>, guard scoped before .await)
5	Handler dropped freshness signal	Fixed in e1d2b9c (returns full ExchangeRateSnapshot)
6	HashMap-keyed cache YAGNI	Fixed in e1d2b9c (collapsed to Option<_>)
7	Wordlist concerns (hodl/nft)	Fixed in e1d2b9c; verified byte-identical to upstream qp-human-checkphrase@main
8	Router-level smoke test	Not done — fine as a follow-up
9a	time_last_update_unix #[allow(dead_code)]	Not done — trivial, leave or follow-up
9b	result: String could be enum	Not done — non-blocking

Verdict

LGTM, merging now. Outstanding items (router smoke test, dead-code field) are non-blocking and can be picked up in a follow-up. Nice work iterating on this.