Skip to content
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.

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

[VDG] Privacy warnings Improved UX #11463

Merged
merged 20 commits into from
Mar 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 53 additions & 0 deletions WalletWasabi.Fluent/Behaviors/PrivacyWarningFadeOutBehavior.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
using Avalonia;
using Avalonia.Animation;
using Avalonia.Controls;
using Avalonia.Controls.Primitives;
using ReactiveUI;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using System.Reactive.Disposables;
using System.Reactive.Linq;
using WalletWasabi.Fluent.Extensions;
using WalletWasabi.Fluent.Models.Transactions;

namespace WalletWasabi.Fluent.Behaviors;

public class PrivacyWarningFadeOutBehavior : AttachedToVisualTreeBehavior<Control>
{
private const string FadeOutClassName = "fadeout";

public static readonly StyledProperty<IEnumerable<PrivacyWarning>> PreviewWarningsProperty =
AvaloniaProperty.Register<PrivacyWarningFadeOutBehavior, IEnumerable<PrivacyWarning>>(nameof(PreviewWarnings));

public IEnumerable<PrivacyWarning> PreviewWarnings
{
get => GetValue(PreviewWarningsProperty);
set => SetValue(PreviewWarningsProperty, value);
}

protected override void OnAttachedToVisualTree(CompositeDisposable disposable)
{
if (AssociatedObject?.DataContext is not PrivacyWarning current)
{
return;
}

this.WhenAnyValue(x => x.PreviewWarnings)
.WhereNotNull()
.Do(_ =>
{
var fadeOut = !PreviewWarnings.Any(p => p == current);
if (fadeOut && !AssociatedObject.Classes.Contains(FadeOutClassName))
{
AssociatedObject.Classes.Add(FadeOutClassName);
}
else if (!fadeOut && AssociatedObject.Classes.Contains(FadeOutClassName))
{
AssociatedObject.Classes.Remove(FadeOutClassName);
}
})
.Subscribe()
.DisposeWith(disposable);
}
}
10 changes: 6 additions & 4 deletions WalletWasabi.Fluent/Models/Transactions/PrivacySuggestion.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@

public record LabelManagementSuggestion(BuildTransactionResult? Transaction = null, LabelsArray? NewLabels = null) : PrivacySuggestion(Transaction);

public record FullPrivacySuggestion(BuildTransactionResult Transaction, decimal Difference, string DifferenceFiatText, IEnumerable<SmartCoin> Coins, bool IsChangeless) : PrivacySuggestion(Transaction);
public record FullPrivacySuggestion(BuildTransactionResult Transaction, decimal Difference, string DifferenceText, string DifferenceAmountText, IEnumerable<SmartCoin> Coins, bool IsChangeless) : PrivacySuggestion(Transaction);

public record BetterPrivacySuggestion(BuildTransactionResult Transaction, string DifferenceFiatText, IEnumerable<SmartCoin> Coins, bool IsChangeless) : PrivacySuggestion(Transaction);
public record BetterPrivacySuggestion(BuildTransactionResult Transaction, string DifferenceText, string DifferenceAmountText, IEnumerable<SmartCoin> Coins, bool IsChangeless) : PrivacySuggestion(Transaction);

public record ChangeAvoidanceSuggestion(BuildTransactionResult Transaction, decimal Difference, string DifferenceFiatText, bool IsMore, bool IsLess) : PrivacySuggestion(Transaction)
public record ChangeAvoidanceSuggestion(BuildTransactionResult Transaction, decimal Difference, string DifferenceText, string DifferenceAmountText, bool IsMore, bool IsLess) : PrivacySuggestion(Transaction)
{
public Money GetAmount(BitcoinAddress destination) => Transaction!.CalculateDestinationAmount(destination);
public Money GetAmount(BitcoinAddress destination) => Transaction!.CalculateDestinationAmount(destination);

public bool IsSameAmount => !IsMore && !IsLess;

Check notice on line 25 in WalletWasabi.Fluent/Models/Transactions/PrivacySuggestion.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (master)

ℹ Getting worse: Excess Number of Function Arguments

ChangeAvoidanceSuggestion increases from 5 to 6 arguments, threshold = 4. This function has too many arguments, indicating a lack of encapsulation. Avoid adding more arguments.
}
120 changes: 72 additions & 48 deletions WalletWasabi.Fluent/Models/Transactions/PrivacySuggestionsModel.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using DynamicData;

Check notice on line 1 in WalletWasabi.Fluent/Models/Transactions/PrivacySuggestionsModel.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (master)

✅ Getting better: Overall Code Complexity

The mean cyclomatic complexity decreases from 4.85 to 4.79, threshold = 4. This file has many conditional statements (e.g. if, for, while) across its implementation, leading to lower code health. Avoid adding more conditionals.
using NBitcoin;
using Nito.AsyncEx;
using System.Collections.Generic;
Expand Down Expand Up @@ -43,9 +43,13 @@
_cjManager = Services.HostedServices.Get<CoinJoinManager>();
}

/// <summary>
///
/// </summary>
/// <remarks>Method supports being called multiple times. In that case the last call cancels the previous one.</remarks>
public async IAsyncEnumerable<PrivacyItem> BuildPrivacySuggestionsAsync(TransactionInfo info, BuildTransactionResult transactionResult, [EnumeratorCancellation] CancellationToken cancellationToken)
public async IAsyncEnumerable<PrivacyItem> BuildPrivacySuggestionsAsync(TransactionInfo transactionInfo, BuildTransactionResult transactionResult, [EnumeratorCancellation] CancellationToken cancellationToken, bool includeSuggestions)
{
var parameters = new Parameters(transactionInfo, transactionResult, includeSuggestions);
var result = new List<PrivacyItem>();

using CancellationTokenSource singleRunCts = new();
Expand All @@ -62,16 +66,16 @@

using (await _asyncLock.LockAsync(CancellationToken.None))
{
result.Add(VerifyLabels(info, transactionResult));
result.Add(VerifyPrivacyLevel(info, transactionResult));
result.Add(VerifyConsolidation(transactionResult));
result.Add(VerifyUnconfirmedInputs(transactionResult));
result.Add(VerifyCoinjoiningInputs(transactionResult));
result.Add(VerifyLabels(parameters));
result.Add(VerifyPrivacyLevel(parameters));
result.Add(VerifyConsolidation(parameters));
result.Add(VerifyUnconfirmedInputs(parameters));
result.Add(VerifyCoinjoiningInputs(parameters));
foreach (var item in result)
{
yield return item;
}
await foreach (var item in VerifyChangeAsync(info, transactionResult, _linkedCancellationTokenSource).ConfigureAwait(false))
await foreach (var item in VerifyChangeAsync(parameters, _linkedCancellationTokenSource).ConfigureAwait(false))
{
yield return item;
}
Expand All @@ -82,15 +86,15 @@
}
}

private IEnumerable<PrivacyItem> VerifyLabels(TransactionInfo info, BuildTransactionResult transactionResult)
private IEnumerable<PrivacyItem> VerifyLabels(Parameters parameters)
{
var warning = GetLabelWarning(transactionResult, info.Recipient);
var warning = GetLabelWarning(parameters.Transaction, parameters.TransactionInfo.Recipient);

if (warning is not null)
{
yield return warning;

if (info.IsOtherPocketSelectionPossible)
if (parameters.IncludeSuggestions && parameters.TransactionInfo.IsOtherPocketSelectionPossible)
{
yield return new LabelManagementSuggestion();
}
Expand Down Expand Up @@ -121,125 +125,132 @@
return null;
}

private IEnumerable<PrivacyItem> VerifyPrivacyLevel(TransactionInfo transactionInfo, BuildTransactionResult originalTransaction)
private IEnumerable<PrivacyItem> VerifyPrivacyLevel(Parameters parameters)
{
var canModifyTransactionAmount = !transactionInfo.IsPayJoin && !transactionInfo.IsFixedAmount;
var canModifyTransactionAmount = !parameters.TransactionInfo.IsPayJoin && !parameters.TransactionInfo.IsFixedAmount;

var transactionLabels = originalTransaction.SpentCoins.SelectMany(x => x.GetLabels(_wallet.AnonScoreTarget));
var transactionLabels = parameters.Transaction.SpentCoins.SelectMany(x => x.GetLabels(_wallet.AnonScoreTarget));
var onlyKnownByRecipient =
transactionInfo.Recipient.Equals(new LabelsArray(transactionLabels), StringComparer.OrdinalIgnoreCase);
parameters.TransactionInfo.Recipient.Equals(new LabelsArray(transactionLabels), StringComparer.OrdinalIgnoreCase);

var foundNonPrivate = !onlyKnownByRecipient &&
originalTransaction.SpentCoins.Any(x => x.GetPrivacyLevel(_wallet.AnonScoreTarget) == PrivacyLevel.NonPrivate);
parameters.Transaction.SpentCoins.Any(x => x.GetPrivacyLevel(_wallet.AnonScoreTarget) == PrivacyLevel.NonPrivate);

var foundSemiPrivate =
originalTransaction.SpentCoins.Any(x => x.GetPrivacyLevel(_wallet.AnonScoreTarget) == PrivacyLevel.SemiPrivate);
parameters.Transaction.SpentCoins.Any(x => x.GetPrivacyLevel(_wallet.AnonScoreTarget) == PrivacyLevel.SemiPrivate);

if (foundNonPrivate)
{
yield return new NonPrivateFundsWarning();
}

if (foundSemiPrivate)
{
yield return new SemiPrivateFundsWarning();
}

if (!parameters.IncludeSuggestions)
{
// Return early, to avoid needless compute.
yield break;
}

ImmutableList<SmartCoin> coinsToExclude = _cjManager.CoinsInCriticalPhase[_wallet.WalletId];
bool wasCoinjoiningCoinUsed = originalTransaction.SpentCoins.Any(coinsToExclude.Contains);
bool wasCoinjoiningCoinUsed = parameters.Transaction.SpentCoins.Any(coinsToExclude.Contains);

// Only exclude coins if the original transaction doesn't use them either.
var allPrivateCoin = _wallet.Coins.Where(x => x.GetPrivacyLevel(_wallet.AnonScoreTarget) == PrivacyLevel.Private).ToArray();

allPrivateCoin = wasCoinjoiningCoinUsed ? allPrivateCoin : allPrivateCoin.Except(coinsToExclude).ToArray();

var onlyKnownByTheRecipientCoins = _wallet.Coins.Where(x => transactionInfo.Recipient.Equals(x.GetLabels(_wallet.AnonScoreTarget), StringComparer.OrdinalIgnoreCase)).ToArray();
var onlyKnownByTheRecipientCoins = _wallet.Coins.Where(x => parameters.TransactionInfo.Recipient.Equals(x.GetLabels(_wallet.AnonScoreTarget), StringComparer.OrdinalIgnoreCase)).ToArray();
var allSemiPrivateCoin =
_wallet.Coins.Where(x => x.GetPrivacyLevel(_wallet.AnonScoreTarget) == PrivacyLevel.SemiPrivate)
.Union(onlyKnownByTheRecipientCoins)
.ToArray();

allSemiPrivateCoin = wasCoinjoiningCoinUsed ? allSemiPrivateCoin : allSemiPrivateCoin.Except(coinsToExclude).ToArray();

var usdExchangeRate = _wallet.Synchronizer.UsdExchangeRate;
var totalAmount = originalTransaction.CalculateDestinationAmount(transactionInfo.Destination).ToDecimal(MoneyUnit.BTC);
var totalAmount = parameters.Transaction.CalculateDestinationAmount(parameters.TransactionInfo.Destination).ToDecimal(MoneyUnit.BTC);
FullPrivacySuggestion? fullPrivacySuggestion = null;

if ((foundNonPrivate || foundSemiPrivate) && allPrivateCoin.Length != 0 &&
TryCreateTransaction(transactionInfo, allPrivateCoin, out var newTransaction, out var isChangeless))
TryCreateTransaction(parameters.TransactionInfo, allPrivateCoin, out var newTransaction, out var isChangeless))
{
var amountDifference = totalAmount - newTransaction.CalculateDestinationAmount(transactionInfo.Destination).ToDecimal(MoneyUnit.BTC);
var amountDifference = totalAmount - newTransaction.CalculateDestinationAmount(parameters.TransactionInfo.Destination).ToDecimal(MoneyUnit.BTC);
var amountDifferencePercentage = amountDifference / totalAmount;

if (amountDifferencePercentage <= MaximumDifferenceTolerance && (canModifyTransactionAmount || amountDifference == 0m))
{
var differenceFiat = GetDifferenceFiat(transactionInfo, newTransaction, usdExchangeRate);
var differenceFiatText = GetDifferenceFiatText(differenceFiat);
fullPrivacySuggestion = new FullPrivacySuggestion(newTransaction, amountDifference, differenceFiatText, allPrivateCoin, isChangeless);
var (differenceBtc, differenceFiat) = GetDifference(parameters.TransactionInfo, newTransaction, usdExchangeRate);
var differenceText = GetDifferenceText(differenceBtc);
var differenceAmountText = GetDifferenceAmountText(differenceBtc, differenceFiat);
fullPrivacySuggestion = new FullPrivacySuggestion(newTransaction, amountDifference, differenceText, differenceAmountText, allPrivateCoin, isChangeless);
yield return fullPrivacySuggestion;
}
}

// Do not calculate the better privacy option when the full privacy option has the same amount.
// This is only possible if the user makes a silly selection with coin control.
if (fullPrivacySuggestion is { } sug && sug.Difference == 0m)
{
yield break;
}

var coins = allPrivateCoin.Union(allSemiPrivateCoin).ToArray();
if (foundNonPrivate && allSemiPrivateCoin.Length != 0 &&
TryCreateTransaction(transactionInfo, coins, out newTransaction, out isChangeless))
TryCreateTransaction(parameters.TransactionInfo, coins, out newTransaction, out isChangeless))
{
var amountDifference = totalAmount - newTransaction.CalculateDestinationAmount(transactionInfo.Destination).ToDecimal(MoneyUnit.BTC);
var amountDifference = totalAmount - newTransaction.CalculateDestinationAmount(parameters.TransactionInfo.Destination).ToDecimal(MoneyUnit.BTC);
var amountDifferencePercentage = amountDifference / totalAmount;

if (amountDifferencePercentage <= MaximumDifferenceTolerance && (canModifyTransactionAmount || amountDifference == 0m))
{
var differenceFiat = GetDifferenceFiat(transactionInfo, newTransaction, usdExchangeRate);
var differenceFiatText = GetDifferenceFiatText(differenceFiat);
yield return new BetterPrivacySuggestion(newTransaction, differenceFiatText, coins, isChangeless);
var (btcDifference, fiatDifference) = GetDifference(parameters.TransactionInfo, newTransaction, usdExchangeRate);
var differenceText = GetDifferenceText(btcDifference);
var differenceAmountText = GetDifferenceAmountText(btcDifference, fiatDifference);
yield return new BetterPrivacySuggestion(newTransaction, differenceText, differenceAmountText, coins, isChangeless);

Check notice on line 213 in WalletWasabi.Fluent/Models/Transactions/PrivacySuggestionsModel.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (master)

ℹ Getting worse: Complex Method

VerifyPrivacyLevel increases in cyclomatic complexity from 22 to 23, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
}
}
}

private IEnumerable<PrivacyItem> VerifyConsolidation(BuildTransactionResult originalTransaction)
private IEnumerable<PrivacyItem> VerifyConsolidation(Parameters parameters)
{
var consolidatedCoins = originalTransaction.SpentCoins.Count();

var consolidatedCoins = parameters.Transaction.SpentCoins.Count();
if (consolidatedCoins > ConsolidationTolerance)
{
yield return new ConsolidationWarning(ConsolidationTolerance);
}
}

private IEnumerable<PrivacyItem> VerifyUnconfirmedInputs(BuildTransactionResult transaction)
private IEnumerable<PrivacyItem> VerifyUnconfirmedInputs(Parameters parameters)
{
if (transaction.SpendsUnconfirmed)
if (parameters.Transaction.SpendsUnconfirmed)
{
yield return new UnconfirmedFundsWarning();
}
}

private IEnumerable<PrivacyItem> VerifyCoinjoiningInputs(BuildTransactionResult transaction)
private IEnumerable<PrivacyItem> VerifyCoinjoiningInputs(Parameters parameters)
{
if (transaction.SpendsCoinjoining)
if (parameters.Transaction.SpendsCoinjoining)
{
yield return new CoinjoiningFundsWarning();
}
}

private async IAsyncEnumerable<PrivacyItem> VerifyChangeAsync(TransactionInfo info, BuildTransactionResult transaction, CancellationTokenSource linkedCts)
private async IAsyncEnumerable<PrivacyItem> VerifyChangeAsync(Parameters parameters, CancellationTokenSource linkedCts)
{
var hasChange = transaction.InnerWalletOutputs.Any(x => x.ScriptPubKey != info.Destination.ScriptPubKey);
var hasChange = parameters.Transaction.InnerWalletOutputs.Any(x => x.ScriptPubKey != parameters.TransactionInfo.Destination.ScriptPubKey);

if (hasChange)
{
yield return new CreatesChangeWarning();

if (!info.IsFixedAmount && !info.IsPayJoin)
if (parameters.IncludeSuggestions && !parameters.TransactionInfo.IsFixedAmount && !parameters.TransactionInfo.IsPayJoin)

Check warning on line 251 in WalletWasabi.Fluent/Models/Transactions/PrivacySuggestionsModel.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (master)

❌ New issue: Complex Conditional

VerifyChangeAsync has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
{
var suggestions = await CreateChangeAvoidanceSuggestionsAsync(info, transaction, linkedCts).ConfigureAwait(false);
var suggestions = await CreateChangeAvoidanceSuggestionsAsync(parameters.TransactionInfo, parameters.Transaction, linkedCts).ConfigureAwait(false);
foreach (var suggestion in suggestions)
{
yield return suggestion;
Expand Down Expand Up @@ -347,8 +358,13 @@

if (transaction is not null)
{
var differenceFiat = GetDifferenceFiat(transactionInfo, transaction, usdExchangeRate);
yield return new ChangeAvoidanceSuggestion(transaction, differenceFiat, GetDifferenceFiatText(differenceFiat), IsMore: differenceFiat > 0, IsLess: differenceFiat < 0);
var (btcDifference, fiatDifference) = GetDifference(transactionInfo, transaction, usdExchangeRate);
var differenceText = GetDifferenceText(btcDifference);
var differenceAmountText = GetDifferenceAmountText(btcDifference, fiatDifference);
var isMore = fiatDifference > 0;
var isLess = fiatDifference < 0;

yield return new ChangeAvoidanceSuggestion(transaction, fiatDifference, differenceText, differenceAmountText, isMore, isLess);
}
}
}
Expand Down Expand Up @@ -396,25 +412,33 @@
return true;
}

private decimal GetDifferenceFiat(TransactionInfo transactionInfo, BuildTransactionResult transaction, decimal usdExchangeRate)
private (decimal BtcDifference, decimal FiatDifference) GetDifference(TransactionInfo transactionInfo, BuildTransactionResult transaction, decimal usdExchangeRate)
{
var originalAmount = transactionInfo.Amount.ToDecimal(MoneyUnit.BTC);
var totalAmount = transaction.CalculateDestinationAmount(transactionInfo.Destination);
var total = totalAmount.ToDecimal(MoneyUnit.BTC);
var btcDifference = total - originalAmount;
var fiatTotal = total * usdExchangeRate;
var fiatOriginal = originalAmount * usdExchangeRate;
var fiatDifference = fiatTotal - fiatOriginal;

return fiatDifference;
return (btcDifference, fiatDifference);
}

private string GetDifferenceFiatText(decimal fiatDifference)
private string GetDifferenceText(decimal btcDifference)
soosr marked this conversation as resolved.
Show resolved Hide resolved
{
return fiatDifference switch
return btcDifference switch
{
> 0 => $"{fiatDifference.ToUsd()} more",
< 0 => $"{Math.Abs(fiatDifference).ToUsd()} less",
> 0 => "more",
< 0 => "less",
_ => "the same amount"
};
}

private string GetDifferenceAmountText(decimal btcDifference, decimal fiatDifference)
{
return $"{Math.Abs(btcDifference).FormattedBtc()} BTC {Math.Abs(fiatDifference).ToUsdAproxBetweenParens()}";
}

private record Parameters(TransactionInfo TransactionInfo, BuildTransactionResult Transaction, bool IncludeSuggestions);
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
using DynamicData;
using ReactiveUI;
using System.Collections;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using System.Reactive.Subjects;
using System.Threading;
using System.Threading.Tasks;
using WalletWasabi.Blockchain.TransactionBuilding;
Expand All @@ -11,6 +16,7 @@ namespace WalletWasabi.Fluent.ViewModels.Wallets.Send;
public partial class PrivacySuggestionsFlyoutViewModel : ViewModelBase
{
private readonly PrivacySuggestionsModel _privacySuggestionsModel;
private readonly Subject<IEnumerable<PrivacyWarning>> _previewWarnings = new();

[AutoNotify] private PrivacySuggestion? _previewSuggestion;
[AutoNotify] private PrivacySuggestion? _selectedSuggestion;
Expand All @@ -28,6 +34,27 @@ public PrivacySuggestionsFlyoutViewModel(Wallet wallet)

public ObservableCollection<PrivacyWarning> Warnings { get; } = new();
public ObservableCollection<PrivacySuggestion> Suggestions { get; } = new();
public IObservable<IEnumerable<PrivacyWarning>> PreviewWarnings => _previewWarnings;

public async Task UpdatePreviewWarningsAsync(TransactionInfo info, BuildTransactionResult transaction, CancellationToken cancellationToken)
{
var previewWarningList = new List<PrivacyWarning>();

await foreach (var item in _privacySuggestionsModel.BuildPrivacySuggestionsAsync(info, transaction, cancellationToken, includeSuggestions: false))
{
if (item is PrivacyWarning warning)
{
previewWarningList.Add(warning);
}
}

_previewWarnings.OnNext(previewWarningList);
}

public void ClearPreviewWarnings()
{
_previewWarnings.OnNext(Warnings);
}

/// <remarks>Method supports being called multiple times. In that case the last call cancels the previous one.</remarks>
public async Task BuildPrivacySuggestionsAsync(TransactionInfo info, BuildTransactionResult transaction, CancellationToken cancellationToken)
Expand All @@ -42,7 +69,7 @@ public async Task BuildPrivacySuggestionsAsync(TransactionInfo info, BuildTransa

IsBusy = true;

await foreach (var item in _privacySuggestionsModel.BuildPrivacySuggestionsAsync(info, transaction, cancellationToken))
await foreach (var item in _privacySuggestionsModel.BuildPrivacySuggestionsAsync(info, transaction, cancellationToken, includeSuggestions: true))
{
if (item is PrivacyWarning warning)
{
Expand Down