feat: fetch exchange rates from Nostr with HTTP/cache fallback#551
feat: fetch exchange rates from Nostr with HTTP/cache fallback#551
Conversation
Implements #550 ## Changes ### New: NostrExchangeService (replaces YadioExchangeService as default) Fetches BTC/fiat rates from Nostr (NIP-33 kind 30078) with automatic fallback to Yadio HTTP API and SharedPreferences cache. Fallback chain: Nostr (10s) → Yadio HTTP (30s) → SharedPreferences cache ### Security Events are verified against settings.mostroPublicKey to prevent price manipulation attacks from untrusted publishers. ### Files - lib/services/nostr_exchange_service.dart — main service - lib/shared/providers/exchange_service_provider.dart — updated provider - test/services/nostr_exchange_service_test.dart — unit tests - docs/NOSTR_EXCHANGE_RATES.md — documentation Closes #550
- Fix createdAt comparison: use DateTime.isAfter instead of compareTo with String default (createdAt is DateTime?, not String) - Remove unused import in test file
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe pull request introduces Nostr-based Bitcoin/fiat exchange rate fetching with a multi-tier fallback mechanism. A new Changes
Sequence DiagramsequenceDiagram
participant App as Mobile App
participant Service as NostrExchangeService
participant Mem as In-Memory Cache
participant Nostr as Nostr Relay
participant Http as Yadio HTTP
participant Prefs as SharedPreferences
App->>Service: getExchangeRate(currency)
Service->>Mem: Check cache
alt Cache Hit
Mem-->>Service: Return rate
else Cache Miss
Service->>Service: _refreshRates()
Service->>Nostr: Subscribe kind 30078<br/>(10s timeout)
alt Nostr Success
Nostr-->>Service: Validate pubkey & parse JSON
Service->>Mem: Update cache
Prefs->>Prefs: Persist rates + timestamp
else Nostr Timeout/Failure
Service->>Http: Request BTC rates
alt HTTP Success
Http-->>Service: Parse response
Service->>Mem: Update cache
Prefs->>Prefs: Persist rates + timestamp
else HTTP Failure
Service->>Prefs: Load cached rates
alt Cache Valid (<1hr)
Prefs-->>Service: Return persisted rates
else Cache Expired/Missing
Service-->>App: Throw exception
end
end
end
end
Service-->>App: Return rate
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/NOSTR_EXCHANGE_RATES.md`:
- Around line 11-28: Add a language specifier to the fenced code block that
contains the ASCII flow diagram (the block starting with ``` above "Request rate
for USD") by changing the opening fence from ``` to ```text so markdownlint
MD040 is satisfied; no other content changes are needed.
- Around line 53-55: The docs currently claim "verify event signatures" but the
implementation in lib/services/nostr_exchange_service.dart only compares
event.pubkey to settings.mostroPublicKey; update the wording to accurately say
this is a pubkey/origin check (e.g., "verify event pubkey by comparing
event.pubkey to settings.mostroPublicKey" or "verify event origin via pubkey
comparison") and avoid implying full cryptographic signature verification unless
signature checking is performed elsewhere; reference settings.mostroPublicKey
and the code path that reads event.pubkey to ensure the doc matches behavior.
In `@lib/services/nostr_exchange_service.dart`:
- Around line 157-175: _fetchFromYadio currently returns an empty map when BTC
exists but contains no numeric entries, which short-circuits the fallback chain;
change _fetchFromYadio to detect when the constructed rates map is empty and
throw a descriptive exception (e.g., "No usable rates from Yadio") instead of
returning {} so that _refreshRates will treat this as a failure and continue
falling back (and thus avoid persisting an empty cache); keep the
Map<String,double> signature but raise the error early when rates.isEmpty to
ensure getExchangeRate can fall back to SharedPreferences.
- Around line 34-37: The in-memory cache _cachedRates currently short-circuits
all future requests once populated and never expires; add a companion DateTime?
field (e.g., _cachedRatesFetchedAt) and enforce the same 1-hour freshness used
for SharedPreferences: on fetch set _cachedRates and _cachedRatesFetchedAt =
DateTime.now(), when reading return cached rates only if _cachedRates != null &&
DateTime.now().difference(_cachedRatesFetchedAt!).inMinutes < 60, and when
hydrating from SharedPreferences restore both the rates and the persisted
timestamp (do not overwrite the persisted timestamp with now) so the in-memory
cache respects the original expiration; also ensure any cache-clear or error
path clears both _cachedRates and _cachedRatesFetchedAt.
- Around line 123-154: Filter the fetched events list to only include events
where event.kind == _exchangeRatesEventKind and the 'd' tag equals
_exchangeRatesDTag before selecting the newest event; use the existing
tag-parsing pattern (inspect event.tags for a tag whose first element is 'd' and
take its second element) to extract dTag for each event, then call .reduce() (or
pick the latest) on the filtered list, throw if the filtered list is empty, and
only then verify event.pubkey == _mostroPubkey and pass event.content to
_parseRatesContent.
In `@lib/shared/providers/exchange_service_provider.dart`:
- Around line 19-29: The provider currently uses
ref.read(exchangeServiceProvider) causing a non-reactive dependency; change it
to ref.watch(exchangeServiceProvider) inside the StateNotifierProvider.family
callback so ExchangeRateNotifier is recreated when the underlying service (and
its settingsProvider dependencies like the Mostro pubkey) change; ensure you
still construct ExchangeRateNotifier(exchangeService) and call
notifier.fetchExchangeRate(currency) after obtaining the watched service so the
notifier uses the up-to-date exchangeService instance.
In `@test/services/nostr_exchange_service_test.dart`:
- Around line 11-37: The test currently duplicates parsing logic in parseRates
and compares literal pubkey groups, which bypasses NostrExchangeService; update
the tests to exercise the real code path by instantiating NostrExchangeService
and driving its parsing/validation (call the public method that consumes
NostrService responses) using a Mockito-generated fake NostrService and a fake
cache seam, or alternatively move the parsing/validation logic into a new
production helper (e.g. export _parseRatesContent as parseRatesFromContent or a
RatesParser class) and write tests against that helper; reference
NostrExchangeService, NostrService, _parseRatesContent (or the new helper) and
the pubkey validation code so the assertions run through the actual
implementation instead of the local parseRates copy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fdbc3a8-6073-4e93-9728-057f264728b4
📒 Files selected for processing (4)
docs/NOSTR_EXCHANGE_RATES.mdlib/services/nostr_exchange_service.dartlib/shared/providers/exchange_service_provider.darttest/services/nostr_exchange_service_test.dart
| /// In-memory cache of all BTC→fiat rates from the last successful fetch. | ||
| /// Keys are uppercase currency codes ("USD", "EUR", …), values are the | ||
| /// price of 1 BTC in that currency. | ||
| Map<String, double>? _cachedRates; |
There was a problem hiding this comment.
Expire the in-memory rates cache too.
Line 57 short-circuits every future request once _cachedRates is populated. In a long-running app session that can serve hours-old rates forever, even though the SharedPreferences fallback correctly expires after 1 hour. Track a fetched timestamp alongside _cachedRates, and when hydrating from SharedPreferences preserve the persisted timestamp instead of resetting freshness.
Also applies to: 55-69, 81-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/services/nostr_exchange_service.dart` around lines 34 - 37, The in-memory
cache _cachedRates currently short-circuits all future requests once populated
and never expires; add a companion DateTime? field (e.g., _cachedRatesFetchedAt)
and enforce the same 1-hour freshness used for SharedPreferences: on fetch set
_cachedRates and _cachedRatesFetchedAt = DateTime.now(), when reading return
cached rates only if _cachedRates != null &&
DateTime.now().difference(_cachedRatesFetchedAt!).inMinutes < 60, and when
hydrating from SharedPreferences restore both the rates and the persisted
timestamp (do not overwrite the persisted timestamp with now) so the in-memory
cache respects the original expiration; also ensure any cache-clear or error
path clears both _cachedRates and _cachedRatesFetchedAt.
| /// Fetch all BTC rates from Yadio HTTP API and return them as a map. | ||
| Future<Map<String, double>> _fetchFromYadio() async { | ||
| final data = await getRequest('exrates/BTC'); | ||
|
|
||
| final btcRates = data['BTC']; | ||
| if (btcRates is! Map) { | ||
| throw Exception('Unexpected Yadio response format'); | ||
| } | ||
|
|
||
| final rates = <String, double>{}; | ||
| for (final entry in btcRates.entries) { | ||
| if (entry.key == 'BTC') continue; // skip BTC→BTC = 1 | ||
| final value = entry.value; | ||
| if (value is num) { | ||
| rates[entry.key as String] = value.toDouble(); | ||
| } | ||
| } | ||
| return rates; | ||
| } |
There was a problem hiding this comment.
Fail fast when Yadio returns no usable rates.
If BTC exists but none of its entries are numeric, _fetchFromYadio() returns {} and _refreshRates() treats that as success. That stops the fallback chain, persists an empty cache, and getExchangeRate() later fails with Rate not found instead of falling back to SharedPreferences.
Minimal fix
final rates = <String, double>{};
for (final entry in btcRates.entries) {
if (entry.key == 'BTC') continue; // skip BTC→BTC = 1
final value = entry.value;
if (value is num) {
rates[entry.key as String] = value.toDouble();
}
}
+ if (rates.isEmpty) {
+ throw const FormatException('No valid rates found');
+ }
return rates;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Fetch all BTC rates from Yadio HTTP API and return them as a map. | |
| Future<Map<String, double>> _fetchFromYadio() async { | |
| final data = await getRequest('exrates/BTC'); | |
| final btcRates = data['BTC']; | |
| if (btcRates is! Map) { | |
| throw Exception('Unexpected Yadio response format'); | |
| } | |
| final rates = <String, double>{}; | |
| for (final entry in btcRates.entries) { | |
| if (entry.key == 'BTC') continue; // skip BTC→BTC = 1 | |
| final value = entry.value; | |
| if (value is num) { | |
| rates[entry.key as String] = value.toDouble(); | |
| } | |
| } | |
| return rates; | |
| } | |
| /// Fetch all BTC rates from Yadio HTTP API and return them as a map. | |
| Future<Map<String, double>> _fetchFromYadio() async { | |
| final data = await getRequest('exrates/BTC'); | |
| final btcRates = data['BTC']; | |
| if (btcRates is! Map) { | |
| throw Exception('Unexpected Yadio response format'); | |
| } | |
| final rates = <String, double>{}; | |
| for (final entry in btcRates.entries) { | |
| if (entry.key == 'BTC') continue; // skip BTC→BTC = 1 | |
| final value = entry.value; | |
| if (value is num) { | |
| rates[entry.key as String] = value.toDouble(); | |
| } | |
| } | |
| if (rates.isEmpty) { | |
| throw const FormatException('No valid rates found'); | |
| } | |
| return rates; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/services/nostr_exchange_service.dart` around lines 157 - 175,
_fetchFromYadio currently returns an empty map when BTC exists but contains no
numeric entries, which short-circuits the fallback chain; change _fetchFromYadio
to detect when the constructed rates map is empty and throw a descriptive
exception (e.g., "No usable rates from Yadio") instead of returning {} so that
_refreshRates will treat this as a failure and continue falling back (and thus
avoid persisting an empty cache); keep the Map<String,double> signature but
raise the error early when rates.isEmpty to ensure getExchangeRate can fall back
to SharedPreferences.
| final exchangeRateProvider = | ||
| StateNotifierProvider.family< | ||
| ExchangeRateNotifier, | ||
| AsyncValue<double>, | ||
| String | ||
| >((ref, currency) { | ||
| final exchangeService = ref.read(exchangeServiceProvider); | ||
| final notifier = ExchangeRateNotifier(exchangeService); | ||
| notifier.fetchExchangeRate(currency); | ||
| return notifier; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In flutter_riverpod, when a provider factory uses ref.read(otherProvider), does that create a reactive dependency, or is ref.watch(otherProvider)required for the provider to rebuild whenotherProvider changes?
💡 Result:
In flutter_riverpod, when a provider factory (the build function of a provider) uses ref.read(otherProvider), it does NOT create a reactive dependency. The provider will NOT rebuild when otherProvider changes. ref.watch(otherProvider) is required to establish the reactive dependency and trigger a rebuild of the provider when otherProvider changes.
Citations:
- 1: https://docs-v2.riverpod.dev/docs/concepts/reading
- 2: https://pub.dev/documentation/riverpod/latest/riverpod/Ref/read.html
- 3: https://pub.dev/documentation/riverpod/latest/riverpod/Provider-class.html
- 4: https://riverpod.dev/docs/concepts/combining_providers
- 5: Should I use ref.watch or ref.read inside build phase of a notifier provider? rrousselGit/riverpod#3135
🏁 Script executed:
cd /tmp && find . -name "exchange_service_provider.dart" -type f 2>/dev/null | head -5Repository: MostroP2P/mobile
Length of output: 42
🏁 Script executed:
git ls-files "**/*exchange_service_provider.dart" 2>/dev/nullRepository: MostroP2P/mobile
Length of output: 111
🏁 Script executed:
cat -n lib/shared/providers/exchange_service_provider.dartRepository: MostroP2P/mobile
Length of output: 2060
🏁 Script executed:
rg "exchangeRateProvider" --type dart -B 2 -A 2Repository: MostroP2P/mobile
Length of output: 2698
Switch this dependency to watch.
Line 25 uses ref.read(exchangeServiceProvider), which does not create a reactive dependency. Since consumers watch exchangeRateProvider(currency) and exchangeServiceProvider itself depends on settingsProvider, the provider instances will not rebuild when the Mostro pubkey or other settings change. The notifiers will retain stale service references.
Minimal fix
- final exchangeService = ref.read(exchangeServiceProvider);
+ final exchangeService = ref.watch(exchangeServiceProvider);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final exchangeRateProvider = | |
| StateNotifierProvider.family< | |
| ExchangeRateNotifier, | |
| AsyncValue<double>, | |
| String | |
| >((ref, currency) { | |
| final exchangeService = ref.read(exchangeServiceProvider); | |
| final notifier = ExchangeRateNotifier(exchangeService); | |
| notifier.fetchExchangeRate(currency); | |
| return notifier; | |
| }); | |
| final exchangeRateProvider = | |
| StateNotifierProvider.family< | |
| ExchangeRateNotifier, | |
| AsyncValue<double>, | |
| String | |
| >((ref, currency) { | |
| final exchangeService = ref.watch(exchangeServiceProvider); | |
| final notifier = ExchangeRateNotifier(exchangeService); | |
| notifier.fetchExchangeRate(currency); | |
| return notifier; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/shared/providers/exchange_service_provider.dart` around lines 19 - 29,
The provider currently uses ref.read(exchangeServiceProvider) causing a
non-reactive dependency; change it to ref.watch(exchangeServiceProvider) inside
the StateNotifierProvider.family callback so ExchangeRateNotifier is recreated
when the underlying service (and its settingsProvider dependencies like the
Mostro pubkey) change; ensure you still construct
ExchangeRateNotifier(exchangeService) and call
notifier.fetchExchangeRate(currency) after obtaining the watched service so the
notifier uses the up-to-date exchangeService instance.
| Map<String, double> parseRates(String content) { | ||
| // Replicate the parsing logic from NostrExchangeService._parseRatesContent | ||
| final decoded = jsonDecode(content); | ||
| if (decoded is! Map<String, dynamic>) { | ||
| throw const FormatException('Expected JSON object'); | ||
| } | ||
|
|
||
| final btcRates = decoded['BTC']; | ||
| if (btcRates is! Map<String, dynamic>) { | ||
| throw const FormatException('Missing or invalid "BTC" key'); | ||
| } | ||
|
|
||
| final rates = <String, double>{}; | ||
| for (final entry in btcRates.entries) { | ||
| if (entry.key == 'BTC') continue; | ||
| final value = entry.value; | ||
| if (value is num) { | ||
| rates[entry.key] = value.toDouble(); | ||
| } | ||
| } | ||
|
|
||
| if (rates.isEmpty) { | ||
| throw const FormatException('No valid rates found'); | ||
| } | ||
|
|
||
| return rates; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
These assertions never exercise NostrExchangeService.
parseRates() copies the parser, and the pubkey group only compares literals. That means the suite can stay green even if the production parser or pubkey validation regresses, so the critical Nostr path is still effectively untested. Please drive these cases through NostrExchangeService with a fake/mock NostrService and cache seam, or extract the parser/validator into a testable production helper.
As per coding guidelines, "Place unit tests beside their feature counterparts using the *_test.dart suffix and prefer Mockito fakes generated via build_runner."
Also applies to: 116-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/services/nostr_exchange_service_test.dart` around lines 11 - 37, The
test currently duplicates parsing logic in parseRates and compares literal
pubkey groups, which bypasses NostrExchangeService; update the tests to exercise
the real code path by instantiating NostrExchangeService and driving its
parsing/validation (call the public method that consumes NostrService responses)
using a Mockito-generated fake NostrService and a fake cache seam, or
alternatively move the parsing/validation logic into a new production helper
(e.g. export _parseRatesContent as parseRatesFromContent or a RatesParser class)
and write tests against that helper; reference NostrExchangeService,
NostrService, _parseRatesContent (or the new helper) and the pubkey validation
code so the assertions run through the actual implementation instead of the
local parseRates copy.
- Add in-memory cache expiration (_cachedRatesFetchedAt) with same 1-hour TTL as SharedPreferences cache - Filter fetched Nostr events by kind, d-tag, and pubkey before selecting newest (defense-in-depth against untrusted relays) - Throw on empty Yadio rates instead of returning empty map (allows fallback chain to continue to SharedPreferences) - Preserve original persisted timestamp when loading from cache (don't overwrite with DateTime.now()) - Clear stale in-memory cache on total failure - Change ref.read to ref.watch in exchangeRateProvider for reactive dependency on exchangeServiceProvider - Fix docs: add language specifier to code block, clarify pubkey verification is origin check not signature verification
- Rename _parseRatesContent to parseRatesContent (public static) - Tests import and call NostrExchangeService.parseRatesContent directly instead of duplicating the parsing logic locally
2 participants
Implements #550
Summary
Replaces
YadioExchangeService(HTTP-only) withNostrExchangeServicethat fetches BTC/fiat exchange rates from Nostr relays (NIP-33 kind30078) first, falling back to the Yadio HTTP API and a SharedPreferences cache.This solves the censorship problem: the Yadio API is blocked in Venezuela and other regions, but Nostr relays are accessible.
Changes
New:
NostrExchangeService(lib/services/nostr_exchange_service.dart)Drops in as a replacement for
YadioExchangeService— same interface (ExchangeService), different behavior:Key decisions:
SharedPreferencesfor cache (already a dependency, lightweight key-value store — no need for Sembast for a single cached JSON string)getCurrencyCodes()delegates to Yadio (currency list rarely changes, already loaded fromassets/data/fiat.jsonin the provider)Security: Pubkey Verification
The service verifies
event.pubkey == settings.mostroPublicKeybefore parsing rates. This is defense-in-depth:authors: [mostroPubkey]Attack prevented: Malicious actor publishes fake kind
30078events to manipulate prices → rejected because wrong pubkey.Updated:
exchange_service_provider.dartThe provider now watches
settingsProviderso the service automatically updates when the user changes Mostro instance (different pubkey).New: Tests (
test/services/nostr_exchange_service_test.dart)Unit tests covering:
New: Documentation (
docs/NOSTR_EXCHANGE_RATES.md)Overview of the fallback chain, event format, security, and references.
What This Does NOT Change
ExchangeServiceinterface unchanged (all consumers work without modification)currencyCodesProviderstill loads fromassets/data/fiat.jsonexchangeRateProviderAPI unchanged (.familykeyed by currency code)How to Test
Verify daemon is publishing rates
Run unit tests
flutter test test/services/nostr_exchange_service_test.dartManual testing
Nostr exchange rates failed(fallback to HTTP) orUsing cached exchange ratesReferences
Closes #550
Summary by CodeRabbit
New Features
Documentation