Skip to content

fix(yadio): canonicalise currency codes to uppercase #896

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.

Sign up for GitHub

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

Open
Arowolokehinde wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(yadio): canonicalise currency codes to uppercase #896

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd06535
fix(yadio): canonicalise currency codes to uppercase
Arowolokehinde Aug 19, 2026
58c4115
fix(yadio): canonicalise currency codes to uppercase
Arowolokehinde Aug 19, 2026
d8e93db
fix(yadio): address reviewer nits — add §6.6 comment and strengthen test
Arowolokehinde Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/price/providers/yadio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ impl YadioProvider {
.btc
.into_iter()
.filter_map(|(code, value)| match value {
Some(v) if v.is_finite() && v > 0.0 => Some((code, Quote::PerBtc(v))),
Some(v) if v.is_finite() && v > 0.0 => {
Some((code.to_uppercase(), Quote::PerBtc(v)))

@grunch grunch Aug 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🧹 Consistency & Maintainability | 🔵 Nit | ⚡ Quick win

Add the "why" comment — every sibling that canonicalises carries one, and Yadio is the case that most needs it.

The four adapters that already do this each explain themselves at the call site:

File Line Comment
blockchain.rs 53-54 // Codes arrive uppercase already; canonicalise anyway so the adapter honours §6.6 even if the API drifts.
coingecko.rs 89 // CoinGecko ships lowercase codes — canonicalise (§6.6).
currency_api.rs 67 // currency-api ships lowercase codes — canonicalise (§6.6).
yadio.rs 50-52 (none)

blockchain.rs is the exact analogue — upstream ships uppercase there too — and
#859 quoted its comment as the model for this fix precisely because, without it,
the next reader sees a to_uppercase() on data that is already uppercase and has
no way to tell it is deliberate rather than redundant. That is also what makes the
rustfmt-forced block form here read as arbitrary; the comment justifies the braces.

💡 Suggested change 
                Some(v) if v.is_finite() && v > 0.0 => {
                    // Yadio ships uppercase codes today; canonicalise anyway so
                    // the adapter honours §6.6 even if the API drifts.
                    Some((code.to_uppercase(), Quote::PerBtc(v)))
                }

The module header would carry it well too — coingecko.rs:5 and
currency_api.rs:15 both state the casing contract in //! docs.

@Arowolokehinde Arowolokehinde Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i just made the suggested changes
Thank you

@grunch grunch Aug 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🗄️ Data Integrity | 🟡 Minor | 📋 Context for the maintainer

Re: CodeRabbit's duplicate-key comment — here is what the change actually does to that case, which its analysis stops short of.

Not a request for a change in this PR; posting because the finding above it is
half the picture and the maintainer should merge knowing the full delta.

CodeRabbit is right that "USD" + "usd" in one payload now collapse to one key
with the survivor picked by HashMap iteration order. What it does not say is
what happened before this PR, which I traced:

🔍 Pre-PR path for a case-variant duplicate

Both keys survived the adapter, then aggregate_tick (src/price/aggregate.rs:141)
uppercased them anyway and pushed both into the same bucket:

let currency = currency.to_uppercase();
match quote {
    Quote::PerBtc(v) => direct.entry(currency).or_default().push((*id, *v)),

Consequences, both real:

  1. combine() sees two values from one provider → Yadio is double-weighted in
    the median.
  2. sources counts candidate values, not distinct providers
    (aggregate.rs:206-210), so it reads 2 for a single-source currency —
    inflating the published figure and suppressing the single-source warn!
    in observe_warnings (manager.rs:386).

This is the same class of bug #859 documents as already having been hit once, in
currencies_covered_by_non_nostr (fixed in 2de2902).

So the change trades a deterministic double-count for a nondeterministic pick. On
the axis that matters — one provider, one vote — it is a net improvement, and
the nondeterminism it introduces is unreachable today (Yadio ships uppercase) and
is shared verbatim by blockchain.rs:55, coingecko.rs:90, currency_api.rs:68
and nostr.rs:146.

Which is why I would not block on it: fixing collision precedence in this one
adapter would leave the other four inconsistent, and #859 explicitly scoped that
kind of cleanup out ("a separate decision and should not be a precondition for the
one-line fix"). If it is worth doing, it belongs in aggregate_tick — dedupe
(provider, currency) pairs once, where it fixes sources for every provider at
once — as a follow-up issue.

@grunch grunch Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🧹 Consistency & Maintainability | 🔵 Nit | 🧷 Do not change in this PR

to_ascii_uppercase() is the precise primitive here — but changing it alone would be worse than leaving it.

ISO-4217 codes are ASCII by definition, and str::to_uppercase() is the full
Unicode mapping: it can expand ("ß" → "SS", "fi" → "FI") and it
allocates unconditionally even when the input is already uppercase — which,
per the comment two lines up, is every payload Yadio ships today. On a 120+
currency response that is 120+ throwaway Strings per tick.

Neither cost is worth acting on here, because the four siblings
(blockchain.rs:55, coingecko.rs:90, currency_api.rs:68,
nostr.rs:146) all use to_uppercase(), and #859's whole argument is that
Yadio should look like its siblings. Diverging on this line would trade the
consistency the PR just bought for a micro-optimisation.

Flagging it so it is on the record: if it is ever revisited, all five change
together in one commit, not this one.

}
_ => None,
})
.collect())
Expand Down Expand Up @@ -111,6 +113,18 @@ mod tests {
assert_eq!(quotes.get("GBP"), Some(&Quote::PerBtc(50_000.0)));
}

#[test]
fn canonicalises_lowercase_currency_codes() {
let body = r#"{"BTC": {"usd": 75000.0, "eur": 65000.0}}"#;
let quotes = YadioProvider::parse(body).unwrap();
assert_eq!(quotes.get("USD"), Some(&Quote::PerBtc(75_000.0)));
assert_eq!(quotes.get("EUR"), Some(&Quote::PerBtc(65_000.0)));
assert!(
!quotes.contains_key("usd"),
"raw lowercase key must not survive"
);

@grunch grunch Aug 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🎯 Test Precision | 🔵 Nit | ⚡ Quick win

Pin the map size, and cover mixed case.

CodeRabbit already asked for the missing "eur" assertion; the size assertion is
the stronger form of the same request and subsumes it — drops_non_finite_and_non_positive
(line 108) already uses that idiom in this very file:

        assert_eq!(quotes.len(), 2, "only the two canonical keys survive");

Separately, an all-lowercase input is the weakest possible probe for this fix.
Both to_uppercase() and a hypothetical wrong to_ascii_uppercase()/manual
first-char implementation pass it. A mixed-case code is what actually discriminates:

        let body = r#"{"BTC": {"usd": 75000.0, "Eur": 65000.0}}"#;

This is a nit — the fix is a stdlib call, so the risk of it being wrong is ~0. It
costs one character to make the test earn its name.

Comment on lines +125 to +128

@grunch grunch Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🎯 Test Precision | 🔵 Nit | ⚡ Quick win

This assertion is now dead weight — the size assertion you added on line 122 already proves it.

assert_eq!(quotes.len(), 2, "only the two canonical keys survive");
assert_eq!(quotes.get("USD"), Some(&Quote::PerBtc(75_000.0)));
assert_eq!(quotes.get("EUR"), Some(&Quote::PerBtc(65_000.0)));

Three assertions pin the map to exactly {USD, EUR}: length 2, with two
distinct known keys present. "usd" cannot also be in a 2-entry map that
already contains USD and EUR, so lines 125-128 can never fail while
122-124 pass — it is unreachable as a failure signal.

That is precisely why the size form was suggested over CodeRabbit's
"add the eur check": it subsumes both. Keeping the redundant assert is
harmless — dropping lines 125-128 leaves the same contract with less to read.

Your call: if you prefer to keep it as executable prose naming the bug the
test guards against, that is a legitimate reason to leave it, and I would not
hold the merge for it.

}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
#[test]
fn parse_error_is_returned() {
let err = YadioProvider::parse("not json").unwrap_err();
Expand Down