improve: money handling and delivery feedback#3692
Conversation
Refactors money addition and removal logic to return the actual amount delivered and a status code, adds handling for partial delivery (e.g., due to capacity limits), and provides player feedback and logging in such cases. Updates related interfaces and Lua bindings to reflect these changes, and introduces a new flag to allow dropping money on the map if inventory is full.
dudantas
changed the title
Eliminated the unnecessary inclusion of <utility> from game.hpp to clean up unused dependencies.
|
src/creatures/npcs/npc.cpp
Outdated
| player->sendTextMessage(MESSAGE_EVENT_ADVANCE, msg); | ||
| } else { | ||
| g_game().addMoney(player, totalCost); | ||
| uint32_t flags = FLAG_DROPONMAP; |
There was a problem hiding this comment.
Can this situation, where the gold drops directly when the player has no backpack or no empty slots, be avoided?
There was a problem hiding this comment.
Can this situation, where the gold drops directly when the player has no backpack or no empty slots, be avoided?
Yes, it needs to have the drop on map option disabled (I haven't tested this part; if it's bugged, let me know)
|
This PR is stale because it has been open 45 days with no activity. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
📝 WalkthroughWalkthroughGame::addMoney now returns (addedAmount, ReturnValue) and accepts flags; NPC selling was refactored into a removal/delivery/refund workflow for gold and custom currencies; Bank::withdraw handles partial/over-delivery and refunds; drop-on-map flag added; integration tests for money/change and runtime test config added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Player as "Player"
participant Scheduler as "Scheduler"
participant NPC as "NPC"
participant Game as "Game"
participant Bank as "Bank"
participant Inventory as "Inventory"
participant Inbox as "Inbox"
Player->>Scheduler: request sell (sellItem / sellAllLoot)
Scheduler->>NPC: schedule sell handler (context)
NPC->>Game: removeSellItems(context) -> SellRemovalResult(totalRemoved, refundItems)
Game->>Game: totalCost = sellPrice * totalRemoved
alt currency == gold
Game->>Game: addMoney(player, totalCost, flags) -> (delivered, ReturnValue)
alt delivered to bank
Game->>Bank: deposit(delivered)
Bank-->>Player: notify deposit
else partial or wallet delivery
NPC->>Inventory: try deliver coins to inventory
alt inventory full / partial
NPC->>Inbox: send refundItems
end
NPC-->>Player: notify partial delivery/refund
end
else custom currency
Game->>Game: deliverCustomCurrency(player, totalCost)
alt partial/fail
NPC->>Inbox: refundSoldItems(refundItems)
NPC-->>Player: notify refund outcome
else success
NPC-->>Player: deliver currency items
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/game/game.cpp (2)
2703-2707: Bug crítico:playerpode sernullptre é dereferenciado emremoveMoney.Hoje
playersó é definido quandouseBalance == true, masplayer->removeItem(...)/player->updateState()são chamados independentemente disso. Isso estoura fácil (ex.: qualquer chamadaremoveMoney(player, X)comuseBalancedefaultfalse).Proposta de correção (mínima e segura)
- const auto &player = useBalance ? std::dynamic_pointer_cast<Player>(cylinder) : nullptr; + const auto player = std::dynamic_pointer_cast<Player>(cylinder); uint64_t balance = 0; if (useBalance && player) { balance = player->getBankBalance(); } + // Se a função assumiu Player (por usar player->removeItem/updateState), falhe cedo. + // Alternativa: reverter remoções para internalRemoveItem(...) quando !player. + if (!player) { + g_logger().error("[{}] removeMoney called with non-player cylinder", __FUNCTION__); + return false; + }Also applies to: 2713-2719, 2747-2754
2769-2817: Overflow/truncamento emaddMoney: contagens emuint16_t/uint32_tpodem corromper valores grandes.
moneyéuint64_t, mascrystalCoinséuint32_t,platinumCoinséuint16_t, eaddCoinsrecebeuint32_t count. Para valores altos, isso trunca e pode adicionar bem menos do que o solicitado.Proposta de correção (tipos em uint64 e casts seguros)
- auto addCoins = [&](uint16_t itemId, uint32_t count, uint64_t unitValue) { + auto addCoins = [&](uint16_t itemId, uint64_t count, uint64_t unitValue) { while (count > 0) { - const uint16_t createCount = std::min<uint32_t>(100, count); + const uint32_t createCount = static_cast<uint32_t>(std::min<uint64_t>(100, count)); const auto &remaindItem = Item::CreateItem(itemId, createCount); ReturnValue ret = internalAddItem(cylinder, remaindItem, INDEX_WHEREEVER, flags); if (ret != RETURNVALUE_NOERROR) { if ((flags & FLAG_DROPONMAP) != 0 && cylinder->getTile()) { ret = internalAddItem(cylinder->getTile(), remaindItem, INDEX_WHEREEVER, FLAG_NOLIMIT); } } if (ret == RETURNVALUE_NOERROR) { totalAdded += static_cast<uint64_t>(createCount) * unitValue; } else { returnValue = ret; break; } - count -= createCount; + count -= createCount; } }; - uint32_t crystalCoins = money / 10000; - money -= crystalCoins * 10000; + uint64_t crystalCoins = money / 10000; + money -= crystalCoins * 10000; addCoins(ITEM_CRYSTAL_COIN, crystalCoins, 10000); - uint16_t platinumCoins = money / 100; - money -= platinumCoins * 100; + uint64_t platinumCoins = money / 100; + money -= platinumCoins * 100; addCoins(ITEM_PLATINUM_COIN, platinumCoins, 100); if (money > 0) { addCoins(ITEM_GOLD_COIN, money, 1); }src/game/bank/bank.cpp (1)
142-177: Remova a verificação defensiva paraaddedMoney > amount, pois é matematicamente impossível.A função
Game::addMoney()decompõe o dinheiro em moedas (cristal: 10000, platina: 100, ouro: 1) e apenas incrementatotalAddedquando cada lote é adicionado com sucesso. O valor retornado nunca pode exceder o montante solicitado.Porém, há questões de design legítimas:
Inconsistência com
Game::removeMoney(): Ao entregar troco,removeMoney()tenta comFLAG_DROPONMAPcomo fallback e aborta a transação se a entrega completa falhar.Bank::withdraw()usaflags=0e retornatruemesmo em falha parcial (apenas refunda o saldo ao banco).Falta de
FLAG_DROPONMAP: Considere usarFLAG_DROPONMAPcomo fallback (similar ao padrão emremoveMoney) para evitar entregas parciais quando o inventário está cheio.Semântica de retorno: Decide explicitamente se
withdraw()deve retornarfalseem caso de falha parcial/total, em vez de sempre retornartrue.src/lua/functions/creatures/player/player_functions.cpp (1)
2471-2485: Corrigir inconsistências de tipo numérico e contrato Lua emPlayer:addMoney.A função retorna 2 valores (quantidade adicionada e código de retorno), mas há três problemas:
Tipo numérico inconsistente:
addedMoneyéuint64_tmas está sendo empilhado comlua_pushinteger(inteiro assinado).getMoney()usalua_pushnumberpara o mesmo tipo. Uselua_pushnumberouLua::pushNumberpara consistência.Flags hardcoded: Ao contrário de
removeMoney(), que aceitaflagscomo parâmetro,addMoney()o deixa fixo em 0. Considere adicionar como parâmetro Lua opcional.Contrato Lua muda de 1 para 2 retornos: Scripts existentes funcionam porque Lua descarta retornos extras, mas é uma mudança de API que deve ser documentada se intencional.
🤖 Fix all issues with AI agents
In @src/creatures/npcs/npc.cpp:
- Around line 559-575: The sell flow removes items then may partially fail to
deliver gold via g_game().addMoney (addedMoney < totalCost) but currently only
logs/messages — fix by compensating the shortfall: when addedMoney < totalCost
compute refund = totalCost - addedMoney and automatically credit the player's
bank balance (player->setBankBalance(player->getBankBalance() + refund)) and
send a clear message to the player explaining delivered vs refunded amounts;
ensure g_metrics() counters reflect actual delivered amount and/or refund as
separate metrics. Also fix the AUTOBANK branch message in Npc::onPlayerSellItem
to use totalCost (not totalPrice), correct the typo "transfered" → "transferred"
and adjust punctuation for clarity.
In @src/game/game.cpp:
- Around line 2755-2764: The bank debit path in removeMoney (when useBalance &&
player && player->getBankBalance() >= money) updates the balance via
player->setBankBalance(oldBalance - money) but does not notify the client; after
calling player->setBankBalance(...) add a call to
sendResourceBalance(RESOURCE_BANK, player->getBankBalance()) so the client
receives the new bank balance (place it before logging or immediately after
setBankBalance, similar to other code paths that update bank balance); ensure
you reference the existing player, getBankBalance, setBankBalance, and
RESOURCE_BANK symbols when adding the call.
🧹 Nitpick comments (3)
src/items/items_definitions.hpp (1)
471-481: Preferir literal unsigned no bitshift do novo flag (evita surprises de tipo).
Hoje1 << 8éint; como isso é usado como bitmask (uint32_t flags), eu usaria1u << 8por consistência com máscaras/flags.Proposta de ajuste
- FLAG_DROPONMAP = 1 << 8, // Drop item on map + FLAG_DROPONMAP = 1u << 8, // Drop item on mapsrc/creatures/npcs/npc.cpp (2)
492-500: Contexto noscheduleEvent: ok, mas padronizar o identificador (evitar dados “variáveis demais” no nome).
Boa para rastreabilidade, só avalie se incluirplayer->getName()no “context” é desejado (ruído/alta cardinalidade em logs/telemetria).
577-584: Custom currency: boa checagem deReturnValue, mas ajustar log/mensagem (código vs texto).
LGTM a validação do retorno deinternalPlayerAddItem. Eu só ajustaria o log para imprimir o código numérico ou a mensagem (hoje o placeholder diz “error code”, mas passagetReturnMessage(...)).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/creatures/npcs/npc.cppsrc/game/bank/bank.cppsrc/game/game.cppsrc/game/game.hppsrc/items/items_definitions.hppsrc/lua/functions/creatures/player/player_functions.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
src/game/bank/bank.cpp (1)
src/game/bank/bank.hpp (6)
player(46-46)amount(17-17)amount(40-40)amount(41-41)amount(42-42)amount(44-44)
src/game/game.cpp (1)
src/game/game.hpp (3)
cylinder(218-218)cylinder(222-222)cylinder(224-224)
src/creatures/npcs/npc.cpp (3)
src/utils/tools.cpp (2)
getReturnMessage(1285-1554)getReturnMessage(1285-1285)src/utils/tools.hpp (1)
getReturnMessage(154-154)src/items/item.cpp (4)
CreateItem(42-113)CreateItem(42-42)CreateItem(278-313)CreateItem(278-278)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: windows-2022-windows-release
- GitHub Check: triage
🔇 Additional comments (2)
src/game/game.cpp (1)
2769-2769: Assinatura deaddMoneyfoi corretamente propagada para header e call-sites.A mudança de retorno
std::pair<uint64_t, ReturnValue>foi implementada completamente: header (game.hpp:224), definição (game.cpp:2769) e todos os call-sites utilizam structured bindings (auto [addedMoney, returnValue]) para desempacotar o par corretamente.src/game/game.hpp (1)
222-225: Mudança de API pública foi implementada completamente. Todos os callsites foram atualizados para o novo tipo de retornostd::pair<uint64_t, ReturnValue>:
src/creatures/npcs/npc.cpp:565— structured bindingsrc/game/bank/bank.cpp:150— structured bindingsrc/game/game.cpp:2726, 2729— structured binding estd::tiesrc/lua/functions/creatures/player/player_functions.cpp:2477— binding Lua com retorno corretoNenhuma chamada deixada esperando
void. Boa execução da mudança de contrato.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the money handling system to support partial delivery tracking and improved error handling. The addMoney function now returns a pair containing the actual amount delivered and a status code, enabling callers to detect and handle scenarios where money cannot be fully delivered (e.g., due to capacity limits).
Changes:
- Modified
addMoneyto returnstd::pair<uint64_t, ReturnValue>instead ofvoid - Added
FLAG_DROPONMAPflag to allow dropping money on the map when inventory is full - Updated all callers (Lua bindings, bank withdrawals, NPC sales) to handle partial delivery with player notifications and logging
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/game/game.hpp | Changed addMoney return type to return amount delivered and status |
| src/game/game.cpp | Implemented partial delivery tracking, refactored change calculation in removeMoney, added logging |
| src/items/items_definitions.hpp | Added FLAG_DROPONMAP flag for dropping items on map |
| src/lua/functions/creatures/player/player_functions.cpp | Updated Lua binding to return two values (amount and status) |
| src/game/bank/bank.cpp | Added partial delivery handling with refunds and player notifications |
| src/creatures/npcs/npc.cpp | Added partial delivery handling for NPC sales with warnings and notifications |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/game/game.cpp (1)
2700-2751:⚠️ Potential issue | 🔴 CriticalFix critical null pointer dereference in
removeMoneywhenuseBalance=false.When
removeMoneyis called withuseBalance=false(the default), theplayerpointer is set tonullptr(line 2700). However, if the cylinder contains money items, the function iterates throughmoneyMapand unconditionally dereferencesplayerviaplayer->removeItem()andplayer->updateState()(lines 2714, 2738–2739, 2742–2743, 2747).This causes a crash whenever
removeMoneyis called with a Player cylinder and the defaultuseBalance=false, even though many call sites do exactly this (e.g.,bank.cpp:205,bank_functions.cpp:157).Either enforce Player-only usage or safely handle non-Player cylinders by conditionally calling player-specific methods or providing fallback mechanisms.
src/lua/functions/creatures/player/player_functions.cpp (1)
2473-2487:⚠️ Potential issue | 🟠 MajorUpdate documentation and consider exposing the
flagsparameter to align with the new API signature.The function now returns two integers
(addedMoney, returnValue)instead of a boolean, which is a breaking Lua API change. While no existing scripts in the repo check this return value (all calls are standalone), the comment on line 2474 still claims// player:addMoney(money)and doesn't document the new return signature—this should be updated.Additionally, the
flagsparameter is hardcoded to0despite the PR introducingFLAG_DROPONMAP. Consider exposing this as an optional Lua parameter (player:addMoney(money[, flags = 0])) to allow scripts to use the new feature, or document why it's intentionally internal-only.Suggested improvements
int PlayerFunctions::luaPlayerAddMoney(lua_State* L) { - // player:addMoney(money) + // player:addMoney(money[, flags = 0]) + // Returns: addedMoney (uint64), returnValue (ReturnValue as int); or (nil, nil) on error const uint64_t money = Lua::getNumber<uint64_t>(L, 2); const auto &player = Lua::getUserdataShared<Player>(L, 1, "Player"); if (player) { - uint32_t flags = 0; + uint32_t flags = Lua::getNumber<uint32_t>(L, 3, 0); auto [addedMoney, returnValue] = g_game().addMoney(player, money, flags);
🧹 Nitpick comments (1)
src/game/game.hpp (1)
224-224: Signature change is well-implemented and properly integrated throughout the codebase.The move from
voidtostd::pair<uint64_t, ReturnValue>enables proper partial-delivery tracking. All five call sites have been correctly updated to handle the new return type using structured bindings (auto [addedMoney, returnValue] = ...) orstd::tie().The transitive
<utility>dependency is not a practical concern:std::pairis already part of the public API surface elsewhere in this header (e.g.,playerSetMonsterPodium), and standard headers like<map>and<vector>already in use transitively include<utility>.Optional refinement: Consider replacing
std::pair<uint64_t, ReturnValue>with a named struct likestruct AddMoneyResult { uint64_t addedAmount; ReturnValue status; }for improved readability at call sites (result.addedAmountvsresult.first) and self-documenting code. This is purely optional—the current implementation is correct and idiomatic C++17.
Enhance money transfer and removal logic, tighten logging, update Lua bindings, and add integration tests. - src/creatures/npcs/npc.cpp: Refactor sell flow to use player ID in scheduled context, track actual delivered amount when paying players, deposit refunds into bank if inventory delivery fails, send updated bank balance to player, improve log messages, and report delivered amount to metrics. - src/game/game.cpp: Make removeMoney resilient when operating on non-player cylinders by using internalRemoveItem for null player cases; only call updateState/sendResourceBalance when a player exists; send bank balance update after debiting bank. - src/items/items_definitions.hpp: Make FLAG_DROPONMAP an unsigned shift (1u << 8) to avoid signed-shift issues. - src/lua/functions/creatures/player/player_functions.cpp: Extend player:addMoney Lua binding to accept optional flags and return three values (success, addedMoney, returnValue). - tests/integration/game: Add money integration tests (money_it.cpp) covering change-making, failure when change cannot be delivered, and bank debits; update CMakeLists to include the new test. These changes fix corner cases when delivering currency to players or containers, ensure correct bookkeeping of refunds and metrics, and add tests to prevent regressions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/game/game.cpp (1)
2794-2826:⚠️ Potential issue | 🟠 MajorFix integer truncation when converting coin counts from
uint64_tmoney amount.The
addMoneyfunction acceptsuint64_t money, but at lines 2816–2822 it converts to narrower types (uint32_t crystalCoins,uint16_t platinumCoins). For balances exceeding 4,294,967,295, the division results truncate, delivering fewer coins than intended. Useuint64_tfor coin counts and only downcast the per-stack amount touint16_twithin the loop.Suggested fix
- auto addCoins = [&](uint16_t itemId, uint32_t count, uint64_t unitValue) { + auto addCoins = [&](uint16_t itemId, uint64_t count, uint64_t unitValue) { while (count > 0) { - const uint16_t createCount = std::min<uint32_t>(100, count); + const uint16_t createCount = static_cast<uint16_t>(std::min<uint64_t>(100, count)); const auto &remaindItem = Item::CreateItem(itemId, createCount); ReturnValue ret = internalAddItem(cylinder, remaindItem, INDEX_WHEREEVER, flags); if (ret != RETURNVALUE_NOERROR) { if ((flags & FLAG_DROPONMAP) != 0 && cylinder->getTile()) { ret = internalAddItem(cylinder->getTile(), remaindItem, INDEX_WHEREEVER, FLAG_NOLIMIT); } } if (ret == RETURNVALUE_NOERROR) { totalAdded += static_cast<uint64_t>(createCount) * unitValue; } else { returnValue = ret; break; } count -= createCount; } }; - uint32_t crystalCoins = money / 10000; + uint64_t crystalCoins = money / 10000; money -= crystalCoins * 10000; addCoins(ITEM_CRYSTAL_COIN, crystalCoins, 10000); - uint16_t platinumCoins = money / 100; + uint64_t platinumCoins = money / 100; money -= platinumCoins * 100; addCoins(ITEM_PLATINUM_COIN, platinumCoins, 100); if (money > 0) { addCoins(ITEM_GOLD_COIN, money, 1); }
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Around line 586-592: In Npc::onPlayerSellItem the code creates a custom
currency Item and calls g_game().internalPlayerAddItem(...) then returns on
failure, which silently loses the items already removed from the player; change
the failure branch to attempt an immediate refund instead of returning: use the
same removed item objects/collection that were taken earlier (e.g., the
soldItems/itemsToRemove variables used around lines 538-541) and try to restore
them to the player (via the existing player-add APIs or
g_game().internalPlayerAddItem), if that fails fall back to securely dropping
them on the map at the player’s position or crediting the player’s bank, log the
full error via g_logger().error with getReturnMessage(returnValue) and notify
the player with a clear message; ensure no silent early return remains so all
error paths either restore items or record a safe fallback.
- Around line 559-584: The AUTOBANK branch sets the player's bank balance via
player->setBankBalance(...) but does not call
player->sendResourceBalance(RESOURCE_BANK, ...) to update the client UI; add a
call to player->sendResourceBalance(RESOURCE_BANK, player->getBankBalance())
immediately after player->setBankBalance(...) inside the AUTOBANK block (near
the code using g_configManager().getBoolean(AUTOBANK), player->setBankBalance,
and deliveredAmount) so the client sees the new bank balance just like the
refund path does.
🧹 Nitpick comments (4)
src/items/items_definitions.hpp (1)
480-480: Minor: inconsistent literal suffix1uvs1in the rest of the enum.All other flags in
CylinderFlags_tuse1 << N(signed literal), while this new entry uses1u << 8. Neither causes a bug at bit 8, but the inconsistency is a style nit. Consider using1 << 8for consistency, or converting all entries to1u << Nif you want to be explicit about unsigned shifts.tests/integration/game/money_it.cpp (2)
79-88: Test isolation: destructor does not restore coinItemTypeslots modified within the original range.The destructor restores the vector size but not the contents of slots below
originalSize_. IfITEM_GOLD_COIN,ITEM_PLATINUM_COIN, orITEM_CRYSTAL_COINfall within the pre-existing range and hadid == 0,setupCoinwill have written to them, and that modification persists after~ItemTypeScope(). This is unlikely to cause problems if tests run in isolation, but could introduce subtle ordering dependencies if other tests in the same process rely on those slots being default-initialized.Consider saving and restoring the original
ItemTypeat each coin index, or documenting that coin slots may be permanently modified.
170-178: LGTM on the bank-debit test.Clean verification that
removeMoneywith the bank-debit flag falls through to the player's bank balance when no inventory money is available.One thought: consider adding a complementary test where the player has partial inventory money and the remainder is debited from the bank, to exercise the split-source path described in the PR objectives.
src/lua/functions/creatures/player/player_functions.cpp (1)
2473-2489: Simplify the flags parameter extraction.Line 2478 reads flags as
int32_tand then casts touint32_t, which is a roundabout path. ThegetNumber<uint32_t>overload already handles negative inputs by clamping to 0 (as seen inlua_functions_loader.hppLines 65-85). Use it directly:♻️ Suggested simplification
- const auto flags = static_cast<uint32_t>(Lua::getNumber<int32_t>(L, 3, 0)); + const auto flags = Lua::getNumber<uint32_t>(L, 3, 0);
Npc: when selling items for non-gold currency, clone removed items for potential refund and return them to the player if delivering the reward items fails; update autobank flow to send bank balance and improve player message. Game: validate bank debit by checking expected vs actual balance and log an error on mismatch (use debug for successful debit). Items: use unsigned bit-shifts (1u << n) for cylinder flags and define FLAG_DROPONMAP. Lua: read flags as uint32_t in luaPlayerAddMoney. Tests: prevent overwriting default coin ItemType definitions by stashing/restoring original coin types and add helper functions and includes for the test adjustments.
Cleanup test file by removing unused headers: <gtest/gtest.h>, <algorithm>, <exception>, <filesystem>, and <optional> from tests/integration/game/money_it.cpp. This reduces unnecessary dependencies and compilation noise; no functional changes to the test logic.
Replace FLAG_DROPONMAP with 0 when calling g_game().addMoney during NPC buys so the payment is added directly instead of being dropped on the map. This prevents coins from appearing on the ground when a player sells items to an NPC and keeps deliveredAmount logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Around line 608-622: In Npc::onPlayerSellItem handle failures from
g_game().internalPlayerAddItem when refunding removedItemsForRefund: detect
per-item failures and for any refundItem where refundRet != RETURNVALUE_NOERROR
attempt a fallback (e.g., deposit to the player's depot/mailbox/inbox via the
existing player API or g_game() helper) and only call
player->sendTextMessage("...returned...") if all refunds succeeded; otherwise
send a conditional message indicating partial/failed returns and log the refund
item IDs and errors for owners (use refundItem->getID(), player->getName(),
getReturnMessage(refundRet) to identify failures).
In `@src/game/game.cpp`:
- Around line 2726-2733: The change making logic may abort delivery when
addMoney fails with RETURNVALUE_NOTENOUGHROOM or
RETURNVALUE_CONTAINERNOTENOUGHROOM because the retry-to-drop branch only checks
RETURNVALUE_NOTENOUGHCAPACITY; update the addMoney retry so that if addedMoney <
expectedChange and FLAG_DROPONMAP is not already set in flags, call
addMoney(cylinder, expectedChange, flags | FLAG_DROPONMAP) to attempt dropping
on the map (use the same addedMoney/returnValue tuple to continue flow);
reference addMoney, expectedChange, flags, FLAG_DROPONMAP,
RETURNVALUE_NOTENOUGHCAPACITY, RETURNVALUE_NOTENOUGHROOM, and
RETURNVALUE_CONTAINERNOTENOUGHROOM and ensure the retry guard prevents
re-entering when FLAG_DROPONMAP is already present.
🧹 Nitpick comments (2)
src/creatures/npcs/npc.cpp (1)
593-597: Technical return message exposed in player-facing text.
getReturnMessage(returnValue)is appended to the player-visible message. These return messages are engine-internal strings (e.g.,"There is not enough room.") and may not read naturally as part of a sentence. Consider either dropping it from the player message (it's already in the log at line 591) or wrapping it in a more user-friendly phrasing like"Reason: {}".💡 Suggested improvement
- std::string msg = fmt::format( - "Only {} of {} gold coins were delivered to your inventory. The remaining {} was deposited in your bank. {}", - addedMoney, totalCost, refund, getReturnMessage(returnValue) - ); + std::string msg = fmt::format( + "Only {} of {} gold coins were delivered to your inventory. The remaining {} was deposited in your bank.", + addedMoney, totalCost, refund + );tests/integration/game/money_it.cpp (1)
20-22: Consider a shared or money-specific test config instead of reusingimbuements_test.lua.Using a config named after imbuements for money integration tests is misleading. If the config is generic enough for both, renaming it to something neutral (e.g.,
integration_test.lua) would improve clarity. Alternatively, create a dedicatedmoney_test.luaif the tests diverge later.
Npc: When returning sold items after a currency delivery failure, attempt a fallback refund into the player's inbox (internalAddItem with FLAG_NOLIMIT) if internalPlayerAddItem fails. Track whether all refunds succeeded and log both primary and fallback errors; update the player-facing message to indicate partial failures when some items couldn't be returned. Game: Tighten the removeMoney change handling: only retry adding change with FLAG_DROPONMAP when the original addMoney shortfall is due to capacity/room/container errors (NOTENOUGHCAPACITY, NOTENOUGHROOM, CONTAINERNOTENOUGHROOM) and FLAG_DROPONMAP wasn't already set. This avoids unnecessary drops and improves error handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Around line 603-635: Item::CreateItem(getCurrency(), totalCost) can return
nullptr and the current code silently continues, losing removed items; modify
the onPlayerSellItem flow to detect when newItem is null and treat it like a
delivery failure: iterate removedItemsForRefund and attempt to return each item
to the player using g_game().internalPlayerAddItem (falling back to adding to
player->getInbox() via g_game().internalAddItem with INDEX_WHEREEVER and
FLAG_NOLIMIT), log failures with the same pattern used for delivery errors, send
the player a MESSAGE_EVENT_ADVANCE explaining the refund status, and return
early (like the existing error branch) so the Lua callback is not invoked on a
failed sale.
In `@src/game/game.cpp`:
- Around line 2828-2837: Change the coin count variables crystalCoins and
platinumCoins from 32-bit to 64-bit to match money (uint64_t) so large balances
won't truncate; compute crystalCoins = money / 10000 and platinumCoins = money /
100 as uint64_t, subtract appropriately from money, then call
addCoins(ITEM_CRYSTAL_COIN, crystalCoins, 10000) and
addCoins(ITEM_PLATINUM_COIN, platinumCoins, 100) as before, but ensure the
per-stack createCount passed to addCoins remains capped at 100 (i.e., when
splitting large uint64_t counts into stacks, limit each stack size to 100).
Include the inbox header and pass the player's inbox to g_game().internalAddItem as a std::static_pointer_cast<Cylinder>(inbox). This ensures the call matches the expected Cylinder parameter (avoiding overload/type issues) when attempting to add refund items to the player's inbox (INDEX_WHEREEVER, FLAG_NOLIMIT). Logic for handling fallback and tracking allRefunded remains unchanged.
Npc: Split creation vs delivery failure when giving custom currency in Npc::onPlayerSellItem. Now checks Item::CreateItem() result first, logs a clearer error on creation failure, attempts to refund removed items (with inbox fallback), notifies the player, and returns early. Delivery failure handling remains but is executed only after a successful creation. Game: In Game::addMoney, use 64-bit counters for coin quantities (crystal/platinum) and ensure createCount is cast to uint16_t when calling Item::CreateItem. This prevents potential overflow/narrowing issues when adding large amounts of money.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/creatures/npcs/npc.cpp (1)
568-568:⚠️ Potential issue | 🟠 MajorInteger overflow before the
uint64_tcast.
sellPriceisuint32_tandtotalRemovedis promoted touint32_t, so the multiplication is performed in 32-bit arithmetic before the cast touint64_t. IfsellPrice * totalRemoved > UINT32_MAX(e.g., sell price 100,000 × 50,000 items = 5 billion), the result wraps around silently.🐛 Fix: widen before multiplying
- auto totalCost = static_cast<uint64_t>(sellPrice * totalRemoved); + auto totalCost = static_cast<uint64_t>(sellPrice) * totalRemoved;
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Line 604: totalCost (uint64_t) is being narrowed to uint16_t when calling
Item::CreateItem(getCurrency(), totalCost); fix by breaking totalCost into
chunks that fit the Item::CreateItem count parameter (max UINT16_MAX) and create
multiple currency items in a loop; use getCurrency() and Item::CreateItem(type,
count) repeatedly (subtracting each chunk from totalCost) and insert each
created item the same way the current code handles newItem, modeling the
batching behavior used by g_game().addMoney().
In `@src/game/game.cpp`:
- Around line 2793-2841: The addMoney implementation increments totalAdded by
createCount without accounting for partial insertions; update the lambda used in
Game::addMoney (addCoins) to call the internalAddItem overload that returns a
remainderCount, subtract placed = createCount - remainderCount from totalAdded
(use placed * unitValue), and set returnValue to the appropriate non‑NOERROR
when any remainder remains (or preserve existing ret if fallback to tile only
partially succeeds); ensure you also handle the FLAG_DROPONMAP fallback by
checking its remainderCount and updating totalAdded/returnValue consistently.
🧹 Nitpick comments (1)
src/creatures/npcs/npc.cpp (1)
610-675: Duplicate refund logic — extract a helper.The refund blocks at lines 610–639 and 646–675 are nearly identical (~30 lines each), differing only in the log message context string. This duplication makes future maintenance error-prone (e.g., adding a new fallback strategy requires changes in two places).
♻️ Proposed refactor — extract a helper method or lambda
+// Add as a private method or a file-local helper: +// Returns true if all items were successfully refunded. +static bool refundRemovedItems( + const std::shared_ptr<Player> &player, + const std::vector<std::shared_ptr<Item>> &removedItems, + const std::string &errorContext +) { + bool allRefunded = true; + for (const auto &refundItem : removedItems) { + auto refundRet = g_game().internalPlayerAddItem(player, refundItem, true); + if (refundRet != RETURNVALUE_NOERROR) { + ReturnValue fallbackRet = RETURNVALUE_NOTPOSSIBLE; + if (const auto &inbox = player->getInbox()) { + fallbackRet = g_game().internalAddItem( + std::static_pointer_cast<Cylinder>(inbox), + refundItem, INDEX_WHEREEVER, FLAG_NOLIMIT + ); + } + if (fallbackRet != RETURNVALUE_NOERROR) { + allRefunded = false; + g_logger().error( + "[Npc::onPlayerSellItem] - Failed to refund item {} to player {} {} (primary: {}, fallback: {}).", + refundItem->getID(), player->getName(), errorContext, + getReturnMessage(refundRet), getReturnMessage(fallbackRet) + ); + } + } + } + return allRefunded; +}Then both call sites collapse to:
if (!removedItemsForRefund.empty()) { bool allRefunded = refundRemovedItems(player, removedItemsForRefund, "after currency creation failure"); player->sendTextMessage(MESSAGE_EVENT_ADVANCE, allRefunded ? "Could not deliver the reward items. Your sold items were returned to you." : "Could not deliver the reward items. Some sold items could not be returned; please contact staff." ); }
Refactor NPC sell flow to create and deliver custom currency in safe-sized chunks, with explicit rollback and refund lambdas. Adds refundSoldItems and rollbackCurrency helpers to reliably return sold items or remove already-delivered currency when currency creation or delivery fails, and improves error logging with failure context. Update Game::addMoney to account for partial placements by using a remainder count from internalAddItem, correctly increment totalAdded only for actually placed units, and handle drop-on-map fallback for remaining items, returning appropriate errors when there is not enough room.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Around line 663-692: The current chunking in Npc::onPlayerSellItem uses
std::numeric_limits<uint16_t>::max() which can exceed the actual item stack size
for custom currency; change the chunk cap to use the currency item's stack size
(e.g., retrieve the stack size from the created currency item or its ItemType
via Item::CreateItem/getStackSize or getItemType()->stackSize) so each chunk <=
stackSize (like Game::addMoney uses 100). Ensure chunk is computed as
min(remaining, static_cast<uint64_t>(stackSize)) before creating/adding the item
and preserve existing rollback/refund and deliveredCurrency updates.
🧹 Nitpick comments (1)
src/creatures/npcs/npc.cpp (1)
641-661:rollbackCurrencyremoval may be partial with no accounting of how much was actually removed.If
removeItemOfTypereturnsfalse(couldn't remove the full chunk), the loop breaks with only an error log. SinceremoveItemOfTypedoesn't report how many it actually removed, the player could retain some extra currency. This is acceptable as a best-effort error path, but consider loggingremaining(notdeliveredCurrency) for more useful diagnostics.📝 Improved error log
if (!player->removeItemOfType(getCurrency(), chunk, -1, false)) { g_logger().error( - "[Npc::onPlayerSellItem] - Failed to rollback {} currency items for player {} after {} failure.", - deliveredCurrency, player->getName(), context + "[Npc::onPlayerSellItem] - Failed to rollback currency for player {} after {} failure. Attempted to remove {} of {} total delivered.", + player->getName(), context, chunk, deliveredCurrency ); break; }
When giving currency items in Npc::onPlayerSellItem, split the total into chunks limited by the currency item's stackSize (or 1 for non-stackable) instead of using uint16_t::max. This prevents creating currency stacks larger than the item's allowed stack size. Update the events scheduler test fixture to use a temporary runtime Lua config file: write a runtime config with coreDirectory, point g_configManager to that file, reload as needed, and remove the runtime file on teardown. Added helper methods and a runtimeConfigFile_ member to manage the temporary config and cleanup after tests.
Extract helper functions and restructure money and NPC sell flows: move NPC sell item logic into small helpers (price lookup, item removal, gold/custom-currency delivery, refunds/rollback) to improve readability and error handling. Rework Game::removeMoney / addMoney internals by introducing collection/process helpers, safer coin stacking and drop-on-map logic, and clearer failure handling when delivering change. Update tests to use a ScopeExit-based cleanup for ItemTypeScope and modernize some const/attribute usage. Overall fixes duplicate code, improves logging/metrics, and makes currency/refund edge cases more robust.
Refactor cleanup logic in tests/integration/game/money_it.cpp: remove the custom ScopeExit class and introduce ItemTypeScope::CleanupGuard to own CleanupState and perform restoration in its destructor. Update cleanup_ to std::optional<CleanupGuard> and use emplace(std::move(cleanupState)) instead of capturing a lambda. Also add missing <filesystem> and <optional> includes. Functional behavior is preserved while simplifying and localizing the cleanup implementation.
Modernize and simplify money-related code and test cleanup. Replaced explicit types with auto and structured bindings, introduced `using enum MoneyRemovalOutcome` for clearer enum usage, and unified remove-count handling when deleting items. Adjusted money collection emplace to use the inserted key when updating totals and simplified container iteration. In tests, replaced stack-based CleanupGuard with a heap-allocated CleanupState managed by a unique_ptr and custom deleter to provide noexcept, best-effort cleanup.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Around line 187-206: rollbackCurrency currently calls Player::removeItemOfType
which removes any item of that currencyId (including pre-existing ones); instead
modify deliverCustomCurrency to collect and return (or populate) the actual
std::vector<std::shared_ptr<Item>> of items it added, then change
rollbackCurrency to accept that vector and remove those exact shared_ptr<Item>
instances (e.g., call a Player method that removes a specific item by
pointer/id) rather than by type; update function signatures of
deliverCustomCurrency and rollbackCurrency (or add an overload) to pass the
delivered-items list, iterate that list to remove each item and log per-item
failures, and avoid using removeItemOfType in rollbackCurrency.
🧹 Nitpick comments (1)
src/creatures/npcs/npc.cpp (1)
80-88: Narrowing cast fromuint16_ttouint8_tonremoveCount.
removeCountisuint16_tbutsetItemCounttakesuint8_t. In practice this is safe because stackable item counts are bounded bystackSize(typically ≤ 100), but it's a silent truncation if assumptions change.Consider making the intent explicit:
- refundItem->setItemCount(static_cast<uint8_t>(removeCount)); + refundItem->setItemCount(static_cast<uint8_t>(std::min<uint16_t>(removeCount, std::numeric_limits<uint8_t>::max())));
src/creatures/npcs/npc.cpp
Outdated
| void rollbackCurrency(const std::shared_ptr<Player> &player, uint16_t currencyId, uint64_t deliveredCurrency, std::string_view context) { | ||
| if (deliveredCurrency == 0) { | ||
| return; | ||
| } | ||
|
|
||
| uint64_t remaining = deliveredCurrency; | ||
| while (remaining > 0) { | ||
| const auto chunk = static_cast<uint32_t>( | ||
| std::min<uint64_t>(remaining, std::numeric_limits<uint32_t>::max()) | ||
| ); | ||
| if (!player->removeItemOfType(currencyId, chunk, -1, false)) { | ||
| g_logger().error( | ||
| "[Npc::onPlayerSellItem] - Failed to rollback {} currency items for player {} after {} failure.", | ||
| deliveredCurrency, player->getName(), context | ||
| ); | ||
| break; | ||
| } | ||
| remaining -= chunk; | ||
| } | ||
| } |
There was a problem hiding this comment.
rollbackCurrency may remove pre-existing player currency, not just delivered items.
removeItemOfType removes any items matching currencyId from the player's inventory — it does not distinguish between items that were just delivered by deliverCustomCurrency and items the player already had. If the player held some of this custom currency before the sale, the rollback could remove those pre-existing items.
A safer approach would be to track the actual shared_ptr<Item> instances added during delivery and remove those specific items:
Sketch of a safer approach
In deliverCustomCurrency, collect delivered items:
+ std::vector<std::shared_ptr<Item>> deliveredItems;
while (remaining > 0) {
// ... create chunkItem ...
auto returnValue = g_game().internalPlayerAddItem(player, chunkItem, true);
if (returnValue != RETURNVALUE_NOERROR) {
- rollbackCurrency(player, currencyId, deliveredCurrency, "currency delivery");
+ for (const auto &item : deliveredItems) {
+ g_game().internalRemoveItem(item, item->getItemCount());
+ }
// ... refund logic ...
}
+ deliveredItems.push_back(chunkItem);
deliveredCurrency += chunk;🤖 Prompt for AI Agents
In `@src/creatures/npcs/npc.cpp` around lines 187 - 206, rollbackCurrency
currently calls Player::removeItemOfType which removes any item of that
currencyId (including pre-existing ones); instead modify deliverCustomCurrency
to collect and return (or populate) the actual
std::vector<std::shared_ptr<Item>> of items it added, then change
rollbackCurrency to accept that vector and remove those exact shared_ptr<Item>
instances (e.g., call a Player method that removes a specific item by
pointer/id) rather than by type; update function signatures of
deliverCustomCurrency and rollbackCurrency (or add an overload) to pass the
delivered-items list, iterate that list to remove each item and log per-item
failures, and avoid using removeItemOfType in rollbackCurrency.
Address multiple issues in npc.cpp around item refunds and currency rollback during player sell operations. - Ensure refund item counts fit in uint8_t by clamping removeCount to numeric_limits<uint8_t>::max() before casting, avoiding overflow when cloning stackable items. - Change rollbackCurrency signature to accept an originalCurrency snapshot and deliveredCurrency, and compute remaining rollback amount using the player's current currency (current - original) instead of using deliveredCurrency directly. This prevents incorrect/no-op rollbacks when the player's currency has changed. - Adjust logging to report the actual remaining amount attempted to rollback and include context. - Capture originalCurrency before attempting to deliver currency so rollback can restore to the prior state if delivery/creation fails. These changes improve safety and correctness when refunding items and rolling back custom currency operations in NPC shop flows.
Add countInventoryCurrency to iterate the player's inventory and sum stack counts for a given currency ID using Item::countByType. Replace uses of player->getItemTypeCount(currencyId) with this helper in rollback/refund code so currency totals correctly reflect stacked items when computing original and delivered amounts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/creatures/npcs/npc.cpp`:
- Around line 787-796: The code only increments totalPrice when getCurrency() ==
ITEM_GOLD_COIN, causing undercount for custom-currency NPCs; in the else branch
around deliverCustomCurrency(player, getCurrency(), totalCost,
removal.refundItems, getName()) add logic to update totalPrice by totalCost when
deliverCustomCurrency succeeds (i.e., after delivered is true) so the
accumulated total used by onPlayerSellAllLoot is accurate, or if custom
currencies should never affect totalPrice, add a clear comment on that
constraint referencing getCurrency(), ITEM_GOLD_COIN, totalPrice,
deliverCustomCurrency(), and deliverGoldSale() to document the intent.
🧹 Nitpick comments (1)
src/creatures/npcs/npc.cpp (1)
36-43: Considerstd::find_ifinstead of a raw loop.Static analysis (cppcheck) flags this. A small readability win:
♻️ Proposed refactor
uint32_t getSellPrice(const std::vector<ShopBlock> &shopVector, const ItemType &itemType) { - for (const auto &shopBlock : shopVector) { - if (itemType.id == shopBlock.itemId && shopBlock.itemSellPrice != 0) { - return shopBlock.itemSellPrice; - } - } - return 0; + auto it = std::find_if(shopVector.begin(), shopVector.end(), [&](const auto &shopBlock) { + return itemType.id == shopBlock.itemId && shopBlock.itemSellPrice != 0; + }); + return it != shopVector.end() ? it->itemSellPrice : 0; }
Ensure totalPrice is incremented when delivering items in Npc::onPlayerSellItem. Previously totalPrice was not updated per delivered item, causing the reported total to be incorrect for the npc:onSellItem callback. This adds totalPrice += totalCost inside the delivery loop to accumulate the full sale amount.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bank: check the result of credit(refund) and log an error if the refund fails, otherwise keep the existing warning message; preserves balance snapshots in the log. Tests: make writeRuntimeConfigFile() return a bool and mark it [[nodiscard]]; caller now ASSERT_TRUE(...) with a failure message. The helper now verifies the file opened and returns file.good() to indicate success.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/game/bank/bank.cpp`:
- Around line 153-157: In Bank::withdraw, when addedMoney > amount you must not
only log the inconsistency but also claw back the excess and ensure state/return
reflects the anomaly: calculate excess = addedMoney - amount and call
g_game().removeMoney(player, excess) (or otherwise credit the bank/meter the
difference) and update any metrics/returned value to reflect the original
requested amount; ensure you reference addedMoney, amount, player and perform
the removeMoney call and any metric/return adjustments immediately after the
existing g_logger().error call.
- Around line 184-185: The withdraw function currently returns true even when no
gold was delivered; update the return logic in withdraw (referencing the
addedMoney variable and the existing g_metrics().addCounter call) so that if
addedMoney == 0 the function returns false (while still recording the metric),
otherwise return true; ensure this handles the debit-then-refund path where
nothing was delivered so callers see failure when no funds were given.
🧹 Nitpick comments (2)
src/game/bank/bank.cpp (2)
149-151: Dead store:refundis initialized but the initial value is never read.The cppcheck warning is valid.
refundis set to0on line 151 but only ever read after reassignment on line 159. Declare it at point of use instead.Suggested fix
uint32_t flags = 0; auto [addedMoney, returnValue] = g_game().addMoney(player, amount, flags); - uint64_t refund = 0; if (addedMoney > amount) { ... } else if (addedMoney < amount) { - refund = amount - addedMoney; + uint64_t refund = amount - addedMoney;
149-149:flagsis always zero — consider clarifying intent or removing the variable.
flagsis declared and passed toaddMoneybut is always0. If this is a placeholder for future use, a brief comment would help; otherwise passing0directly (or a named constant/default parameter) is cleaner.
Improve robustness of Bank::withdraw by handling cases where g_game().addMoney delivers more than requested: log the inconsistency, attempt to remove the excess with g_game().removeMoney, and cap addedMoney to the requested amount. Make addMoney flags explicit (constexpr) and limit refund scope to where it's needed. Also change the function return to indicate success only when some money was actually delivered (return addedMoney != 0). Additional error logging is added if clawback fails.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/game/bank/bank.cpp`:
- Around line 166-189: In Bank::withdraw, avoid sending the "Only 0 of ..."
message when addedMoney == 0 to prevent a confusing duplicate failure notice;
update the logic around addedMoney and player->sendTextMessage so that if
addedMoney == 0 you either skip the player->sendTextMessage entirely (letting
the caller handle full failure) or send distinct wording for total failure
(e.g., a single failure message referencing returnValue), and ensure you still
log the refund via g_logger() and return the existing boolean result; adjust
checks around addedMoney, refund, and the call site expectations accordingly.
Wrap the partial-delivery player message in a check for addedMoney > 0 so the player isn't notified when zero coins are delivered during a withdraw. Keeps the balance_decrease metric update unchanged. Prevents misleading notifications in edge cases where nothing was added to the inventory.
|
|
This PR is stale because it has been open 45 days with no activity. |
This pull request significantly refactors and improves the item selling and currency delivery logic for NPCs, as well as enhances the reliability and transparency of bank withdrawals. The changes introduce helper functions for selling items, handling refunds, and delivering both gold and custom currency, with robust error handling and rollback mechanisms. Additionally, bank withdrawals now properly handle partial deliveries and provide user feedback and logging.
NPC Sell & Currency Handling Improvements:
removeSellItems,deliverGoldSale,deliverCustomCurrency, andrefundSoldItemsto improve readability, maintainability, and error handling. These helpers manage item removal, gold delivery (with autobank support), custom currency creation, and refunding items when failures occur.onPlayerSellItemto use the new helpers, simplifying logic and ensuring correct handling of refunds and currency delivery. [1] [2]inbox.hppinclude to support fallback refund delivery to player inbox when direct inventory addition fails.Bank Withdrawal Reliability:
Bank::withdrawto handle partial gold delivery: if not all gold can be delivered to the player, the remainder is refunded to the bank, with appropriate logging and user messaging. Metrics now accurately reflect the actual amount delivered.Summary by CodeRabbit
Bug Fixes
Refactor
Tests