From 8063177d4081af7622d66118ced26554136d3072 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 08:00:34 +0000 Subject: [PATCH 1/2] fix: trade from the player's live inventory Enchanting, renaming and brewing change items without the incremental inventory events the server copy relies on, so those items were missing from the trade picker and the client's Ready check cancelled trades whose offer no longer matched the live inventory. The client now sends its live inventory when a trade starts and before each Ready, and the server replaces its copy and drops offered items the snapshot no longer covers. Outdated offers unready instead of cancelling. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01SSW5HGXzi9kAVm4zdsSaid --- Code/client/Services/Generic/TradeService.cpp | 22 ++++++--- Code/client/Services/TradeService.h | 1 + Code/encoding/Messages/ClientMessageFactory.h | 3 +- .../Messages/TradeInventorySyncRequest.cpp | 22 +++++++++ .../Messages/TradeInventorySyncRequest.h | 21 +++++++++ Code/encoding/Opcodes.h | 1 + Code/encoding/Structs/TradeValidation.h | 31 ++++++++++++ Code/server/Services/TradeService.cpp | 47 ++++++++++++++++++- Code/server/Services/TradeService.h | 3 ++ Code/tests/trading.cpp | 38 +++++++++++++++ PLAYER_TRADING_POTIONS.md | 6 +++ 11 files changed, 186 insertions(+), 9 deletions(-) create mode 100644 Code/encoding/Messages/TradeInventorySyncRequest.cpp create mode 100644 Code/encoding/Messages/TradeInventorySyncRequest.h diff --git a/Code/client/Services/Generic/TradeService.cpp b/Code/client/Services/Generic/TradeService.cpp index f71172fea..ce4774b92 100644 --- a/Code/client/Services/Generic/TradeService.cpp +++ b/Code/client/Services/Generic/TradeService.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -233,12 +234,8 @@ void TradeService::SetReady(bool aReady) noexcept request.Ready = aReady; if (aReady) { - auto* player = PlayerCharacter::Get(); - if (!player || !ValidateTradeOffer(player->GetInventory(), m_session.SelfItems)) - { - CancelTrade(); - return; - } + // The server validates the offer against this snapshot; an outdated offer unreadies instead of cancelling. + SyncInventory(); // Accept only partner items this game can rebuild; the rest stay with the partner. // Keep prepared potion forms alive through cancellation races until the session ends. auto& modSystem = m_world.GetModSystem(); @@ -265,6 +262,16 @@ void TradeService::SetReady(bool aReady) noexcept m_transport.Send(request); } +void TradeService::SyncInventory() const noexcept +{ + auto* player = PlayerCharacter::Get(); + if (!player) + return; + TradeInventorySyncRequest request{}; + request.Entries = player->GetInventory().Entries; + m_transport.Send(request); +} + void TradeService::UpdateOffer(const TiltedPhoques::Vector& aSelections) noexcept { ApplyOfferSelection(aSelections); @@ -371,6 +378,9 @@ void TradeService::OnTradeStarted(const NotifyTradeStarted& acMessage) noexcept m_pendingInvites.erase(acMessage.PartnerPlayerId); EmitInviteUpdate(acMessage.PartnerPlayerId, false); + // Offer from the live inventory: enchanted, renamed and brewed items may be missing from the server copy. + SyncInventory(); + EmitStateToUI(); } diff --git a/Code/client/Services/TradeService.h b/Code/client/Services/TradeService.h index 913c7bdd9..74dd9b9fd 100644 --- a/Code/client/Services/TradeService.h +++ b/Code/client/Services/TradeService.h @@ -50,6 +50,7 @@ struct TradeService void ClearSession() noexcept; void ReleasePreparedPotions() noexcept; + void SyncInventory() const noexcept; void EmitStateToUI() const noexcept; void EmitInviteUpdate(uint32_t aInviterId, bool aAdded, uint64_t aExpiryTick = 0) const noexcept; void EmitCancellation(uint32_t aPartnerId, TradeCancelReason aReason, bool aWasInitiator) const noexcept; diff --git a/Code/encoding/Messages/ClientMessageFactory.h b/Code/encoding/Messages/ClientMessageFactory.h index 751e63c32..f3df10076 100644 --- a/Code/encoding/Messages/ClientMessageFactory.h +++ b/Code/encoding/Messages/ClientMessageFactory.h @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -71,7 +72,7 @@ struct ClientMessageFactory { auto s_visitor = CreateMessageVisitor< AuthenticationRequest, AssignCharacterRequest, ClientReferencesMoveRequest, EnterInteriorCellRequest, RequestInventoryChanges, RequestFactionsChanges, RequestQuestUpdate, PartyInviteRequest, PartyAcceptInviteRequest, PartyLeaveRequest, PartyCreateRequest, - PartyChangeLeaderRequest, PartyKickRequest, TradeInviteRequest, TradeInviteResponseRequest, TradeOfferUpdateRequest, TradeSetReadyRequest, TradeCancelRequest, RequestActorValueChanges, RequestActorMaxValueChanges, EnterExteriorCellRequest, RequestHealthChangeBroadcast, ActivateRequest, LockChangeRequest, AssignObjectsRequest, RequestDeathStateChange, ShiftGridCellRequest, + PartyChangeLeaderRequest, PartyKickRequest, TradeInviteRequest, TradeInviteResponseRequest, TradeOfferUpdateRequest, TradeSetReadyRequest, TradeCancelRequest, TradeInventorySyncRequest, RequestActorValueChanges, RequestActorMaxValueChanges, EnterExteriorCellRequest, RequestHealthChangeBroadcast, ActivateRequest, LockChangeRequest, AssignObjectsRequest, RequestDeathStateChange, ShiftGridCellRequest, RequestOwnershipTransfer, RequestOwnershipClaim, RequestObjectInventoryChanges, SpellCastRequest, ProjectileLaunchRequest, InterruptCastRequest, AddTargetRequest, ScriptAnimationRequest, DrawWeaponRequest, MountRequest, NewPackageRequest, RequestRespawn, SyncExperienceRequest, RequestEquipmentChanges, SendChatMessageRequest, TeleportCommandRequest, PlayerRespawnRequest, DialogueRequest, SubtitleRequest, PlayerDialogueRequest, PlayerLevelRequest, TeleportRequest, RequestPlayerHealthUpdate, RequestWeatherChange, RequestCurrentWeather, RequestSetWaypoint, RequestRemoveWaypoint, RemoveSpellRequest, SetTimeCommandRequest>; diff --git a/Code/encoding/Messages/TradeInventorySyncRequest.cpp b/Code/encoding/Messages/TradeInventorySyncRequest.cpp new file mode 100644 index 000000000..6b22461be --- /dev/null +++ b/Code/encoding/Messages/TradeInventorySyncRequest.cpp @@ -0,0 +1,22 @@ +#include + +void TradeInventorySyncRequest::SerializeRaw(TiltedPhoques::Buffer::Writer& aWriter) const noexcept +{ + Serialization::WriteVarInt(aWriter, Entries.size()); + for (const auto& entry : Entries) + entry.Serialize(aWriter); +} + +void TradeInventorySyncRequest::DeserializeRaw(TiltedPhoques::Buffer::Reader& aReader) noexcept +{ + ClientMessage::DeserializeRaw(aReader); + + Entries.clear(); + const auto count = Serialization::ReadVarInt(aReader); + for (uint64_t i = 0; i < count; ++i) + { + Inventory::Entry entry; + entry.Deserialize(aReader); + Entries.push_back(std::move(entry)); + } +} diff --git a/Code/encoding/Messages/TradeInventorySyncRequest.h b/Code/encoding/Messages/TradeInventorySyncRequest.h new file mode 100644 index 000000000..fe5b2b050 --- /dev/null +++ b/Code/encoding/Messages/TradeInventorySyncRequest.h @@ -0,0 +1,21 @@ +#pragma once + +#include "Message.h" +#include + +// The trading player's live inventory. Incremental inventory events miss changes made in place, +// such as enchanting, renaming or brewing, so trades work from this snapshot. +struct TradeInventorySyncRequest final : ClientMessage +{ + static constexpr ClientOpcode Opcode = kTradeInventorySyncRequest; + + TradeInventorySyncRequest() + : ClientMessage(Opcode) + { + } + + void SerializeRaw(TiltedPhoques::Buffer::Writer& aWriter) const noexcept override; + void DeserializeRaw(TiltedPhoques::Buffer::Reader& aReader) noexcept override; + + Vector Entries; +}; diff --git a/Code/encoding/Opcodes.h b/Code/encoding/Opcodes.h index 8db21d270..02e1376e3 100644 --- a/Code/encoding/Opcodes.h +++ b/Code/encoding/Opcodes.h @@ -58,6 +58,7 @@ enum ClientOpcode : unsigned char kTradeOfferUpdateRequest, kTradeSetReadyRequest, kTradeCancelRequest, + kTradeInventorySyncRequest, kClientOpcodeMax }; diff --git a/Code/encoding/Structs/TradeValidation.h b/Code/encoding/Structs/TradeValidation.h index 54b0afe2d..a63902f1f 100644 --- a/Code/encoding/Structs/TradeValidation.h +++ b/Code/encoding/Structs/TradeValidation.h @@ -86,6 +86,37 @@ inline bool ValidateTradeOffer(const Inventory& acInventory, const Vector KeepCoveredTradeOffer(const Inventory& acInventory, const Vector& acItems) noexcept +{ + Vector remaining; + remaining.reserve(acInventory.Entries.size()); + for (const auto& entry : acInventory.Entries) + remaining.push_back(std::max(0, entry.Count)); + + Vector kept; + for (const auto& item : acItems) + { + if (item.Count <= 0 || IsExcludedTradeItem(item) || item.IsWorn()) + continue; + auto reserved = remaining; + int32_t needed = item.Count; + for (size_t i = 0; i < acInventory.Entries.size() && needed > 0; ++i) + { + if (!SameTradeItem(acInventory.Entries[i], item)) + continue; + const int32_t count = std::min(needed, reserved[i]); + reserved[i] -= count; + needed -= count; + } + if (needed != 0) + continue; + remaining = std::move(reserved); + kept.push_back(item); + } + return kept; +} + // Publish neither inventory until both offers and resulting stack counts are valid. inline bool PrepareTradeExchange(const Inventory& acLeft, const Inventory& acRight, const Vector& acLeftOffer, const Vector& acRightOffer, diff --git a/Code/server/Services/TradeService.cpp b/Code/server/Services/TradeService.cpp index af72c9708..f70c78b79 100644 --- a/Code/server/Services/TradeService.cpp +++ b/Code/server/Services/TradeService.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -43,6 +44,7 @@ TradeService::TradeService(World& aWorld, entt::dispatcher& aDispatcher) noexcep m_tradeOfferUpdateConnection = aDispatcher.sink>().connect<&TradeService::OnTradeOfferUpdate>(this); m_tradeSetReadyConnection = aDispatcher.sink>().connect<&TradeService::OnTradeSetReady>(this); m_tradeCancelConnection = aDispatcher.sink>().connect<&TradeService::OnTradeCancel>(this); + m_tradeInventorySyncConnection = aDispatcher.sink>().connect<&TradeService::OnTradeInventorySync>(this); } void TradeService::OnUpdate(const UpdateEvent&) noexcept @@ -302,14 +304,22 @@ void TradeService::OnTradeSetReady(const PacketEvent& acPa if (!ValidateOffer(pPlayer, pSession->Offers[cIndex].Items)) { + // The offer no longer matches the player's inventory; keep the trade open so they can adjust it. spdlog::warn("[TradeService]: Player {} attempted to ready invalid offer", pPlayer->GetId()); - CancelSession(*pSession, TradeCancelReason::FailedValidation, pPlayer); + pSession->Offers[cIndex].Ready = false; + pSession->Offers[cIndex].AcceptedItems.clear(); + ResetCountdown(*pSession); + SendStateUpdate(*pSession); return; } if (!ValidateAcceptedItems(pSession->Offers[1 - cIndex].Items, message.AcceptedItems)) { - CancelSession(*pSession, TradeCancelReason::FailedValidation, pPlayer); + // The partner's offer changed after this player last saw it; they can ready again on the new state. + pSession->Offers[cIndex].Ready = false; + pSession->Offers[cIndex].AcceptedItems.clear(); + ResetCountdown(*pSession); + SendStateUpdate(*pSession); return; } // Keep the server's canonical copies of the accepted partner items, not the client's echo. @@ -346,6 +356,39 @@ void TradeService::OnTradeCancel(const PacketEvent& acPacket RemoveInviteFor(pPlayer, TradeCancelReason::Cancelled, pPlayer); } +void TradeService::OnTradeInventorySync(const PacketEvent& acPacket) noexcept +{ + Player* pPlayer = acPacket.pPlayer; + TradeSession* pSession = GetSession(pPlayer); + if (!pSession) + return; + + const int32_t cIndex = GetSessionIndex(*pSession, pPlayer); + auto* pInventory = GetInventoryFor(pPlayer); + if (cIndex < 0 || !pInventory) + return; + + // Incremental inventory events miss in-place changes such as enchanting, renaming or brewing, + // so the trading player's live inventory replaces the server copy for the trade. + pInventory->Entries = acPacket.Packet.Entries; + + auto& offer = pSession->Offers[cIndex]; + auto covered = KeepCoveredTradeOffer(*pInventory, offer.Items); + if (covered.size() != offer.Items.size()) + { + spdlog::debug("[TradeService]: Dropped {} offered items no longer in player {}'s inventory", offer.Items.size() - covered.size(), pPlayer->GetId()); + offer.Items = std::move(covered); + for (auto& each : pSession->Offers) + { + each.Ready = false; + each.AcceptedItems.clear(); + } + ResetCountdown(*pSession); + } + + SendStateUpdate(*pSession); +} + TradeService::TradeSession* TradeService::GetSession(Player* apPlayer) noexcept { if (!apPlayer) diff --git a/Code/server/Services/TradeService.h b/Code/server/Services/TradeService.h index 35ffd1e8b..ff9a56965 100644 --- a/Code/server/Services/TradeService.h +++ b/Code/server/Services/TradeService.h @@ -13,6 +13,7 @@ struct TradeInviteResponseRequest; struct TradeOfferUpdateRequest; struct TradeSetReadyRequest; struct TradeCancelRequest; +struct TradeInventorySyncRequest; struct Player; @@ -35,6 +36,7 @@ class TradeService void OnTradeOfferUpdate(const PacketEvent& acPacket) noexcept; void OnTradeSetReady(const PacketEvent& acPacket) noexcept; void OnTradeCancel(const PacketEvent& acPacket) noexcept; + void OnTradeInventorySync(const PacketEvent& acPacket) noexcept; private: struct TradeOffer @@ -102,4 +104,5 @@ class TradeService entt::scoped_connection m_tradeOfferUpdateConnection; entt::scoped_connection m_tradeSetReadyConnection; entt::scoped_connection m_tradeCancelConnection; + entt::scoped_connection m_tradeInventorySyncConnection; }; diff --git a/Code/tests/trading.cpp b/Code/tests/trading.cpp index 1cef9b47a..c6d0d2bbf 100644 --- a/Code/tests/trading.cpp +++ b/Code/tests/trading.cpp @@ -10,6 +10,7 @@ #include #include #include +#include namespace { @@ -368,3 +369,40 @@ TEST_CASE("Trade packets round trip through message factories", "[trading][encod REQUIRE(decoded->SelfInventory == state.SelfInventory); } } + +TEST_CASE("Inventory sync keeps only offered items the live inventory still covers", "[trading]") +{ + Inventory::Entry sword; + sword.BaseId.BaseId = 0x12EB7; + sword.Count = 1; + auto enchanted = sword; + enchanted.ExtraEnchantId.ModId = 0xFFFFFFFF; + enchanted.ExtraEnchantId.BaseId = 0x800; + enchanted.EnchantData.IsWeapon = true; + enchanted.EnchantData.Effects.push_back(CraftedPotion().Potion.Effects[0]); + auto potion = CraftedPotion(); + + // The server copy still had the plain sword; enchanting changed it in place. + Inventory live; + live.Entries = {enchanted, potion}; + const auto kept = KeepCoveredTradeOffer(live, {sword, potion, enchanted}); + REQUIRE(kept.size() == 2); + REQUIRE(SameTradeItem(kept[0], potion)); + REQUIRE(SameTradeItem(kept[1], enchanted)); + + auto twoPotions = potion; + twoPotions.Count = potion.Count; + REQUIRE(KeepCoveredTradeOffer(live, {twoPotions, twoPotions}).size() == 1); + + TiltedPhoques::Buffer buffer(4096); + TradeInventorySyncRequest request; + request.Entries = live.Entries; + TiltedPhoques::Buffer::Writer writer(&buffer); + request.Serialize(writer); + TiltedPhoques::Buffer::Reader reader(&buffer); + auto message = ClientMessageFactory{}.Extract(reader); + REQUIRE(message); + REQUIRE(message->GetOpcode() == kTradeInventorySyncRequest); + const auto* decoded = static_cast(message.get()); + REQUIRE(decoded->Entries == request.Entries); +} diff --git a/PLAYER_TRADING_POTIONS.md b/PLAYER_TRADING_POTIONS.md index ef083e9fc..28b368f31 100644 --- a/PLAYER_TRADING_POTIONS.md +++ b/PLAYER_TRADING_POTIONS.md @@ -21,3 +21,9 @@ Each client shows only the partner items it can rebuild: every plugin form the i 9. A offers a crafted potion with an effect from a plugin B lacks, plus a crafted poison with vanilla effects. B offers a weapon with a crafted vanilla enchantment, plus armor from a plugin A lacks. Verify A sees only the weapon, B sees only the poison, each sender's offer marks the hidden item after the partner readies, and after the countdown only the weapon and poison change hands. 10. Coat a weapon with a crafted vanilla-effect poison, trade it, and verify the recipient's weapon shows the same poison effects and remaining uses, then hits apply the poison. Repeat with a poison using an effect from a plugin the recipient lacks: the weapon must stay hidden and remain with the sender. + +## Live inventory snapshot + +Enchanting, renaming and brewing can change items without the incremental inventory events the server relies on. When a trade starts and each time a player readies, their client sends its live inventory and the server replaces its copy for the trade. Offered items the snapshot no longer covers are dropped from the offer and both players are unreadied. Readying an outdated offer, or one built on an outdated partner offer, unreadies instead of cancelling. + +11. Enchant and rename an item ("AAA ...") and brew a potion while connected, then start a trade without relogging. Both must appear in the picker, the renamed item must be found by searching "AAA", and readying with both offered must not cancel the trade. From f87a288d6ea5f7d3af61ec214186d3c551eb7dcd Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 08:02:46 +0000 Subject: [PATCH 2/2] fix: remove the exact traded item instead of a plain copy Inventory removals passed a freshly built extra data list to RemoveItem. It matches none of the inventory's stored lists, so trading away an enchanted, renamed or poisoned item could leave it with the sender and take a plain copy (or nothing) instead. Removals now find the stored extra data list that describes the entry and remove that one. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01SSW5HGXzi9kAVm4zdsSaid --- Code/client/Games/Skyrim/TESObjectREFR.cpp | 45 ++++++++++++++++++++-- Code/client/Games/Skyrim/TESObjectREFR.h | 2 + 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/Code/client/Games/Skyrim/TESObjectREFR.cpp b/Code/client/Games/Skyrim/TESObjectREFR.cpp index 7369586e1..d309c4306 100644 --- a/Code/client/Games/Skyrim/TESObjectREFR.cpp +++ b/Code/client/Games/Skyrim/TESObjectREFR.cpp @@ -901,10 +901,9 @@ void TESObjectREFR::AddOrRemoveItem(const Inventory::Entry& arEntry, bool aIsSet return; } - ExtraDataList* pExtraDataList = GetExtraDataFromItem(arEntry); - if (arEntry.Count > 0) { + ExtraDataList* pExtraDataList = GetExtraDataFromItem(arEntry); bool isWorn = false; bool isWornLeft = false; if (pExtraDataList) @@ -925,8 +924,18 @@ void TESObjectREFR::AddOrRemoveItem(const Inventory::Entry& arEntry, bool aIsSet else if (arEntry.Count < 0) { spdlog::debug("Removing item {:X}, count {}", pObject->formID, -arEntry.Count); + // Removal must name the inventory's own extra data list. A freshly built list matches none of the + // stored ones, so an enchanted, renamed or poisoned item would stay while a plain copy was taken. + auto removalList = [this, &arEntry](TESBoundObject* apItem) -> ExtraDataList* { + if (!arEntry.ContainsExtraData()) + return nullptr; + if (ExtraDataList* pExisting = FindExtraDataForItem(apItem, arEntry)) + return pExisting; + spdlog::warn("{}: No stored extra data matches item {:X}; removing with rebuilt data", __FUNCTION__, apItem->formID); + return GetExtraDataFromItem(arEntry); + }; if (arEntry.Potion.Effects.empty()) - RemoveItem(pObject, -arEntry.Count, ITEM_REMOVE_REASON::kRemove, pExtraDataList, nullptr); + RemoveItem(pObject, -arEntry.Count, ITEM_REMOVE_REASON::kRemove, removalList(pObject), nullptr); else { // Equivalent recipes can have different local IDs. Remove across their actual stacks. @@ -940,7 +949,7 @@ void TESObjectREFR::AddOrRemoveItem(const Inventory::Entry& arEntry, bool aIsSet const auto count = static_cast(std::min(remaining, before)); if (count <= 0) break; - RemoveItem(potion, count, ITEM_REMOVE_REASON::kRemove, pExtraDataList, nullptr); + RemoveItem(potion, count, ITEM_REMOVE_REASON::kRemove, removalList(potion), nullptr); const auto removed = before - GetItemCountInInventory(potion); if (removed <= 0) break; @@ -969,6 +978,34 @@ void TESObjectREFR::AddOrRemoveItem(const Inventory::Entry& arEntry, bool aIsSet UpdateItemList(nullptr); } +ExtraDataList* TESObjectREFR::FindExtraDataForItem(TESBoundObject* apObject, const Inventory::Entry& arEntry) const noexcept +{ + auto* pChanges = GetContainerChanges(); + if (!apObject || !pChanges || !pChanges->entries) + return nullptr; + + for (auto* pEntry : *pChanges->entries) + { + if (!pEntry || pEntry->form != apObject || !pEntry->dataList) + continue; + for (ExtraDataList* pExtraDataList : *pEntry->dataList) + { + if (!pExtraDataList) + continue; + Inventory::Entry local{}; + local.BaseId = arEntry.BaseId; + local.Potion = arEntry.Potion; + { + ScopedExtraDataOverride _; + GetItemFromExtraData(local, pExtraDataList); + } + if (local.CanBeMerged(arEntry)) + return pExtraDataList; + } + } + return nullptr; +} + void TESObjectREFR::UpdateItemList(TESForm* pUnkForm) noexcept { TP_THIS_FUNCTION(TUpdateItemList, void, TESObjectREFR, TESForm*); diff --git a/Code/client/Games/Skyrim/TESObjectREFR.h b/Code/client/Games/Skyrim/TESObjectREFR.h index 2ebd9b219..16cd5baea 100644 --- a/Code/client/Games/Skyrim/TESObjectREFR.h +++ b/Code/client/Games/Skyrim/TESObjectREFR.h @@ -63,6 +63,8 @@ struct TESObjectREFR : TESForm static void GetItemFromExtraData(Inventory::Entry& arEntry, ExtraDataList* apExtraDataList) noexcept; static ExtraDataList* GetExtraDataFromItem(const Inventory::Entry& arEntry) noexcept; + // The inventory's own extra data list describing this entry, so removal takes that exact item. + ExtraDataList* FindExtraDataForItem(TESBoundObject* apObject, const Inventory::Entry& arEntry) const noexcept; virtual void sub_3B(); virtual void sub_3C();