From c0b7a8f643f220c8d387313558dcef726d64cc8e Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Tue, 10 Nov 2026 12:57:58 -0600 Subject: [PATCH] fix: check a trade offer against the items the player holds A trade used the LOT and the count from the offer as the player sent it. The offer is now resolved when it arrives and again when the trade completes: the LOT comes from the held item, the count stops at the held count, and an offer that names an unknown item, the same item twice or a count of 0 is refused. The other player only receives an item that was taken from the offering player. Co-Authored-By: Claude Fable 5.2 --- dGame/TradingManager.cpp | 48 +++++++++++++-- dGame/TradingManager.h | 22 +++++++ dGame/dGameMessages/TradeMessages.cpp | 19 +++++- .../dGameMessagesTests/CMakeLists.txt | 3 +- .../dGameMessagesTests/TradeOfferTests.cpp | 58 +++++++++++++++++++ 5 files changed, 143 insertions(+), 7 deletions(-) create mode 100644 tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp diff --git a/dGame/TradingManager.cpp b/dGame/TradingManager.cpp index 938f380c0..dcc822562 100644 --- a/dGame/TradingManager.cpp +++ b/dGame/TradingManager.cpp @@ -13,7 +13,9 @@ #include "CharacterComponent.h" #include "MissionComponent.h" #include "eMissionTaskType.h" +#include #include +#include namespace { std::unique_ptr g_EmptyTrade; @@ -27,6 +29,26 @@ Trade::Trade(LWOOBJID tradeId, LWOOBJID participantA, LWOOBJID participantB) { m_ParticipantB = participantB; } +std::optional> Trade::ResolveOffer( + const std::vector& offered, + const std::function(LWOOBJID)>& findHeld) { + std::vector resolved; + resolved.reserve(offered.size()); + std::set seen; + + for (const auto& tradeItem : offered) { + // The client keeps its offer by item ID, so an item is only ever in it once + if (tradeItem.itemCount == 0 || !seen.insert(tradeItem.itemId).second) return std::nullopt; + + const auto held = findHeld(tradeItem.itemId); + if (!held || held->count == 0) return std::nullopt; + + resolved.push_back({ tradeItem.itemId, held->lot, std::min(tradeItem.itemCount, held->count) }); + } + + return resolved; +} + Trade::~Trade() { } @@ -149,6 +171,20 @@ void Trade::Complete() { return; } + // The inventories may have changed since the offers were made, so check the offers against them again + const auto holds = [](const InventoryComponent* const inventory) { + return [inventory](const LWOOBJID itemId) -> std::optional { + const auto* const item = inventory->FindItemById(itemId); + if (!item) return std::nullopt; + return HeldTradeItem{ item->GetLot(), item->GetCount() }; + }; + }; + + if (ResolveOffer(m_ItemsA, holds(inventoryA)) != m_ItemsA || ResolveOffer(m_ItemsB, holds(inventoryB)) != m_ItemsB) { + LOG("The items offered by %s and %s no longer match what they hold, aborting trade", characterA->GetName().c_str(), characterB->GetName().c_str()); + return; + } + for (const auto& tradeItem : m_ItemsA) { auto* itemToRemove = inventoryA->FindItemById(tradeItem.itemId); if (itemToRemove) { @@ -194,7 +230,7 @@ void Trade::Complete() { InventoryComponent* to, const LWOOBJID fromCharacter, const LWOOBJID toCharacter) { for (const auto& tradeItem : items) { auto* itemToRemove = from->FindItemById(tradeItem.itemId); - if (!itemToRemove) continue; + if (!itemToRemove || itemToRemove->GetCount() < tradeItem.itemCount) continue; // A whole item keeps its data (subkey, config) but gets a new object id, as upstream: reusing the id would // leave two objects with one id if the server stopped between the two players' saves. A stack merges into @@ -203,17 +239,19 @@ void Trade::Complete() { const auto config = itemToRemove->GetConfig(); const auto subKey = itemToRemove->GetSubKey(); const auto bound = itemToRemove->GetBound(); + // Taking the whole stack removes the item, so read its LOT first + const LOT lot = itemToRemove->GetLot(); itemToRemove->SetCount(itemToRemove->GetCount() - tradeItem.itemCount); - fromMissions->Progress(eMissionTaskType::GATHER, tradeItem.itemLot, LWOOBJID_EMPTY, "", -static_cast(tradeItem.itemCount)); + fromMissions->Progress(eMissionTaskType::GATHER, lot, LWOOBJID_EMPTY, "", -static_cast(tradeItem.itemCount)); // Live sent the trade's ID with the traded items ReceiveItemOptions options{}; options.metrics.transactionId = tradeId; const auto received = whole - ? to->ReceiveItem(LWOOBJID_EMPTY, tradeItem.itemLot, tradeItem.itemCount, eLootSourceType::TRADE, config, subKey, bound, options) - : to->ReceiveItem(LWOOBJID_EMPTY, tradeItem.itemLot, tradeItem.itemCount, eLootSourceType::TRADE, {}, LWOOBJID_EMPTY, false, options); + ? to->ReceiveItem(LWOOBJID_EMPTY, lot, tradeItem.itemCount, eLootSourceType::TRADE, config, subKey, bound, options) + : to->ReceiveItem(LWOOBJID_EMPTY, lot, tradeItem.itemCount, eLootSourceType::TRADE, {}, LWOOBJID_EMPTY, false, options); EconomyLedger::RecordTransfer({ .method = IEconomyLedger::eTransferMethod::TRADE, .itemId = tradeItem.itemId, .newItemId = received.id, - .lot = tradeItem.itemLot, .count = tradeItem.itemCount, .fromCharacter = fromCharacter, .toCharacter = toCharacter, .zone = zone, + .lot = lot, .count = tradeItem.itemCount, .fromCharacter = fromCharacter, .toCharacter = toCharacter, .zone = zone, .merged = received.merged }); } }; diff --git a/dGame/TradingManager.h b/dGame/TradingManager.h index d9984a5b1..5924acd14 100644 --- a/dGame/TradingManager.h +++ b/dGame/TradingManager.h @@ -2,17 +2,39 @@ #include "Entity.h" +#include #include #include +#include +#include struct TradeItem { LWOOBJID itemId; LOT itemLot; uint32_t itemCount; + + bool operator==(const TradeItem& other) const = default; +}; + +// What a player holds of an item they offered +struct HeldTradeItem { + LOT lot; + uint32_t count; }; class Trade { public: + /** + * Checks the items a player offered against the items they hold. + * @param offered the items as the player sent them + * @param findHeld gives the held item with an ID, or nothing if the player has no such item + * @return the offer with every LOT taken from the held item and every count limited to the held count, + * or nothing if the offer names an item the player does not hold, the same item twice or a count of 0 + */ + static std::optional> ResolveOffer( + const std::vector& offered, + const std::function(LWOOBJID)>& findHeld); + explicit Trade(LWOOBJID tradeId, LWOOBJID participantA, LWOOBJID participantB); ~Trade(); diff --git a/dGame/dGameMessages/TradeMessages.cpp b/dGame/dGameMessages/TradeMessages.cpp index 344fa8a71..48df485f6 100644 --- a/dGame/dGameMessages/TradeMessages.cpp +++ b/dGame/dGameMessages/TradeMessages.cpp @@ -9,6 +9,8 @@ #include "ePermissionMap.h" #include "Game.h" #include "GeneralUtils.h" +#include "InventoryComponent.h" +#include "Item.h" #include "Logger.h" #include "TradingManager.h" #include "User.h" @@ -233,8 +235,23 @@ namespace GameMessages { if (trade == nullptr) return; + const auto* const inventoryComponent = entity.GetComponent(); + if (!inventoryComponent) return; + + // The LOT and the count the trade works with come from the items the player holds + const auto resolvedItems = Trade::ResolveOffer(items, [inventoryComponent](const LWOOBJID itemId) -> std::optional { + const auto* const item = inventoryComponent->FindItemById(itemId); + if (!item) return std::nullopt; + return HeldTradeItem{ item->GetLot(), item->GetCount() }; + }); + + if (!resolvedItems) { + LOG("Trade update from (%llu) does not match the items they hold, ignoring it", entity.GetObjectID()); + return; + } + trade->SetCoins(entity.GetObjectID(), i64Currency); - trade->SetItems(entity.GetObjectID(), items); + trade->SetItems(entity.GetObjectID(), *resolvedItems); trade->SendUpdateToOther(entity.GetObjectID()); } diff --git a/tests/dGameTests/dGameMessagesTests/CMakeLists.txt b/tests/dGameTests/dGameMessagesTests/CMakeLists.txt index d4509a568..2b5eec9b6 100644 --- a/tests/dGameTests/dGameMessagesTests/CMakeLists.txt +++ b/tests/dGameTests/dGameMessagesTests/CMakeLists.txt @@ -19,7 +19,8 @@ SET(DGAMEMESSAGES_TESTS "HotPropertyMessageTests.cpp" "ClientMessagesTests.cpp" "LoadSequenceTests.cpp" - "ChatRoutingTests.cpp") + "ChatRoutingTests.cpp" + "TradeOfferTests.cpp") # Get the folder name and prepend it to the files above get_filename_component(thisFolderName ${CMAKE_CURRENT_SOURCE_DIR} NAME) diff --git a/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp b/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp new file mode 100644 index 000000000..696b93c8d --- /dev/null +++ b/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp @@ -0,0 +1,58 @@ +#include + +#include + +#include "TradingManager.h" + +namespace { + const std::map g_Held = { + { 100, { 1727, 5 } }, + { 200, { 3040, 1 } }, + }; + + std::optional FindHeld(const LWOOBJID itemId) { + const auto it = g_Held.find(itemId); + if (it == g_Held.end()) return std::nullopt; + return it->second; + } +} + +TEST(TradeOfferTest, AnOfferOfHeldItemsIsKept) { + const std::vector offered = { { 100, 1727, 3 }, { 200, 3040, 1 } }; + const auto resolved = Trade::ResolveOffer(offered, FindHeld); + ASSERT_TRUE(resolved.has_value()); + EXPECT_EQ(*resolved, offered); +} + +TEST(TradeOfferTest, AnEmptyOfferIsKept) { + const auto resolved = Trade::ResolveOffer({}, FindHeld); + ASSERT_TRUE(resolved.has_value()); + EXPECT_TRUE(resolved->empty()); +} + +TEST(TradeOfferTest, TheLotComesFromTheHeldItem) { + const auto resolved = Trade::ResolveOffer({ { 100, 7777, 2 } }, FindHeld); + ASSERT_TRUE(resolved.has_value()); + ASSERT_EQ(resolved->size(), 1); + EXPECT_EQ(resolved->front().itemLot, 1727); + EXPECT_EQ(resolved->front().itemCount, 2); +} + +TEST(TradeOfferTest, TheCountStopsAtTheHeldCount) { + const auto resolved = Trade::ResolveOffer({ { 100, 1727, 4000000000U } }, FindHeld); + ASSERT_TRUE(resolved.has_value()); + ASSERT_EQ(resolved->size(), 1); + EXPECT_EQ(resolved->front().itemCount, 5); +} + +TEST(TradeOfferTest, AnItemOfferedTwiceIsRefused) { + EXPECT_FALSE(Trade::ResolveOffer({ { 100, 1727, 3 }, { 100, 1727, 3 } }, FindHeld).has_value()); +} + +TEST(TradeOfferTest, AnItemThePlayerDoesNotHoldIsRefused) { + EXPECT_FALSE(Trade::ResolveOffer({ { 300, 1727, 1 } }, FindHeld).has_value()); +} + +TEST(TradeOfferTest, ACountOfZeroIsRefused) { + EXPECT_FALSE(Trade::ResolveOffer({ { 100, 1727, 0 } }, FindHeld).has_value()); +}