Merge pull request #12576 from soosr/fix-leak

[Release] Fix coin model leak
zkSNACKs · Feb 29, 2024 · 938405e · 938405e
2 parents 167d023 + 708a7ee
commit 938405e
Show file tree

Hide file tree

Showing 6 changed files with 59 additions and 17 deletions.
diff --git a/WalletWasabi.Fluent/Models/Wallets/CoinModel.cs b/WalletWasabi.Fluent/Models/Wallets/CoinModel.cs
@@ -10,6 +10,7 @@ namespace WalletWasabi.Fluent.Models.Wallets;
 [AutoInterface]
 public partial class CoinModel : ReactiveObject
 {
+	private bool _subscribedToCoinChanges;
 	[AutoNotify] private bool _isExcludedFromCoinJoin;
 	[AutoNotify] private bool _isCoinJoinInProgress;
 	[AutoNotify] private bool _isBanned;
@@ -30,18 +31,16 @@ public CoinModel(SmartCoin coin, int anonScoreTarget)
 		BannedUntilUtc = coin.BannedUntilUtc;
 		ScriptType = ScriptType.FromEnum(coin.ScriptType);
 
-		this.WhenAnyValue(c => c.Coin.IsExcludedFromCoinJoin).BindTo(this, x => x.IsExcludedFromCoinJoin);
-		this.WhenAnyValue(c => c.Coin.Confirmed).BindTo(this, x => x.IsConfirmed);
-		this.WhenAnyValue(c => c.Coin.HdPubKey.AnonymitySet).Select(x => (int)x).BindTo(this, x => x.AnonScore);
-		this.WhenAnyValue(c => c.Coin.CoinJoinInProgress).BindTo(this, x => x.IsCoinJoinInProgress);
-		this.WhenAnyValue(c => c.Coin.IsBanned).BindTo(this, x => x.IsBanned);
-		this.WhenAnyValue(c => c.Coin.BannedUntilUtc).WhereNotNull().Subscribe(x => BannedUntilUtcToolTip = $"Can't participate in coinjoin until: {x:g}");
+		IsExcludedFromCoinJoin = coin.IsExcludedFromCoinJoin;
+		IsConfirmed = coin.Confirmed;
+		AnonScore = (int)coin.HdPubKey.AnonymitySet;
+		IsCoinJoinInProgress = coin.CoinJoinInProgress;
+		IsBanned = coin.IsBanned;
+		BannedUntilUtcToolTip = $"Can't participate in coinjoin until: {coin.BannedUntilUtc}";
 
-		this.WhenAnyValue(c => c.Coin.Height).Select(_ => Coin.GetConfirmations()).Subscribe(x =>
-		{
-			Confirmations = x;
-			ConfirmedToolTip = TextHelpers.GetConfirmationText(x);
-		});
+		var confirmations = coin.GetConfirmations();
+		Confirmations = confirmations;
+		ConfirmedToolTip = TextHelpers.GetConfirmationText(confirmations);
 	}
 
 	internal SmartCoin Coin { get; }
@@ -64,6 +63,31 @@ public CoinModel(SmartCoin coin, int anonScoreTarget)
 
 	public bool IsNonPrivate => PrivacyLevel == PrivacyLevel.NonPrivate;
 
+	/// <summary>Subscribes to property changes of underlying SmartCoin.</summary>
+	/// <remarks>This method is not thread safe. Make sure it's not called concurrently.</remarks>
+	public void SubscribeToCoinChanges()
+	{
+		if (_subscribedToCoinChanges)
+		{
+			return;
+		}
+
+		this.WhenAnyValue(c => c.Coin.IsExcludedFromCoinJoin).BindTo(this, x => x.IsExcludedFromCoinJoin);
+		this.WhenAnyValue(c => c.Coin.Confirmed).BindTo(this, x => x.IsConfirmed);
+		this.WhenAnyValue(c => c.Coin.HdPubKey.AnonymitySet).Select(x => (int)x).BindTo(this, x => x.AnonScore);
+		this.WhenAnyValue(c => c.Coin.CoinJoinInProgress).BindTo(this, x => x.IsCoinJoinInProgress);
+		this.WhenAnyValue(c => c.Coin.IsBanned).BindTo(this, x => x.IsBanned);
+		this.WhenAnyValue(c => c.Coin.BannedUntilUtc).WhereNotNull().Subscribe(x => BannedUntilUtcToolTip = $"Can't participate in coinjoin until: {x:g}");
+
+		this.WhenAnyValue(c => c.Coin.Height).Select(_ => Coin.GetConfirmations()).Subscribe(x =>
+		{
+			Confirmations = x;
+			ConfirmedToolTip = TextHelpers.GetConfirmationText(x);
+		});
+
+		_subscribedToCoinChanges = true;
+	}
+
 	public bool IsSameAddress(ICoinModel anotherCoin) => anotherCoin is CoinModel cm && cm.Coin.HdPubKey == Coin.HdPubKey;
 
 	// TODO: Leaky abstraction. This shouldn't exist.

diff --git a/WalletWasabi.Fluent/ViewModels/Dialogs/LabelEntryDialogViewModel.cs b/WalletWasabi.Fluent/ViewModels/Dialogs/LabelEntryDialogViewModel.cs
@@ -49,6 +49,7 @@ protected override void OnNavigatedTo(bool isInHistory, CompositeDisposable disp
 	{
 		base.OnNavigatedTo(isInHistory, disposables);
 
+		// TODO: why are we using this here and turning it into a Signal, instead of an event like _wallet.TransactionProcessed?
 		_wallet.Coins.List
 			.Connect()
 			.ToSignal()

diff --git a/WalletWasabi.Fluent/ViewModels/Wallets/Advanced/WalletCoins/WalletCoinsViewModel.cs b/WalletWasabi.Fluent/ViewModels/Wallets/Advanced/WalletCoins/WalletCoinsViewModel.cs
@@ -54,6 +54,7 @@ protected override void OnNavigatedTo(bool isInHistory, CompositeDisposable disp
 		var coinChanges =
 			_wallet.Coins.List
 				.Connect()
+				.OnItemAdded(c => c.SubscribeToCoinChanges()) // Subscribe to SmartCoin changes for dynamic updates
 				.TransformWithInlineUpdate(x => new WalletCoinViewModel(x), (_, _) => { })
 				.Replay(1)
 				.RefCount();

diff --git a/WalletWasabi.Fluent/ViewModels/Wallets/Home/Tiles/PrivacyControlTileViewModel.cs b/WalletWasabi.Fluent/ViewModels/Wallets/Home/Tiles/PrivacyControlTileViewModel.cs
@@ -31,7 +31,11 @@ private PrivacyControlTileViewModel(IWalletModel wallet)
 
 		PrivacyBar = new PrivacyBarViewModel(wallet);
 
-		var coinList = _wallet.Coins.List.Connect(suppressEmptyChangeSets: false);
+		var coinList =
+			_wallet.Coins.List
+						 .Connect(suppressEmptyChangeSets: false); // coinList here is not subscribed to SmartCoin changes.
+																   // Dynamic updates to SmartCoin properties won't be reflected in the UI.
+																   // See CoinModel.SubscribeToCoinChanges().
 
 		TotalAmount = coinList.Sum(set => set.Amount.ToDecimal(MoneyUnit.Satoshi));
 		PrivateAmount = coinList.Filter(x => x.IsPrivate, suppressEmptyChangeSets: false).Sum(set => set.Amount.ToDecimal(MoneyUnit.Satoshi));
@@ -55,9 +59,14 @@ protected override void OnActivated(CompositeDisposable disposables)
 	{
 		base.OnActivated(disposables);
 
+		var coinList =
+			_wallet.Coins.List                                      // coinList here is not subscribed to SmartCoin changes.
+						 .Connect(suppressEmptyChangeSets: false)   // Dynamic updates to SmartCoin properties won't be reflected in the UI.
+						 .ToCollection();                           // See CoinModel.SubscribeToCoinChanges().
+
 		_wallet.Privacy.Progress
 					   .CombineLatest(_wallet.Privacy.IsWalletPrivate)
-					   .CombineLatest(_wallet.Coins.List.Connect(suppressEmptyChangeSets: false).ToCollection())
+					   .CombineLatest(coinList)
 					   .Flatten()
 					   .Do(tuple =>
 					   {

diff --git a/WalletWasabi.Fluent/ViewModels/Wallets/Home/Tiles/PrivacyRing/PrivacyBarViewModel.cs b/WalletWasabi.Fluent/ViewModels/Wallets/Home/Tiles/PrivacyRing/PrivacyBarViewModel.cs
@@ -37,9 +37,9 @@ protected override void OnActivated(CompositeDisposable disposables)
 			.Subscribe()
 			.DisposeWith(disposables);
 
-		Wallet.Coins.List
-			.Connect(suppressEmptyChangeSets: false)
-			.ToCollection()
+		Wallet.Coins.List                                 // Wallet.Coins.List here is not subscribed to SmartCoin changes.
+			.Connect(suppressEmptyChangeSets: false)      // Dynamic updates to SmartCoin properties won't be reflected in the UI.
+			.ToCollection()                               // See CoinModel.SubscribeToCoinChanges().
 			.Subscribe(x => itemsSourceList.Edit(l => Update(l, x)))
 			.DisposeWith(disposables);
 	}

diff --git a/WalletWasabi.Fluent/ViewModels/Wallets/Home/Tiles/PrivacyRing/PrivacyRingViewModel.cs b/WalletWasabi.Fluent/ViewModels/Wallets/Home/Tiles/PrivacyRing/PrivacyRingViewModel.cs
@@ -76,9 +76,16 @@ protected override void OnNavigatedTo(bool isInHistory, CompositeDisposable disp
 				.Throttle(TimeSpan.FromMilliseconds(100))
 				.ToSignal();
 
+		var coinsList =
+			_wallet.Coins.List
+						 .Connect(suppressEmptyChangeSets: false)
+						 .OnItemAdded(c => c.SubscribeToCoinChanges()) // Subscribe to SmartCoin changes for dynamic updates
+						 .ToCollection()
+						 .Select(x => x.Distinct());
+
 		_wallet.Privacy.ProgressUpdated
 					   .Merge(sizeTrigger)
-					   .WithLatestFrom(_wallet.Coins.List.Connect(suppressEmptyChangeSets: false).ToCollection().Select(x => x.Distinct()))
+					   .WithLatestFrom(coinsList)
 					   .ObserveOn(RxApp.MainThreadScheduler)
 					   .Do(t => RenderRing(itemsSourceList, t.Second))
 					   .Subscribe()