From b8a4faf090409b850225d4b10e66ad8477ce3d8e Mon Sep 17 00:00:00 2001 From: erwan-joly Date: Mon, 31 Aug 2026 16:56:01 +1200 Subject: [PATCH] fix(exchange): an offered item that left the inventory was duplicated PlayerStateComponent declares InShop and InExchange, the ECS generates working setters for both, and nothing ever assigned either. They were permanently false, so five guards were inert: WorldPacketHandlingStrategy the Scope.InTrade gate RemovePacketHandler dropping an item mid-trade WearHandler equipping one BiPacketHandler destroying one VehicleHandler mounting ProcessExchange builds the destination item fresh rather than moving it, and InventoryService is a ConcurrentDictionary, so removing an item that has since gone returns false without complaint. Offer an item, drop it, confirm: the receiver gets a real item and the giver keeps one. The lifecycle now owns the flag - OpenExchange and CloseExchange set it for both parties - rather than seven call sites each remembering to, which is how it came to never be set at all. ProcessExchange also confirms every offer is still present at the offered amount before anything is created, so a trade whose subject vanished transfers nothing instead of half of it. --- .../ExchangeService/ExchangeService.cs | 34 ++++++++++++++++++- .../ExchangeService/ExchangeServiceTests.cs | 31 +++++++++++++++-- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/src/NosCore.GameObject/Services/ExchangeService/ExchangeService.cs b/src/NosCore.GameObject/Services/ExchangeService/ExchangeService.cs index 9563cff4b..7aceeb252 100644 --- a/src/NosCore.GameObject/Services/ExchangeService/ExchangeService.cs +++ b/src/NosCore.GameObject/Services/ExchangeService/ExchangeService.cs @@ -11,6 +11,7 @@ using NosCore.GameObject.Ecs.Extensions; using NosCore.GameObject.Ecs.Interfaces; using NosCore.GameObject.Networking.ClientSession; +using NosCore.GameObject.Services.BroadcastService; using NosCore.GameObject.Services.InventoryService; using NosCore.GameObject.Services.ItemGenerationService; using NosCore.Packets.Enumerations; @@ -28,7 +29,8 @@ namespace NosCore.GameObject.Services.ExchangeService { public class ExchangeService(IItemGenerationService itemBuilderService, IOptions worldConfiguration, ILogger logger, IExchangeRequestRegistry exchangeRegistry, - ILogLanguageLocalizer logLanguage, IGameLanguageLocalizer gameLanguageLocalizer) + ILogLanguageLocalizer logLanguage, IGameLanguageLocalizer gameLanguageLocalizer, + ISessionRegistry sessionRegistry) : IExchangeService { public void SetGold(long visualId, long gold, long bankGold) @@ -204,12 +206,25 @@ public bool CheckExchange(long visualId, long targetId) logger.LogError(logLanguage[LogLanguageKey.TRY_REMOVE_FAILED], data.Value); } + SetInExchange(data.Key, false); + SetInExchange(data.Value, false); + return new ExcClosePacket { Type = resultType }; } + // The lifecycle owns the flag. Set at the seven call sites instead, it was simply + // never set at all, which left every InExchangeOrShop guard inert. + private void SetInExchange(long visualId, bool value) + { + if (sessionRegistry.TryGetCharacter(c => c.VisualId == visualId, out var character)) + { + character.InExchange = value; + } + } + public bool OpenExchange(long visualId, long targetVisualId) { if (CheckExchange(visualId) || CheckExchange(targetVisualId)) @@ -222,6 +237,8 @@ public bool OpenExchange(long visualId, long targetVisualId) exchangeRegistry.SetExchangeRequest(targetVisualId, visualId); exchangeRegistry.SetExchangeData(visualId, new ExchangeData()); exchangeRegistry.SetExchangeData(targetVisualId, new ExchangeData()); + SetInExchange(visualId, true); + SetInExchange(targetVisualId, true); return true; } @@ -257,6 +274,21 @@ bool IsFullTransfer } } + // The offer captured these references when the item was put in the window. Confirm + // each is still there, at that amount, before anything is created: the destination + // item is built fresh, so a source that has since gone would duplicate it. + foreach (var transfer in pendingTransfers) + { + if (!transfer.OriginInventory.TryGetValue(transfer.OriginalItem.ItemInstanceId, out var live) + || live.ItemInstance == null + || live.ItemInstance.ItemVNum != transfer.OriginalItem.ItemInstance.ItemVNum + || live.ItemInstance.Amount < transfer.Amount) + { + logger.LogError(logLanguage[LogLanguageKey.INVALID_EXCHANGE]); + return new List>(); + } + } + var addedItems = new List<(IInventoryService Inventory, InventoryItemInstance Item)>(); foreach (var transfer in pendingTransfers) diff --git a/test/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cs b/test/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cs index 745994c9b..b8ecb7d69 100644 --- a/test/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cs +++ b/test/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cs @@ -56,7 +56,8 @@ public void Setup() }; ItemProvider = new GameObject.Services.ItemGenerationService.ItemGenerationService(items, NullLoggerFactory.Instance, TestHelpers.Instance.LogLanguageLocalizer); - ExchangeProvider = new GameObject.Services.ExchangeService.ExchangeService(ItemProvider, WorldConfiguration, NullLogger.Instance, new ExchangeRequestRegistry(), TestHelpers.Instance.LogLanguageLocalizer, TestHelpers.Instance.GameLanguageLocalizer); + ExchangeProvider = new GameObject.Services.ExchangeService.ExchangeService(ItemProvider, WorldConfiguration, NullLogger.Instance, new ExchangeRequestRegistry(), TestHelpers.Instance.LogLanguageLocalizer, TestHelpers.Instance.GameLanguageLocalizer, + TestHelpers.Instance.SessionRegistry); } [TestMethod] @@ -183,7 +184,8 @@ private async Task SessionAndTargetAreSetUp() _sessionB = await TestHelpers.Instance.GenerateSessionAsync(); _realExchange = new GameObject.Services.ExchangeService.ExchangeService( ItemProvider!, WorldConfiguration!, NullLogger.Instance, new ExchangeRequestRegistry(), - TestHelpers.Instance.LogLanguageLocalizer, TestHelpers.Instance.GameLanguageLocalizer); + TestHelpers.Instance.LogLanguageLocalizer, TestHelpers.Instance.GameLanguageLocalizer, + TestHelpers.Instance.SessionRegistry); _realExchange.OpenExchange(_sessionA.Character.CharacterId, _sessionB.Character.VisualId); } @@ -332,5 +334,30 @@ private void ProcessExchangeShouldReturnItemsForBoth() var itemList = ExchangeProvider.ProcessExchange(1, 2, inventory1, inventory2); Assert.IsTrue((itemList.Count(s => s.Key == 1) == 2) && (itemList.Count(s => s.Key == 2) == 2)); } + + [TestMethod] + public void AnOfferedItemThatLeftTheInventoryDoesNotReachTheOtherSide() + { + IInventoryService giver = + new GameObject.Services.InventoryService.InventoryService(new List { new Item { VNum = 1012, Type = NoscorePocketType.Main } }, + WorldConfiguration!, NullLogger.Instance); + IInventoryService receiver = + new GameObject.Services.InventoryService.InventoryService(new List { new Item { VNum = 1012, Type = NoscorePocketType.Main } }, + WorldConfiguration!, NullLogger.Instance); + + var offered = giver.AddItemToPocket(InventoryItemInstance.Create(ItemProvider!.Create(1012, 1), 0))!.First(); + + ExchangeProvider!.OpenExchange(1, 2); + ExchangeProvider.AddItems(1, offered, 1); + + // The destination item is built fresh rather than moved, so dropping the offered + // item after putting it in the window used to hand over a second copy of it. + giver.Remove(offered.ItemInstanceId); + + var itemList = ExchangeProvider.ProcessExchange(1, 2, giver, receiver); + + Assert.AreEqual(0, itemList.Count, "a vanished offer must not transfer"); + Assert.AreEqual(0, receiver.Count, "the receiver must not gain a duplicate"); + } } }