Improvements and fixes for market transactions; code quality improvements #291
Conversation
…rader features; rename TranserFunds to TransferFunds; code legibility improvements
| @@ -396,7 +396,7 @@ | |||
| (itemID == npcTraderJoe) | |||
|
|
|||
| #define IsTrader(itemID) \ | |||
| (((itemID >= minTrader) && (itemID <= maxTrader)) | |||
| ((itemID >= minTrader) && (itemID <= maxTrader)) | |||
There was a problem hiding this comment.
This macro contained an extra parenthetical at the beginning, causing usage of IsTrader to fail immediately anywhere. I have fixed it here.
| TransferFunds( | ||
| call.client->GetCharacterID(), | ||
| toID->value(), | ||
| amount->value(), | ||
| reasonStr.c_str(), | ||
| Journal::EntryType::PlayerDonation, | ||
| call.client->GetCharacterID() | ||
| ); | ||
|
|
There was a problem hiding this comment.
Legibility improvements only.
| TransferFunds( | ||
| call.client->GetCorporationID(), | ||
| toID->value(), | ||
| amount->value(), | ||
| reason.c_str(), | ||
| Journal::EntryType::CorporationAccountWithdrawal, | ||
| call.client->GetCharacterID(), | ||
| fromAcctKey->value(), | ||
| toAcctKey, | ||
| call.client | ||
| ); | ||
|
|
There was a problem hiding this comment.
Legibility improvements only.
| void AccountService::TransferFunds( | ||
| uint32 fromID, | ||
| uint32 toID, | ||
| double amount, | ||
| std::string reason /*""*/, | ||
| uint8 entryTypeID /*Journal::EntryType::Undefined*/, | ||
| uint32 referenceID /*0*/, | ||
| uint16 fromKey /*Account::KeyType::Cash*/, | ||
| uint16 toKey /*Account::KeyType::Cash*/, | ||
| Client* pClient /*nullptr*/ | ||
| ) { | ||
| if (is_log_enabled(ACCOUNT__TRACE)) { | ||
| _log(ACCOUNT__TRACE, | ||
| "TransferFunds() - from: %u, to: %u, entry: %u, refID: %u, amount: %.2f, fKey: %u, tKey: %u", | ||
| fromID, | ||
| toID, | ||
| entryTypeID, | ||
| referenceID, | ||
| amount, | ||
| fromKey, | ||
| toKey | ||
| ); | ||
| } | ||
|
|
||
| uint8 fromCurrency = Account::CreditType::ISK; | ||
|
|
There was a problem hiding this comment.
Legibility improvements only.
|
|
||
| Client* pClientFrom(nullptr); | ||
|
|
||
| if (IsCharacterID(fromID)) { | ||
| pClientFrom = sEntityList.FindClientByCharID(fromID); | ||
| if (pClientFrom == nullptr) { | ||
| // sender is offline. xfer funds thru db. | ||
| // sender is offline. transfer funds through the database instead | ||
| newBalanceFrom = AccountDB::OfflineFundXfer(fromID, -amount, fromCurrency); | ||
| } else { | ||
| // this will throw if it fails | ||
| pClientFrom->AddBalance(-amount, fromCurrency); | ||
| newBalanceFrom = pClientFrom->GetBalance(fromCurrency); | ||
| } | ||
| AccountDB::AddJournalEntry(fromID, entryTypeID, fromID, toID, fromCurrency, fromKey, -amount, newBalanceFrom, reason, referenceID); | ||
|
|
||
| AccountDB::AddJournalEntry( | ||
| fromID, | ||
| entryTypeID, | ||
| fromID, | ||
| toID, | ||
| fromCurrency, | ||
| fromKey, | ||
| -amount, | ||
| newBalanceFrom, | ||
| reason, | ||
| referenceID | ||
| ); |
There was a problem hiding this comment.
Legibility improvements only.
|
|
||
| AccountService::TransferFunds( | ||
| call.client->GetCharacterID(), | ||
| corpSCC, | ||
| amount->value(), | ||
| reason, | ||
| Journal::EntryType::Insurance, | ||
| -shipRef->itemID() | ||
| ); // for paying ins, shipID should be negative |
There was a problem hiding this comment.
Legibility improvements only.
| AccountService::TranserFunds(corpCONCORD, cur.first, cur.second.amount, reason, Journal::EntryType::BountyPrizes, m_data.systemID); | ||
|
|
||
| AccountService::TransferFunds(corpCONCORD, cur.first, cur.second.amount, reason, Journal::EntryType::BountyPrizes, m_data.systemID); | ||
| count = 0; |
There was a problem hiding this comment.
Legibility improvements only.
| std::string method_name ("GetOrders_"); | ||
| method_name += std::to_string(regionID); | ||
| method_name += "_"; | ||
| method_name += std::to_string(typeID); | ||
| ObjectCachedMethodID method_id("marketProxy", method_name.c_str()); | ||
| this->m_cache->InvalidateCache( method_id ); | ||
| this->m_cache->InvalidateCache(method_id); |
There was a problem hiding this comment.
Legibility improvements only.
| /* | ||
| * Does not currently handle aurum transactions, but it would be good to do that | ||
| * in the future. | ||
| */ | ||
| bool MarketMgr::ExecuteBuyOrder(Client* seller, uint32 orderID, InventoryItemRef iRef, uint32 quantity, bool useCorp, uint32 typeID, uint32 stationID, double price, uint16 accountKey/*Account::KeyType::Cash*/) { | ||
| Market::OrderInfo oInfo = Market::OrderInfo(); | ||
| if (!MarketDB::GetOrderInfo(orderID, oInfo)) { | ||
| _log(MARKET__ERROR, "ExecuteBuyOrder - Failed to get order info for #%u.", orderID); | ||
| return true; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /* will this method also be used to buy/sell using aurm? | ||
| * unknown yet | ||
| */ | ||
| // get buyer id and determine if buyer is player, corp, or npc/bot | ||
| bool isPlayer(false), isCorp(false), isTraderJoe(false), isTrader(false); | ||
|
|
||
| // get buyer id and determine if buyer is player or corp (or bot for later) | ||
| bool isPlayer(false), isCorp(false), isTrader(false); | ||
| if (IsPlayerCorp(oInfo.ownerID)) { | ||
| // buyer is player corp | ||
| isCorp = true; | ||
| } else if (oInfo.ownerID == 1) { | ||
| oInfo.ownerID = stDataMgr.GetOwnerID(stationID); | ||
| } else if (IsCharacterID(oInfo.ownerID)) { | ||
| isPlayer = true; | ||
| } else if (IsTraderJoe(oInfo.ownerID)) { | ||
| isTraderJoe = true; | ||
| } else if (IsTrader(oInfo.ownerID)) { |
There was a problem hiding this comment.
This function as well as ExecuteSellOrder have been slightly refactored and modified to support the aims of this pull request.
| // set an upper bound (this used to be a while loop that spun | ||
| // forever in some cases) | ||
| for (int i = 0; i < 1000; i++) { | ||
| _log(MARKET__DUMP, "Mkt::PlaceCharOrder(): issue 16 - finding buy order: %i, %i, %i, %.2f", typeID->value(), stationID->value(), quantity->value(), price->value()); | ||
|
|
||
| orderID = MarketDB::FindBuyOrder(typeID->value(), stationID->value(), quantity->value(), price->value()); | ||
| if (orderID) { | ||
| _log(MARKET__TRACE, "PlaceCharOrder - Found buy order #%u in %s for %s.", \ | ||
| orderID, stDataMgr.GetStationName(stationID->value()).c_str(), call.client->GetName()); | ||
| search = sMktMgr.ExecuteBuyOrder(call.client, orderID, iRef, quantity->value(), useCorp->value(), typeID->value(), stationID->value(), price->value()); | ||
|
|
||
| if (!orderID) { | ||
| continue; | ||
| } | ||
|
|
||
| _log(MARKET__TRACE, | ||
| "PlaceCharOrder - Found buy order #%u in %s for %s.", | ||
| orderID, | ||
| stDataMgr.GetStationName(stationID->value()).c_str(), | ||
| call.client->GetName() | ||
| ); | ||
|
|
||
| completedOrder = sMktMgr.ExecuteBuyOrder( | ||
| call.client, | ||
| orderID, | ||
| iRef, | ||
| quantity->value(), | ||
| useCorp->value(), | ||
| typeID->value(), | ||
| stationID->value(), | ||
| price->value() | ||
| ); | ||
|
|
||
| if (!completedOrder) { | ||
| continue; | ||
| } | ||
|
|
||
| _log(MARKET__DUMP, "Mkt::PlaceCharOrder(): issue 16 - order completed"); | ||
| break; |
There was a problem hiding this comment.
The code changes in this PR fix infinite loop issues that occurred in the previous version of this function, right here.
Reviewer's Guide by SourceryThis pull request enhances market transaction behaviors, adds support for NPC traders, fixes transaction-related issues, renames a key function for clarity, and improves code formatting and legibility. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @charles-m-knox - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider splitting the PR into smaller, more focused changes to facilitate easier review and testing.
- Add more inline comments to explain complex logic, particularly around NPC trader handling and transaction adjustments.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| data.accountKey = Account::KeyType::Cash; | ||
|
|
||
| data.clientID = clientID->IsNone() ? 0 : PyRep::IntegerValueU32(clientID); | ||
| data.isBuy = sellBuy->IsNone() ? -1 : PyRep::IntegerValueU32(sellBuy); |
There was a problem hiding this comment.
suggestion: Consider replacing the magic number '-1' with a named constant for better readability.
Using a named constant instead of '-1' for the isBuy field would improve code readability and maintainability. It would make it clear what this value represents, reducing potential confusion for future developers.
| data.isBuy = sellBuy->IsNone() ? -1 : PyRep::IntegerValueU32(sellBuy); | |
| const int BUY_UNDEFINED = -1; | |
| data.isBuy = sellBuy->IsNone() ? BUY_UNDEFINED : PyRep::IntegerValueU32(sellBuy); |
| if (is_log_enabled(ACCOUNT__TRACE)) | ||
| _log(ACCOUNT__TRACE, "TranserFunds() - from: %u, to: %u, entry: %u, refID: %u, amount: %.2f, fKey: %u, tKey: %u", \ | ||
| fromID, toID, entryTypeID, referenceID, amount, fromKey, toKey); | ||
| void AccountService::TransferFunds( |
There was a problem hiding this comment.
suggestion: Consider breaking down the long function into smaller helper functions.
The TransferFunds function is quite long and could benefit from being broken down into smaller, focused helper functions. This would improve readability and maintainability.
void AccountService::TransferFunds(uint32 fromID, uint32 toID, uint32 entryTypeID, uint32 referenceID, double amount, uint32 fromKey, uint32 toKey) {
if (is_log_enabled(ACCOUNT__TRACE)) {
LogTransferDetails(fromID, toID, entryTypeID, referenceID, amount, fromKey, toKey);
}
// Existing logic for transferring funds
}
void AccountService::LogTransferDetails(uint32 fromID, uint32 toID, uint32 entryTypeID, uint32 referenceID, double amount, uint32 fromKey, uint32 toKey) {
_log(ACCOUNT__TRACE, "TransferFunds() - from: %u, to: %u, entry: %u, refID: %u, amount: %.2f, fKey: %u, tKey: %u",
fromID, toID, entryTypeID, referenceID, amount, fromKey, toKey);
}
There was a problem hiding this comment.
Refactoring this function is out of scope, but... good bot.
There was a problem hiding this comment.
Thanks for acknowledging the suggestion! It's completely understandable to keep the scope focused on the current improvements and fixes. If you decide to revisit this area in the future, breaking down the function could enhance readability and maintainability. Let me know if there's anything else you'd like to discuss or need help with!
| /* | ||
| * Does not currently handle aurum transactions, but it would be good to do that | ||
| * in the future. | ||
| */ | ||
| bool MarketMgr::ExecuteBuyOrder(Client* seller, uint32 orderID, InventoryItemRef iRef, uint32 quantity, bool useCorp, uint32 typeID, uint32 stationID, double price, uint16 accountKey/*Account::KeyType::Cash*/) { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the quantity handling logic into separate functions.
The ExecuteBuyOrder function has become complex due to multiple boolean flags and nested conditionals. To improve readability and maintainability, consider refactoring the quantity handling logic into separate functions. This will reduce nesting and make the code more modular.
Here's a suggestion:
void HandleCompleteOrder(InventoryItemRef iRef, Market::OrderInfo& oInfo, uint32 stationID, bool isPlayer, bool isCorp) {
if (isPlayer) {
iRef->Donate(oInfo.ownerID, stationID, flagHangar, true);
} else if (isCorp) {
iRef->Donate(oInfo.ownerID, stationID, flagCorpMarket, true);
}
}
void HandleOverOrder(InventoryItemRef iRef, Market::OrderInfo& oInfo, uint32 stationID, bool isPlayer, bool isCorp) {
InventoryItemRef siRef = iRef->Split(oInfo.quantity);
if (siRef.get() == nullptr) {
_log(MARKET__ERROR, "Failed to split item.");
return;
}
if (isPlayer) {
siRef->Donate(oInfo.ownerID, stationID, flagHangar, true);
} else if (isCorp) {
siRef->Donate(oInfo.ownerID, stationID, flagCorpMarket, true);
}
}
// In ExecuteBuyOrder
if (qtyStatus == Market::QtyStatus::Complete) {
HandleCompleteOrder(iRef, oInfo, stationID, isPlayer, isCorp);
} else if (qtyStatus == Market::QtyStatus::Over) {
HandleOverOrder(iRef, oInfo, stationID, isPlayer, isCorp);
}This refactoring reduces the complexity of the main function by moving specific logic into helper functions, making it easier to follow and maintain.
There was a problem hiding this comment.
I'd love to see it refactored too, but it's out of scope for this PR. Again, good bot.
There was a problem hiding this comment.
That's completely understandable, Charles. Refactoring can sometimes be a larger task that might not fit within the scope of a specific PR, especially when the focus is on fixing bugs and implementing new features. It's great to hear that you're considering it for the future, though. If you decide to tackle it later, breaking down complex functions into smaller, more manageable pieces can definitely improve readability and maintainability. Let me know if you need any assistance or further suggestions when you get to that point!
|
@charles-m-knox Thanks for the excellent and detailed contribution and cleanup!! ❤️ I looked it over and it looks good to me, so here's your merge! As a side note, it seems like the bot is actually quite useful for things like that, I'm glad to see that it seems to have been worthwhile adding it 🙂 |
Summary:
This pull request implements more expected behaviors when doing market transactions. It also implements more support for the NPC traders mentioned throughout the codebase.
It also fixes #96 and #80:
Additionally, it renames
TranserFundstoTransferFunds, and offers a series of simple code formatting and legibility improvements.Also, please accept my apologies for the significant number of files changed in this PR. I usually try to keep it slim, but this one touched upon a few different areas in the codebase. You'll primarily want to review
MarketProxyService.cpp,MarketMgr.cpp, andMarketDB.cpp- almost everything else is unchanged aside from formatting/legibility.More context/details:
One of my goals with evemu has been to seed the market with reasonable buyer and seller activity on the market. During code exploration, I discovered reserved trader NPC character IDs including trader joe. I began to seed the market with bidders across different regions using SQL statements, but shortly discovered that market transactions were not behaving as expected. This led me down the rabbit hole that eventually turned into this pull request today.
So - in my experience, during normal usage, I've encountered a few issues regarding market transactions:
The evemu codebase has scattered mentions of "Trader Joe" as well as a range of character ID ranges from 4xxx-5xxxx that allow trader NPC's to essentially "create" market activity. This pull request fleshes out that logic and it seems stable so far on my system.
In order to get NPC traders to work properly within the wallet transactions view, any time a trader NPC's client ID is detected in
MarketDB::GetTransactions, it is swapped with the current player's ID (only when being sent to the game client) so that the game client's subsequent lookups to resolve information about the entity succeed. Without this step, the server fails to lookup an entity for the trader NPCs and throws an exception. We could discuss better options for this - for example, seeding the database with trader NPCs could possibly help. The actual trader NPC character ID is stored in the database as expected.As part of this implementation, I found that
characterID(ormemberID) for market transactions in the DB was intended to be reserved for corporation transactions. However, the reason for #80 is because each market transaction in the DB does not have an "owner" currently, it only has theclientID. I found that there was sufficient room to leverage simple ternary statements (many already existed actually) to determine if an entity is part of a corporation or not using theisCorpflags present in the DB - if no corp, then thecharacterIDfor the market transaction is set to the player's ID, as expected. This allows a proper market transaction history to be associated with an actual character, while still leaving room for corporation-specific logic to exist in the future.In the above screenshot, you can see the before/after behavior -
characterIDis0before, and after, it is now associated with an actual character.Testing
To seed the market, I ran the following SQL - please note that this will delete all market orders in the entire db, as well as resetting the market seed migrations. You'll notice that this is a slightly modified version of the standard market seeding SQL query that ships with the codebase.
This SQL code essentially seeds the market with sellers and buyers. Bidders place orders at a rate of 3% less than the sell to create a profit margin for the buyer, which somewhat emulates a real marketplace.
Expand here to see SQL that you can copy/paste
Test cases
Here are the test cases I did. Where appropriate, I used immediate orders as well as orders of a non-zero duration and fulfilled them on two accounts.
Corporation transactions are out of scope and are untested due to the fact that the code explicitly forbids a character from doing a corporation transaction at this time.
Other notes
While making these changes, I found that market results are still cached and do not invalidate upon submitting an order. I wasn't able to figure this one out without increasing the scope of this PR, unfortunately.
Summary by Sourcery
Improve market transaction handling by adding support for NPC traders, fixing transaction value and client display issues, and enhancing transaction logic to prevent excessive loops. Rename
TranserFundstoTransferFundsand improve code formatting for better readability.New Features:
Bug Fixes:
Enhancements:
TranserFundstoTransferFundsfor consistency and clarity across the codebase.Tests: