From 24294561ae51b74feeba29bbcf5ceeaf1077655c Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Tue, 10 Nov 2026 15:39:43 -0600 Subject: [PATCH] fix: a changed trade offer takes both accepts back A player could change their offer after the other player gave the final accept, and the trade completed with the new offer on their own accept. Both clients drop their accepts when an offer changes, the one that changed it and the one that receives the update, so the trade now does the same and both players have to accept the offers as they are. Co-Authored-By: Claude Fable 5.2 --- dGame/TradingManager.cpp | 11 +++++ dGame/TradingManager.h | 10 +++++ dGame/dGameMessages/TradeMessages.cpp | 2 + .../dGameMessagesTests/TradeOfferTests.cpp | 43 +++++++++++++++++++ 4 files changed, 66 insertions(+) diff --git a/dGame/TradingManager.cpp b/dGame/TradingManager.cpp index a8423d9a8..a125cb453 100644 --- a/dGame/TradingManager.cpp +++ b/dGame/TradingManager.cpp @@ -160,6 +160,17 @@ void Trade::SetAccepted(LWOOBJID participant, bool value) { } } +bool Trade::GetAccepted(const LWOOBJID participant) const { + if (participant == m_ParticipantA) return m_AcceptedA; + if (participant == m_ParticipantB) return m_AcceptedB; + return false; +} + +void Trade::ResetAccepted() { + m_AcceptedA = false; + m_AcceptedB = false; +} + void Trade::Complete() { auto* entityA = GetParticipantAEntity(); auto* entityB = GetParticipantBEntity(); diff --git a/dGame/TradingManager.h b/dGame/TradingManager.h index fa427e3ea..d96a02a34 100644 --- a/dGame/TradingManager.h +++ b/dGame/TradingManager.h @@ -66,6 +66,16 @@ public: void SetItems(LWOOBJID participant, std::vector items); void SetAccepted(LWOOBJID participant, bool value); + /** + * Whether a participant has given their final accept + */ + bool GetAccepted(LWOOBJID participant) const; + + /** + * Takes back both final accepts. A changed offer does this, as both clients do when one arrives. + */ + void ResetAccepted(); + void Complete(); void Cancel(const LWOOBJID canceller); diff --git a/dGame/dGameMessages/TradeMessages.cpp b/dGame/dGameMessages/TradeMessages.cpp index 046c4f2c9..bf7c5d34b 100644 --- a/dGame/dGameMessages/TradeMessages.cpp +++ b/dGame/dGameMessages/TradeMessages.cpp @@ -256,6 +256,8 @@ namespace GameMessages { trade->SetCoins(entity.GetObjectID(), i64Currency); trade->SetItems(entity.GetObjectID(), *resolvedItems); + // Both clients take their accept back when an offer changes, so an accept only ever stands for the offers as they are + trade->ResetAccepted(); trade->SendUpdateToOther(entity.GetObjectID()); } diff --git a/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp b/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp index d6b53be3f..17c9de852 100644 --- a/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp +++ b/tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp @@ -2,6 +2,7 @@ #include +#include "GameDependencies.h" #include "TradingManager.h" namespace { @@ -12,6 +13,17 @@ namespace { { 400, { 1727, 2, false, true } }, }; + class TradeAcceptTest : public GameDependenciesTest { + protected: + void SetUp() override { + SetUpDependencies(); + } + + void TearDown() override { + TearDownDependencies(); + } + }; + std::optional FindHeld(const LWOOBJID itemId) { const auto it = g_Held.find(itemId); if (it == g_Held.end()) return std::nullopt; @@ -80,3 +92,34 @@ TEST(TradeOfferTest, AnOfferFillsTheSlotsOfTheTradeWindowAtMost) { offered.push_back({ Trade::MaxOfferedItems + 1, 1727, 1 }); EXPECT_FALSE(Trade::ResolveOffer(offered, findAny).has_value()); } + +TEST_F(TradeAcceptTest, OnlyTheFinalAcceptCounts) { + Trade trade(1, 10, 20); + EXPECT_FALSE(trade.GetAccepted(10)); + + trade.SetAccepted(10, true); + EXPECT_FALSE(trade.GetAccepted(10)); + + trade.SetAccepted(10, false); + EXPECT_TRUE(trade.GetAccepted(10)); + EXPECT_FALSE(trade.GetAccepted(20)); + // Not a participant + EXPECT_FALSE(trade.GetAccepted(30)); +} + +TEST_F(TradeAcceptTest, AChangedOfferTakesBothAcceptsBack) { + Trade trade(1, 10, 20); + trade.SetAccepted(10, false); + ASSERT_TRUE(trade.GetAccepted(10)); + + // What the update handler does once the new offer is stored + trade.SetItems(20, { { 100, 1727, 1 } }); + trade.ResetAccepted(); + EXPECT_FALSE(trade.GetAccepted(10)); + EXPECT_FALSE(trade.GetAccepted(20)); + + // The other player's accept alone does not complete the trade, both have to accept again + trade.SetAccepted(20, false); + EXPECT_TRUE(trade.GetAccepted(20)); + EXPECT_FALSE(trade.GetAccepted(10)); +}