Add rate information in on chain wallet and reports - #6841
Merged
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
dstrukt
self-requested a review
July 10, 2025 05:44
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
|
@NicolasDorier Any way we can showcase total in fiat as well?
How can this be unblocked? Most of outgoing or incoming wallet transactions are probably not related to the invoice, why can't we just create on the fly invoice for incoming transactions, just like we do for LNURL payments? |
Member
Author
We could, that would not solve the rate of outgoing transactions though. |
NicolasDorier
marked this pull request as draft
July 10, 2025 09:25
NicolasDorier
force-pushed
the
rate-wallet
branch
from
July 11, 2025 01:26
3d280fd to
68f70d2
Compare
NicolasDorier
changed the title
Contributor
|
cACK, thanks for a clear description of situation! |
dstrukt
reviewed
Jul 12, 2025
NicolasDorier
force-pushed
the
rate-wallet
branch
from
July 12, 2025 11:41
1dfd4fb to
f75e1e2
Compare
NicolasDorier
marked this pull request as ready for review
July 12, 2025 11:41
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Member
Author
This has been addressed, we now show rates for ingoing and outgoing transactions, even those unrelated to invoices. |
NicolasDorier
force-pushed
the
rate-wallet
branch
2 times, most recently
from
July 12, 2025 12:08
b6ea864 to
f5e54f9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
NicolasDorier
force-pushed
the
rate-wallet
branch
2 times, most recently
from
July 14, 2025 00:34
877e796 to
c7709e1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
BTCPayServer/HostedServices/OnChainRateTrackerHostedService.cs (1)
58-61: Use the correct cancellation token parameterThe code should use the
cancellationTokenparameter, not theCancellationTokentype.var fetching = rateFetcher.FetchRates( trackedCurrencies .Select(t => new CurrencyPair(cryptoCode, t)) - .ToHashSet(), rules, new StoreIdRateContext(storeId), CancellationToken); + .ToHashSet(), rules, new StoreIdRateContext(storeId), cancellationToken);
🧹 Nitpick comments (2)
BTCPayServer/HostedServices/OnChainRateTrackerHostedService.cs (2)
41-42: Fix indentation for consistency.The if statement body should be properly indented.
- if (derivation is null) - return; + if (derivation is null) + return;
38-80: Consider adding error handling for rate tracking failures.The method performs multiple async operations that could fail (store retrieval, rate fetching, data persistence). Consider wrapping critical sections in try-catch blocks to ensure individual store failures don't prevent processing other stores, and to provide better observability when rate tracking fails.
Example approach:
foreach (var storeId in stores) { try { // existing store processing logic } catch (Exception ex) { logger.LogError(ex, "Failed to track rates for store {StoreId} on transaction {TxId}", storeId, transactionEvent.NewTransactionEvent.TransactionData.TransactionHash); // Continue processing other stores } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
BTCPayServer.Client/Models/StoreBaseData.cs(1 hunks)BTCPayServer.Tests/PlaywrightTests.cs(3 hunks)BTCPayServer/Controllers/GreenField/GreenfieldStoresController.cs(2 hunks)BTCPayServer/Controllers/UIStoresController.Settings.cs(5 hunks)BTCPayServer/Controllers/UIWalletsController.cs(5 hunks)BTCPayServer/Data/StoreBlob.cs(3 hunks)BTCPayServer/HostedServices/OnChainRateTrackerHostedService.cs(1 hunks)BTCPayServer/Models/StoreViewModels/GeneralSettingsViewModel.cs(2 hunks)BTCPayServer/Payments/IPaymentMethodHandler.cs(1 hunks)BTCPayServer/Services/Reporting/InvoicesReportProvider.cs(7 hunks)BTCPayServer/Services/Reporting/OnChainWalletReportProvider.cs(4 hunks)BTCPayServer/Views/UIStores/GeneralSettings.cshtml(2 hunks)BTCPayServer/wwwroot/swagger/v1/swagger.template.stores.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- BTCPayServer/Views/UIStores/GeneralSettings.cshtml
- BTCPayServer/Controllers/GreenField/GreenfieldStoresController.cs
- BTCPayServer/Models/StoreViewModels/GeneralSettingsViewModel.cs
- BTCPayServer/Payments/IPaymentMethodHandler.cs
- BTCPayServer.Client/Models/StoreBaseData.cs
- BTCPayServer/Controllers/UIStoresController.Settings.cs
- BTCPayServer/wwwroot/swagger/v1/swagger.template.stores.json
- BTCPayServer/Controllers/UIWalletsController.cs
- BTCPayServer/Data/StoreBlob.cs
- BTCPayServer/Services/Reporting/InvoicesReportProvider.cs
- BTCPayServer/Services/Reporting/OnChainWalletReportProvider.cs
- BTCPayServer.Tests/PlaywrightTests.cs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: NicolasDorier
PR: btcpayserver/btcpayserver#0
File: :0-0
Timestamp: 2025-04-22T06:54:36.253Z
Learning: Do not edit or modify PR descriptions in the btcpayserver repository as requested by NicolasDorier.
Learnt from: NicolasDorier
PR: btcpayserver/btcpayserver#0
File: :0-0
Timestamp: 2025-04-26T03:27:15.243Z
Learning: NicolasDorier prefers CodeRabbit reviews to focus on identifying nits and obvious bugs rather than providing summaries of changes.
Learnt from: NicolasDorier
PR: btcpayserver/btcpayserver#6796
File: BTCPayServer/Views/UIWallets/ReservedAddresses.cshtml:38-38
Timestamp: 2025-06-30T01:04:02.183Z
Learning: NicolasDorier prefers to avoid mixing Razor server-side rendering with Vue.js client-side rendering to prevent potential XSS vulnerabilities and recommends using pure Vue.js for dynamic table generation instead.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
BTCPayServer/Controllers/GreenField/GreenfieldStoreRatesController.cs (1)
71-71: Consider optimizing the redundant await.The
await rateTask.Valueis redundant sinceTask.WhenAll(rateTasks.Values)on line 67 already ensures all tasks are completed. You could access the result directly:- var rateTaskResult = await rateTask.Value; + var rateTaskResult = rateTask.Value.Result;However, using
awaitis safer than the previous synchronous.Resultapproach and doesn't cause significant performance issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
BTCPayServer/Controllers/GreenField/GreenfieldStoreRatesController.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: NicolasDorier
PR: btcpayserver/btcpayserver#0
File: :0-0
Timestamp: 2025-04-22T06:54:36.253Z
Learning: Do not edit or modify PR descriptions in the btcpayserver repository as requested by NicolasDorier.
Learnt from: NicolasDorier
PR: btcpayserver/btcpayserver#0
File: :0-0
Timestamp: 2025-04-26T03:27:15.243Z
Learning: NicolasDorier prefers CodeRabbit reviews to focus on identifying nits and obvious bugs rather than providing summaries of changes.
Learnt from: NicolasDorier
PR: btcpayserver/btcpayserver#6796
File: BTCPayServer/Views/UIWallets/ReservedAddresses.cshtml:38-38
Timestamp: 2025-06-30T01:04:02.183Z
Learning: NicolasDorier prefers to avoid mixing Razor server-side rendering with Vue.js client-side rendering to prevent potential XSS vulnerabilities and recommends using pure Vue.js for dynamic table generation instead.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
NicolasDorier
force-pushed
the
rate-wallet
branch
from
July 14, 2025 06:19
09765f0 to
4e34896
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
BTCPayServer/Services/Reporting/OnChainWalletReportProvider.cs (2)
143-143: Add error handling for database operations.The invoice rate fetching lacks error handling and could cause the entire report generation to fail.
Add error handling around the database call:
-var rates = await InvoiceRepository.GetRatesOfInvoices(queryContext.Data.Select(r => r[3]).OfType<string>().ToHashSet()); +Dictionary<string, RateBook> rates; +try +{ + rates = await InvoiceRepository.GetRatesOfInvoices(queryContext.Data.Select(r => r[3]).OfType<string>().ToHashSet()); +} +catch (Exception) +{ + // Log error and continue with empty rates + rates = new Dictionary<string, RateBook>(); +}
169-170: Use appropriate default currency when creating fallback RateBook.Creating a RateBook with an empty currency string could lead to issues in rate calculations and display.
Apply this diff to use a more appropriate default:
-rates.TryGetValue(invoiceId ?? "", out var r); -r ??= new("", new()); +var hasInvoiceRates = invoiceId != null && rates.TryGetValue(invoiceId, out var r); +if (!hasInvoiceRates) + r = new(store.GetStoreBlob().DefaultCurrency ?? cryptoCode ?? "BTC", new());
🧹 Nitpick comments (2)
BTCPayServer/Services/Reporting/OnChainWalletReportProvider.cs (2)
84-104: Consider extracting database query parameters for better maintainability.The inline command definition with complex SQL and parameter construction makes the code harder to read and maintain.
Consider extracting the query construction:
+private static CommandDefinition CreateWalletTransactionQuery( + string walletId, string code, string? assetId, TimeSpan interval, CancellationToken cancellation) +{ + return new CommandDefinition( + commandText: """ + SELECT r.tx_id, r.seen_at, t.blk_id, t.blk_height, r.balance_change + FROM get_wallets_recent(@wallet_id, @code, @asset_id, @interval, NULL, NULL) r + JOIN txs t USING (code, tx_id) + ORDER BY r.seen_at + """, + parameters: new { asset_id = assetId, wallet_id = walletId, code, interval }, + cancellationToken: cancellation); +} -var command = new CommandDefinition( - commandText: - "SELECT r.tx_id, r.seen_at, t.blk_id, t.blk_height, r.balance_change " + - "FROM get_wallets_recent(@wallet_id, @code, @asset_id, @interval, NULL, NULL) r " + - "JOIN txs t USING (code, tx_id) " + - "ORDER BY r.seen_at", - parameters: new - { - asset_id = GetAssetId(network), - wallet_id = NBXplorer.Client.DBUtils.nbxv1_get_wallet_id(network.CryptoCode, settings.AccountDerivation.ToString()), - code = network.CryptoCode, - interval - }, - cancellationToken: cancellation); +var command = CreateWalletTransactionQuery( + NBXplorer.Client.DBUtils.nbxv1_get_wallet_id(network.CryptoCode, settings.AccountDerivation.ToString()), + network.CryptoCode, + GetAssetId(network), + interval, + cancellation);
161-179: Simplify the rate application logic for better maintainability.The nested loops and complex rate merging logic is difficult to follow and maintain.
Consider breaking this into smaller, focused methods:
+private void ApplyRatesToRows(List<List<object?>> rows, Dictionary<string, RateBook> invoiceRates, + Dictionary<string, RateBook> transactionRates, HashSet<string> trackedCurrencies) +{ + foreach (var row in rows) + { + var invoiceId = row[3] as string; + var cryptoCode = row[1] as string; + if (cryptoCode is null) continue; + + var rateBook = GetMergedRateBook(invoiceId, invoiceRates, transactionRates, cryptoCode); + AddTrackedCurrencyRates(row, rateBook, cryptoCode, trackedCurrencies); + } +} +private RateBook GetMergedRateBook(string? invoiceId, Dictionary<string, RateBook> invoiceRates, + Dictionary<string, RateBook> transactionRates, string cryptoCode) +{ + var rateBook = invoiceId != null && invoiceRates.TryGetValue(invoiceId, out var r) + ? r + : new(cryptoCode, new()); + + if (transactionRates.TryGetValue(/* transaction key */, out var txRates)) + rateBook.AddRates(txRates); + + return rateBook; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
BTCPayServer.Client/Models/StoreBaseData.cs(1 hunks)BTCPayServer.Tests/CSVInvoicesTester.cs(1 hunks)BTCPayServer.Tests/CSVTester.cs(1 hunks)BTCPayServer.Tests/POSTests.cs(2 hunks)BTCPayServer.Tests/PlaywrightTester.cs(3 hunks)BTCPayServer.Tests/PlaywrightTests.cs(3 hunks)BTCPayServer.Tests/ServerTester.cs(1 hunks)BTCPayServer/Controllers/GreenField/GreenfieldStoreRatesController.cs(1 hunks)BTCPayServer/Controllers/GreenField/GreenfieldStoresController.cs(2 hunks)BTCPayServer/Controllers/UIStoresController.Settings.cs(5 hunks)BTCPayServer/Controllers/UIWalletsController.cs(5 hunks)BTCPayServer/Data/StoreBlob.cs(3 hunks)BTCPayServer/Data/WalletTransactionInfo.cs(3 hunks)BTCPayServer/HostedServices/OnChainRateTrackerHostedService.cs(1 hunks)BTCPayServer/Hosting/BTCPayServerServices.cs(1 hunks)BTCPayServer/Models/StoreViewModels/GeneralSettingsViewModel.cs(2 hunks)BTCPayServer/Models/WalletViewModels/ListTransactionsViewModel.cs(2 hunks)BTCPayServer/Payments/IPaymentMethodHandler.cs(1 hunks)BTCPayServer/Services/Invoices/InvoiceEntity.cs(1 hunks)BTCPayServer/Services/Invoices/InvoiceRepository.cs(3 hunks)BTCPayServer/Services/Invoices/RateBook.cs(1 hunks)BTCPayServer/Services/Reporting/InvoicesReportProvider.cs(7 hunks)BTCPayServer/Services/Reporting/OnChainWalletReportProvider.cs(4 hunks)BTCPayServer/Services/Stores/StoreRepository.cs(2 hunks)BTCPayServer/Services/WalletRepository.cs(5 hunks)BTCPayServer/Views/UIReports/StoreReports.cshtml(1 hunks)BTCPayServer/Views/UIStores/GeneralSettings.cshtml(2 hunks)BTCPayServer/Views/UIWallets/WalletTransactions.cshtml(11 hunks)BTCPayServer/Views/UIWallets/_WalletTransactionsList.cshtml(1 hunks)BTCPayServer/wwwroot/js/store-reports.js(2 hunks)BTCPayServer/wwwroot/main/site.css(8 hunks)BTCPayServer/wwwroot/swagger/v1/swagger.template.stores.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- BTCPayServer/wwwroot/js/store-reports.js
🚧 Files skipped from review as they are similar to previous changes (30)
- BTCPayServer/Views/UIReports/StoreReports.cshtml
- BTCPayServer/Hosting/BTCPayServerServices.cs
- BTCPayServer/Controllers/GreenField/GreenfieldStoreRatesController.cs
- BTCPayServer/Views/UIWallets/_WalletTransactionsList.cshtml
- BTCPayServer.Tests/CSVTester.cs
- BTCPayServer/Controllers/UIStoresController.Settings.cs
- BTCPayServer/Payments/IPaymentMethodHandler.cs
- BTCPayServer/Views/UIStores/GeneralSettings.cshtml
- BTCPayServer/Services/Stores/StoreRepository.cs
- BTCPayServer/Models/StoreViewModels/GeneralSettingsViewModel.cs
- BTCPayServer/wwwroot/main/site.css
- BTCPayServer.Tests/ServerTester.cs
- BTCPayServer.Tests/POSTests.cs
- BTCPayServer/Controllers/GreenField/GreenfieldStoresController.cs
- BTCPayServer/Services/Invoices/InvoiceRepository.cs
- BTCPayServer/wwwroot/swagger/v1/swagger.template.stores.json
- BTCPayServer/Data/WalletTransactionInfo.cs
- BTCPayServer.Tests/PlaywrightTester.cs
- BTCPayServer/Data/StoreBlob.cs
- BTCPayServer/HostedServices/OnChainRateTrackerHostedService.cs
- BTCPayServer.Tests/CSVInvoicesTester.cs
- BTCPayServer/Views/UIWallets/WalletTransactions.cshtml
- BTCPayServer/Models/WalletViewModels/ListTransactionsViewModel.cs
- BTCPayServer/Controllers/UIWalletsController.cs
- BTCPayServer.Client/Models/StoreBaseData.cs
- BTCPayServer/Services/Reporting/InvoicesReportProvider.cs
- BTCPayServer.Tests/PlaywrightTests.cs
- BTCPayServer/Services/WalletRepository.cs
- BTCPayServer/Services/Invoices/RateBook.cs
- BTCPayServer/Services/Invoices/InvoiceEntity.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NicolasDorier
PR: btcpayserver/btcpayserver#0
File: :0-0
Timestamp: 2025-04-22T06:54:36.253Z
Learning: Do not edit or modify PR descriptions in the btcpayserver repository as requested by NicolasDorier.
Learnt from: NicolasDorier
PR: btcpayserver/btcpayserver#0
File: :0-0
Timestamp: 2025-04-26T03:27:15.243Z
Learning: NicolasDorier prefers CodeRabbit reviews to focus on identifying nits and obvious bugs rather than providing summaries of changes.
Learnt from: NicolasDorier
PR: btcpayserver/btcpayserver#6796
File: BTCPayServer/Views/UIWallets/ReservedAddresses.cshtml:38-38
Timestamp: 2025-06-30T01:04:02.183Z
Learning: NicolasDorier prefers to avoid mixing Razor server-side rendering with Vue.js client-side rendering to prevent potential XSS vulnerabilities and recommends using pure Vue.js for dynamic table generation instead.
BTCPayServer/Services/Reporting/OnChainWalletReportProvider.cs (2)
Learnt from: Abhijay007
PR: btcpayserver/btcpayserver#6830
File: BTCPayServer.Tests/PlaywrightTests.cs:1015-1025
Timestamp: 2025-07-05T11:22:23.462Z
Learning: When suggesting changes to working test code in BTCPayServer project, especially for URL parsing or DOM interaction, the original working implementation should be preferred over potentially more robust alternatives if they cause test failures in the actual environment.
Learnt from: NicolasDorier
PR: btcpayserver/btcpayserver#6748
File: BTCPayServer/Services/Wallets/BTCPayWallet.cs:371-374
Timestamp: 2025-05-22T23:44:48.318Z
Learning: In BTCPayServer, all Bitcoin transactions are intentionally considered RBF-capable by default (via the `Network.IsBTC` condition in the RBF determination logic), regardless of whether the node is opted into Full-RBF or the transaction signals RBF explicitly.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (1)
BTCPayServer/Services/Reporting/OnChainWalletReportProvider.cs (1)
23-26: LGTM: Clean dependency injection implementation.The addition of
InvoiceRepositoryandDisplayFormatterdependencies follows proper DI patterns and enables the rate tracking functionality.Also applies to: 30-31, 38-39
NicolasDorier
force-pushed
the
rate-wallet
branch
4 times, most recently
from
July 17, 2025 04:38
f9da05d to
c766c91
Compare
NicolasDorier
force-pushed
the
rate-wallet
branch
from
July 17, 2025 04:41
833d480 to
962a71a
Compare
Merged
45 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Currently, we only show the exchange rate between the payment currency and the invoice currency. For example, if a USD invoice is paid with BTC, the merchant can see the BTC/USD rate.
However, there are many other contexts where merchants would benefit from seeing exchange rate information—for example, in the wallet transaction list (where a transaction may or may not be related to an invoice), and in reports.
Additionally, in some cases, a merchant may need to know the rate of more than one currency. For instance, when sending funds to contributors of the BTCPay Server Foundation, contributors upload USD- or EUR-denominated invoices, payments are made in BTC, but tax calculations require the JPY rate. In such cases, the store should be able to track not only the exchange rate for the invoice currency, but also for JPY.
This PR adds a new option to specify additional tracked currencies for a store.
Configure the additional tracked currency in the store settings
In the example of the BTCPay Server Foundation, we would fill this new option with
JPY.Rate information in the wallet transaction list
Rate information now appears in the wallet transaction list.
Rate information in the Wallet report
Rate information in the Invoice report
I decided against truncating the rate, as it may have significant repercussion on accounting if the price of a crypto unit is very small.
Easily go to Wallet report from the wallet transactions list
Similar to what we did in #6835, I added a
Reportingbutton.Implementation details
There are two types of rates:
For the "Wallets" report and in wallet transactions list, we are mixing both. When two values conflict, the invoice rate is chosen.
If the tracking of additionally tracked currency fails at invoice creation time, the invoice will still properly generate, and the data will be missing.