Fix currency duplication bug (#32524)

Content.Server/Implants/SubdermalImplantSystem.cs

@@ -74,14 +74,12 @@ public sealed class SubdermalImplantSystem : SharedSubdermalImplantSystem
             return;
 
         // same as store code, but message is only shown to yourself
-        args.Handled = _store.TryAddCurrency(_store.GetCurrencyValue(args.Used, currency), uid, store);
-
-        if (!args.Handled)
+        if (!_store.TryAddCurrency((args.Used, currency), (uid, store)))
             return;
 
+        args.Handled = true;
         var msg = Loc.GetString("store-currency-inserted-implant", ("used", args.Used));
         _popup.PopupEntity(msg, args.User, args.User);
-        QueueDel(args.Used);
     }
 
     private void OnFreedomImplant(EntityUid uid, SubdermalImplantComponent component, UseFreedomImplantEvent args)

Content.Server/Stack/StackSystem.cs

@@ -100,6 +100,13 @@ namespace Content.Server.Stack
         /// </summary>
         public List<EntityUid> SpawnMultiple(string entityPrototype, int amount, EntityCoordinates spawnPosition)
         {
+            if (amount <= 0)
+            {
+                Log.Error(
+                    $"Attempted to spawn an invalid stack: {entityPrototype}, {amount}. Trace: {Environment.StackTrace}");
+                return new();
+            }
+
             var spawns = CalculateSpawns(entityPrototype, amount);
 
             var spawnedEnts = new List<EntityUid>();
@@ -116,6 +123,13 @@ namespace Content.Server.Stack
         /// <inheritdoc cref="SpawnMultiple(string,int,EntityCoordinates)"/>
         public List<EntityUid> SpawnMultiple(string entityPrototype, int amount, EntityUid target)
         {
+            if (amount <= 0)
+            {
+                Log.Error(
+                    $"Attempted to spawn an invalid stack: {entityPrototype}, {amount}. Trace: {Environment.StackTrace}");
+                return new();
+            }
+
             var spawns = CalculateSpawns(entityPrototype, amount);
 
             var spawnedEnts = new List<EntityUid>();

Content.Server/Store/Components/CurrencyComponent.cs

@@ -8,6 +8,11 @@ namespace Content.Server.Store.Components;
 /// Identifies a component that can be inserted into a store
 /// to increase its balance.
 /// </summary>
+/// <remarks>
+/// Note that if this entity is a stack of items, then this is meant to represent the value per stack item, not
+/// the whole stack. This also means that in general, the actual value should not be modified from the initial
+/// prototype value because otherwise stack merging/splitting may modify the total value.
+/// </remarks>
 [RegisterComponent]
 public sealed partial class CurrencyComponent : Component
 {
@@ -16,6 +21,12 @@ public sealed partial class CurrencyComponent : Component
     /// The string is the currency type that will be added.
     /// The FixedPoint2 is the value of each individual currency entity.
     /// </summary>
+    /// <remarks>
+    /// Note that if this entity is a stack of items, then this is meant to represent the value per stack item, not
+    /// the whole stack. This also means that in general, the actual value should not be modified from the initial
+    /// prototype value
+    /// because otherwise stack merging/splitting may modify the total value.
+    /// </remarks>
     [ViewVariables(VVAccess.ReadWrite)]
     [DataField("price", customTypeSerializer: typeof(PrototypeIdDictionarySerializer<FixedPoint2, CurrencyPrototype>))]
     public Dictionary<string, FixedPoint2> Price = new();

Content.Server/Store/Systems/StoreSystem.Ui.cs

@@ -283,6 +283,9 @@ public sealed partial class StoreSystem
     /// </remarks>
     private void OnRequestWithdraw(EntityUid uid, StoreComponent component, StoreRequestWithdrawMessage msg)
     {
+        if (msg.Amount <= 0)
+            return;
+
         //make sure we have enough cash in the bank and we actually support this currency
         if (!component.Balance.TryGetValue(msg.Currency, out var currentAmount) || currentAmount < msg.Amount)
             return;
@@ -306,7 +309,8 @@ public sealed partial class StoreSystem
             var cashId = proto.Cash[value];
             var amountToSpawn = (int) MathF.Floor((float) (amountRemaining / value));
             var ents = _stack.SpawnMultiple(cashId, amountToSpawn, coordinates);
-            _hands.PickupOrDrop(buyer, ents.First());
+            if (ents.FirstOrDefault() is {} ent)
+                _hands.PickupOrDrop(buyer, ent);
             amountRemaining -= value * amountToSpawn;
         }
 

Content.Server/Store/Systems/StoreSystem.cs

@@ -92,14 +92,12 @@ public sealed partial class StoreSystem : EntitySystem
         if (ev.Cancelled)
             return;
 
-        args.Handled = TryAddCurrency(GetCurrencyValue(uid, component), args.Target.Value, store);
+        if (!TryAddCurrency((uid, component), (args.Target.Value, store)))
+            return;
 
-        if (args.Handled)
-        {
+        args.Handled = true;
         var msg = Loc.GetString("store-currency-inserted", ("used", args.Used), ("target", args.Target));
         _popup.PopupEntity(msg, args.Target.Value, args.User);
-            QueueDel(args.Used);
-        }
     }
 
     private void OnImplantActivate(EntityUid uid, StoreComponent component, OpenUplinkImplantEvent args)
@@ -111,6 +109,10 @@ public sealed partial class StoreSystem : EntitySystem
     /// Gets the value from an entity's currency component.
     /// Scales with stacks.
     /// </summary>
+    /// <remarks>
+    /// If this result is intended to be used with <see cref="TryAddCurrency(Robust.Shared.GameObjects.Entity{Content.Server.Store.Components.CurrencyComponent?},Robust.Shared.GameObjects.Entity{Content.Shared.Store.Components.StoreComponent?})"/>,
+    /// consider using <see cref="TryAddCurrency(Robust.Shared.GameObjects.Entity{Content.Server.Store.Components.CurrencyComponent?},Robust.Shared.GameObjects.Entity{Content.Shared.Store.Components.StoreComponent?})"/> instead to ensure that the currency is consumed in the process.
+    /// </remarks>
     /// <param name="uid"></param>
     /// <param name="component"></param>
     /// <returns>The value of the currency</returns>
@@ -121,19 +123,34 @@ public sealed partial class StoreSystem : EntitySystem
     }
 
     /// <summary>
-    /// Tries to add a currency to a store's balance.
+    /// Tries to add a currency to a store's balance. Note that if successful, this will consume the currency in the process.
     /// </summary>
-    /// <param name="currencyEnt"></param>
-    /// <param name="storeEnt"></param>
-    /// <param name="currency">The currency to add</param>
-    /// <param name="store">The store to add it to</param>
-    /// <returns>Whether or not the currency was succesfully added</returns>
-    [PublicAPI]
-    public bool TryAddCurrency(EntityUid currencyEnt, EntityUid storeEnt, StoreComponent? store = null, CurrencyComponent? currency = null)
+    public bool TryAddCurrency(Entity<CurrencyComponent?> currency, Entity<StoreComponent?> store)
     {
-        if (!Resolve(currencyEnt, ref currency) || !Resolve(storeEnt, ref store))
+        if (!Resolve(currency.Owner, ref currency.Comp))
             return false;
-        return TryAddCurrency(GetCurrencyValue(currencyEnt, currency), storeEnt, store);
+
+        if (!Resolve(store.Owner, ref store.Comp))
+            return false;
+
+        var value = currency.Comp.Price;
+        if (TryComp(currency.Owner, out StackComponent? stack) && stack.Count != 1)
+        {
+            value = currency.Comp.Price
+                .ToDictionary(v => v.Key, p => p.Value * stack.Count);
+        }
+
+        if (!TryAddCurrency(value, store, store.Comp))
+            return false;
+
+        // Avoid having the currency accidentally be re-used. E.g., if multiple clients try to use the currency in the
+        // same tick
+        currency.Comp.Price.Clear();
+        if (stack != null)
+            _stack.SetCount(currency.Owner, 0, stack);
+
+        QueueDel(currency);
+        return true;
     }
 
     /// <summary>