Guild bank: tab rename, and item movement with the destination on the right side - #85
Guild bank: tab rename, and item movement with the destination on the right side#85MadMaxMangos wants to merge 2 commits into
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 👍 / 👎.
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