Guild bank: tab rename, and item movement with the destination on the right side - #85
Conversation
CMSG_GUILD_BANK_UPDATE_TAB was dormant, so a tab could be bought and then never named. The inherited handler read a raw GUID, then the tab, then two NUL-terminated strings; at 18414 the tab leads as a plain byte, the GUID is packed, both string lengths live in the bit stream, and neither string is terminated. The reader comes from the client's writer sub_68B694, and which of its two strings is the name comes from the Lua binding SetGuildBankTabInfo(tab, name, iconFileName), the writer being unable to distinguish them. The reply is SMSG_GUILD_EVENT_BANK_TAB_MODIFIED (0x0BF1), broadcast to the guild, and no bank list follows it. An earlier version of this change left that opcode dormant and answered the actor with a list, claiming nothing confirmed the value carried the message. That was wrong, and wrong because it never asked the client. The value routes through the guild SMSG dispatcher sub_68EC4C to parser sub_6A224B, whose body is a 9-bit icon length, a 7-bit name length, the name, a uint32 tab id and the icon; sub_96ED66 then stores both strings in the tab cache and raises event 0x1AF, which is GUILDBANK_UPDATE_TABS. Its Lua handler repaints the frame on every path, so the list the old code sent was redundant as well as unevidenced. The binding from value to parser is recorded in Opcodes.h, because it is the step a re-derivation cannot guess: sub_6A224B holds no opcode literal, and the only push 0xbf1 in the image is an FMOD line number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Registers CMSG_GUILD_BANK_SWAP_ITEMS (0x136A), previously defined but never registered, and replaces its pre-MoP reader. Four player actions build this one opcode and at 18414 they are four different bodies; BankToBank and AutoStore are bits in the mask rather than plain bytes, so the shape now comes out of the reader. bankTab/bankSlot is the destination of a bank-to-bank move, not its source, which is the inverse of the inherited field names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8febf7b8d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (!GetPlayer()->GetGameObjectIfCanInteractWith(goGuid, GAMEOBJECT_TYPE_GUILD_BANK)) | ||
| { | ||
| pGuild->SwapItems(_player, req.srcTab, req.srcSlot, req.bankTab, req.bankSlot, req.splitAmount); |
There was a problem hiding this comment.
When a bank-to-bank request has a nonzero split amount, this newly reachable call enters Guild::SwapItems, whose split branch invokes pl->ItemRemovedQuestCheck(...) at GuildBank.cpp:1396 even though the item never leaves the guild bank or the player's inventory. If the item matches an active delivery objective, merely splitting or moving the bank stack can reduce the player's quest counter and even mark a completed quest incomplete; remove that inventory-removal notification from the bank-to-bank path.
Useful? React with 👍 / 👎.
| /// the database actually holds. Losing unsaved progress is the cheaper error. | ||
| bool Guild::CommitBankMutation(Player* pl, char const* context) | ||
| { | ||
| if (CharacterDatabase.CommitTransactionDirect()) |
There was a problem hiding this comment.
When an older inventory save is still on the async queue, this direct commit can overtake it on the shared database connection. For example, trade completion queues SaveInventoryAndGoldToDB() through CommitTransaction() at TradeHandler.cpp:561-564; if the recipient immediately deposits part of that stack, the direct bank transaction persists the reduced character stack and the bank clone first, after which the older queued item update can restore the original character stack count, duplicating the deposited amount durably. The bank mutation needs an ordering barrier or must remain ordered behind previously queued character/item writes rather than executing directly past them.
Useful? React with 👍 / 👎.
| if (req.toChar) // Bank -> Char cases | ||
| { | ||
| pGuild->MoveFromBankToChar(_player, BankTab, BankTabSlot, PlayerBag, PlayerSlot, SplitedAmount); | ||
| pGuild->MoveFromBankToChar(_player, req.bankTab, req.bankSlot, playerBag, playerSlot, req.splitAmount); |
There was a problem hiding this comment.
Before dispatching this move, compare the current bank-slot contents with req.entryAtBankSlot (and req.srcEntry for bank-to-bank moves). These parsed fields describe the items the client acted on, but the handler ignores them, so if another member changes either shared slot after this client builds its packet but before the server processes it, MoveFromBankToChar or MoveFromCharToBank operates on the replacement item instead; a stale deposit can consequently swap an unintended item out of the guild bank and into the depositor's inventory.
Useful? React with 👍 / 👎.
GuildRank.cpp reaches for sObjectMgr but includes only Guild.h, so it has been relying on a transitive include that MSVC provides and gcc and clang do not. The Linux build has been failing on master since at least 19 August because of it; this is unrelated to the guild bank work in this branch and can be split out if preferred. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t progress Addresses review feedback on the item movement path. The client stamps each move with the entry it believes is in the bank slot it is acting on. A guild bank is shared, so that can be stale by the time the server reads it -- the move was applied to whatever occupied the slot instead, which let a stale deposit pull a stranger's item out of the bank. Guild gains BankSlotHoldsEntry and the handler checks both sides before dispatching. Guild::SwapItems also called ItemRemovedQuestCheck when splitting a stack from one bank slot to another. Nothing leaves the player's inventory there, but the call decrements every matching DELIVER objective, so splitting a bank stack of a quest item consumed the player's own progress. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a695f60ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (req.toChar) // Bank -> Char cases | ||
| { | ||
| pGuild->MoveFromBankToChar(_player, BankTab, BankTabSlot, PlayerBag, PlayerSlot, SplitedAmount); | ||
| pGuild->MoveFromBankToChar(_player, req.bankTab, req.bankSlot, playerBag, playerSlot, req.splitAmount); |
There was a problem hiding this comment.
When an auto-store request is delayed while another member changes the source stack's count, req.autoStoreCount still records the full stack size the client acted on, but this dispatch ignores it and passes splitAmount (zero for the captured auto-store shape). MoveFromBankToChar interprets zero as the entire current stack, so merging more of the same entry into that slot before processing makes the stale request withdraw those additional items despite the entry check succeeding. Validate the current count against autoStoreCount, or use that count as the requested amount, before moving the item.
Useful? React with 👍 / 👎.
Auto-store asks for the whole stack by sending splitAmount 0, which MoveFromBankToChar reads as "however many are there now". Checking the item entry is therefore not enough on that path: another member merging more of the same item into the slot leaves the entry matching while the stack grows, and the stale request walks off with the larger stack. The client stamps the size it saw. That field is genuinely a stack count and not an inferred label -- the auto-store binding sub_96F53D stores its cached bank record's [3] there, and GetGuildBankItemInfo returns that same [3] as its itemCount -- so a mismatch either way means the client acted on a stack that no longer exists, and the request is refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 159ebc5ccc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
|
|
||
| pGuild->SwapItems(_player, req.srcTab, req.srcSlot, req.bankTab, req.bankSlot, req.splitAmount); |
There was a problem hiding this comment.
When this newly reachable bank-to-bank path moves a stack onto a partial stack, Guild::CanStoreItem can add other partial stacks of the same entry to dest (GuildBank.cpp:1262-1274) before placing any remainder back in the source slot. SwapItems persists all those changes but then refreshes only the source and the explicitly requested destination at GuildBank.cpp:1533-1537, so the extra affected slots remain stale on every viewer and the items can appear missing until a full bank refresh. Broadcast the complete dest/gDest set, as the character-to-bank paths already do.
Useful? React with 👍 / 👎.
Guild::CanStoreItem hands _CanStoreItem_InTab the whole tab when the moved stack can merge, so it tops up every partial stack of that entry rather than only the slot the client named. SwapItems then broadcast just the source and the named destination, leaving those other slots stale on every viewer -- items sitting in the bank but drawn as missing until a full refresh. The branches now record what they actually wrote and the refresh broadcasts that set, as the character-to-bank paths already did. Source and destination still travel together when they share a tab, so an ordinary same-tab move is still one packet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Two guild bank opcodes for MoP 5.4.8 (client build 18414), each derived from the client rather than from a reference fork.
CMSG_GUILD_BANK_UPDATE_TAB(0x07C2) — naming a tabRequest layout derived from the client's writer
sub_68B694; the reply,SMSG_GUILD_EVENT_BANK_TAB_MODIFIED(0x0BF1), from its inbound parsersub_6A224B, there being no capture of either at 18414. The opcode value is bound to that parser through the guild SMSG dispatchersub_68EC4Cand its two tables — the step a re-derivation cannot guess, since SMSG values carry no literal in the image. This is the first SMSG on this campaign promoted off the reference tier.Reply field order differs from the request: the name comes first and the tab id sits between the two strings. Name and icon limits are 64 and 256 bytes, the client's own copy limits, now covered by a fixture at the boundary.
CMSG_GUILD_BANK_SWAP_ITEMS(0x136A) — moving itemsPreviously defined but never registered, so this makes the guild bank's item movement reachable for the first time. Four player actions build this one opcode and at 18414 they are four different bodies (20, 21, 23, 25 bytes);
BankToBankandAutoStoreare bits in the mask rather than plain bytes, so the shape comes out of the reader. The fixture decodes real retail bodies of all four shapes.bankTab/bankSlotis the destination of a bank-to-bank move, not its source — the inverse of the inherited field names. Settled from the wire: twelve captured bodies name an empty bank slot there whilesrcTab/srcSlotholds a real item, and an empty slot cannot be a source.Defects closed on the newly-reachable path
Registering the opcode makes
Guild::SwapItems,MoveFromBankToCharandMoveFromCharToBanklive —HandleGuildBankSwapItemsis their only caller — so three pre-existing defects are fixed here:if (BankTab != BankTabDst), and same-tab is the ordinary case, so a forged client could rearrange any purchased tab including one its rank cannot view. Destination deposit rights are now required on every move; the source needs the withdrawal allowance across tabs, or view rights within one.Guild::CommitBankMutation, which usesCommitTransactionDirect— the plainCommitTransactiononly queues once the world has loaded and returns true before MySQL sees anything. On failure it marks the bank untrusted, suppresses the character save and disconnects the session, and every caller abandons the operation without broadcasting.Verification
Full Release build clean;
ctest -C Release115/115 from PowerShell. Reviewed across four adversarial rounds, which found and settled the source/destination inversion, the auto-store slot translation, and all three defects above.🤖 Generated with Claude Code