Implement allocation-based traffic economics and actor settlement#238
Implement allocation-based traffic economics and actor settlement#238wnj00524 wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements an economic settlement layer for the network simulation, introducing the TrafficEconomicSettlementService to calculate revenues, production costs, and taxes. It also updates actor decision-making logic and simulation metrics to reflect these financial outcomes. The review feedback highlights a critical bug in the tax calculation logic that leads to a cash leak in the system. Furthermore, several performance improvements are suggested to address redundant settlement calls, inefficient actor lookups, and excessive object allocations during metrics generation.
| var profit = saleRevenue - (totalTransportCost + totalProductionCost + totalTax); | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(sellerId)) | ||
| { | ||
| var s = Ledger(sellerId!); s.SalesRevenue += saleRevenue; s.TransportCost += totalTransportCost; s.ProductionCost += totalProductionCost; s.TaxesPaid += totalTax; s.Profit += profit; s.CashDelta += profit; | ||
| } | ||
| if (!string.IsNullOrWhiteSpace(buyerId)) | ||
| { | ||
| var b = Ledger(buyerId!); b.PurchaseCost += saleRevenue; b.TaxesPaid += totalTax; b.CashDelta -= (saleRevenue + totalTax); | ||
| } | ||
| if (!string.IsNullOrWhiteSpace(taxActorId)) | ||
| { | ||
| var g = Ledger(taxActorId!); g.TaxesReceived += totalTax; g.CashDelta += totalTax; |
There was a problem hiding this comment.
The economic settlement logic for taxes is inconsistent and results in a cash leak from the simulation. Currently, totalTax is deducted from the buyer's cash (b.CashDelta -= (saleRevenue + totalTax)) and also deducted from the seller's profit (profit = saleRevenue - (... + totalTax)), which is then added to the seller's cash. However, the government only receives totalTax once (g.CashDelta += totalTax). This means totalTax is being subtracted twice from the total system cash but only added once, causing money to disappear from the simulation. Depending on the intended model, either the seller should receive the tax from the buyer before paying it to the government, or only one party should be responsible for the tax payment.
| string? ResolveActor(string nodeId) | ||
| { | ||
| var byControl = actorsById.Values.FirstOrDefault(a => a.ControlledNodeIds.Contains(nodeId, Comparer))?.Id; | ||
| if (!string.IsNullOrWhiteSpace(byControl)) return byControl; | ||
| return nodesById.TryGetValue(nodeId, out var node) ? node.ControllingActor : null; | ||
| } |
There was a problem hiding this comment.
The ResolveActor method performs an NodeId to ActorId at the beginning of the Settle method to reduce this to an
| TotalSalesRevenue = allocs.Sum(x => x.SaleRevenue), | ||
| TotalTransportCost = allocs.Sum(x => x.TotalTransportCost), | ||
| TotalProductionCost = allocs.Sum(x => x.TotalProductionCost), | ||
| TotalTax = allocs.Sum(x => x.TotalTax), | ||
| TotalProfit = allocs.Sum(x => x.Profit) |
There was a problem hiding this comment.
The code performs five separate Sum operations over the allocs list. This is inefficient as it iterates over the list multiple times. These totals should be accumulated in a single pass or during the initial allocation processing loop to improve performance.
| var settlement = new TrafficEconomicSettlementService().Settle(appliedNetwork, appliedSnapshot.TrafficOutcomes, actorMap); | ||
| appliedSnapshot = new VisualAnalyticsSnapshot { Network = appliedSnapshot.Network, TrafficOutcomes = settlement.Outcomes.ToList(), ConsumerCosts = appliedSnapshot.ConsumerCosts, Period = appliedSnapshot.Period }; |
There was a problem hiding this comment.
There is a redundant call to TrafficEconomicSettlementService().Settle. The simulationEngine.Simulate method (called within BuildSnapshot on line 94) already performs economic settlement. However, since Simulate currently only returns the outcomes and not the actor ledgers, you are forced to call it again here. A better approach would be to refactor NetworkSimulationEngine.Simulate to return the full settlement result, avoiding the need to re-process all allocations and create duplicate objects.
| ActorSalesRevenueById = (ledgers ?? new Dictionary<string, SimulationActorEconomicLedger>(StringComparer.OrdinalIgnoreCase)).ToDictionary(k => k.Key, v => v.Value.SalesRevenue, StringComparer.OrdinalIgnoreCase), | ||
| ActorProductionCostById = (ledgers ?? new Dictionary<string, SimulationActorEconomicLedger>(StringComparer.OrdinalIgnoreCase)).ToDictionary(k => k.Key, v => v.Value.ProductionCost, StringComparer.OrdinalIgnoreCase), | ||
| ActorTransportCostById = (ledgers ?? new Dictionary<string, SimulationActorEconomicLedger>(StringComparer.OrdinalIgnoreCase)).ToDictionary(k => k.Key, v => v.Value.TransportCost, StringComparer.OrdinalIgnoreCase), | ||
| ActorTaxesPaidById = (ledgers ?? new Dictionary<string, SimulationActorEconomicLedger>(StringComparer.OrdinalIgnoreCase)).ToDictionary(k => k.Key, v => v.Value.TaxesPaid, StringComparer.OrdinalIgnoreCase), | ||
| ActorTaxesReceivedById = (ledgers ?? new Dictionary<string, SimulationActorEconomicLedger>(StringComparer.OrdinalIgnoreCase)).ToDictionary(k => k.Key, v => v.Value.TaxesReceived, StringComparer.OrdinalIgnoreCase), | ||
| ActorProfitById = (ledgers ?? new Dictionary<string, SimulationActorEconomicLedger>(StringComparer.OrdinalIgnoreCase)).ToDictionary(k => k.Key, v => v.Value.Profit, StringComparer.OrdinalIgnoreCase), |
There was a problem hiding this comment.
The BuildMetrics method creates six new dictionaries using ToDictionary on every call. Additionally, if ledgers is null, it instantiates a new empty dictionary six times. This is inefficient and creates unnecessary garbage collection pressure. Consider extracting a safeLedgers variable or using a more efficient way to populate these metrics.
1 participant
Motivation
RouteAllocationand by introducing explicit production costs and taxes.Description
DefaultUnitSalePrice,DefaultUnitProductionCost,SalesTaxRate, andRouteTaxRatetoTrafficTypeDefinition; addedProductionCostPerUnitandSalesTaxRatetoNodeTrafficProfile; added per-allocation economic fields toRouteAllocationand per-outcome totals toTrafficSimulationOutcome; extendedEconomicSummarywith sales/production/tax fields while keepingTotalRevenuepopulated for compatibility.TrafficEconomicSettlementServiceandSimulationActorEconomicLedgerto compute per-allocationSaleUnitPrice,SaleRevenue,TransportCostPerUnit,ProductionCostPerUnit,TotalTax, andProfit(usingallocation.SourceUnitCostPerUnitas inherited input cost), resolve seller/buyer/tax authority, and produce actor ledgers and cash deltas.NetworkSimulationEngineand applying ledgerCashDeltaupdates toSimulationActorState.CashinSimulationActorCoordinator, and exposed actor-level economic metrics inSimulationActorMetrics.BuyTraffic/SellTrafficactions inSimulationActorActionApplieras intent updates that modifyNodeTrafficProfile.Consumption/.Production(usingAbsoluteValueorDeltaValue) without immediate cash transfer, and updated firm utility (MaximiseProfit) andEconomicCalculatorto use settled seller-side economics and populate new summary fields.Testing
dotnet test tests/MedWNetworkSim.Tests/MedWNetworkSim.Tests.csproj, but thedotnetCLI is not available in this environment so automated tests could not be executed.Codex Task