feat: asymmetric train gold rewards (factory owner +5k over city owner)#3425
feat: asymmetric train gold rewards (factory owner +5k over city owner)#3425zuperzonic1 wants to merge 5 commits intoopenfrontio:mainfrom
Conversation
|
|
WalkthroughThe Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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: 2
🧹 Nitpick comments (1)
tests/core/game/TrainStation.test.ts (1)
92-103: This case never reaches the new receiver branch.
unit.owner()andtrainExecution.owner()are still the same mock here, so the test only proves the sender-sidetrainGold(..., false)call. Add a case with different owners and different sender/receiver return values; otherwise the asymmetric payout path can regress without failing the suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/game/TrainStation.test.ts` around lines 92 - 103, Existing test only covers sender-side payout because unit.owner() and trainExecution.owner() return the same mock; add a separate test that exercises the receiver branch by making unit.owner() and trainExecution.owner() return different owner mocks (or different IDs), mock trainExecution.sender()/receiver() return values appropriately, and then call TrainStation.onTrainStop so trainGold spy (from game.config.trainGold) is invoked with the receiver flag true (e.g., expect(trainGoldSpy).toHaveBeenCalledWith(expect.any(String), 3, true)); reference TrainStation.onTrainStop, unit.owner(), trainExecution.owner(), trainExecution.tradeStopsVisited, and the trainGold spy to locate and implement the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/configuration/Config.ts`:
- Around line 135-139: Update the trainGold signature to require the isReceiver
boolean rather than optional: change trainGold(rel: "self" | "team" | "ally" |
"other", citiesVisited: number, isReceiver?: boolean) to trainGold(rel: "self" |
"team" | "ally" | "other", citiesVisited: number, isReceiver: boolean) in
Config.ts and mirror the change in DefaultConfig.ts (remove any = false
default). Ensure any implementations of the Config interface (e.g., the
DefaultConfig.trainGold method) accept the required isReceiver parameter and
update all callers (including tests like tests/core/game/TrainStation.test.ts
and tests/NationStructureBehavior.test.ts) to pass true/false explicitly so
sender/receiver payout logic remains correct.
In `@src/core/game/TrainStation.ts`:
- Around line 26-35: The stats call for external train payouts is attributing
the gold to the wrong player: change the call to mg.stats().trainExternalTrade
so it passes the station owner who actually receives receiverGold; locate the
block where senderGold is computed and awarded (senderGold, trainOwner,
trainSelfTrade) and the conditional that computes receiverGold via
mg.config().trainGold(..., true), then replace the current
mg.stats().trainExternalTrade(trainOwner, receiverGold) with
mg.stats().trainExternalTrade(stationOwner, receiverGold) so the stat reflects
the actual recipient (stationOwner).
---
Nitpick comments:
In `@tests/core/game/TrainStation.test.ts`:
- Around line 92-103: Existing test only covers sender-side payout because
unit.owner() and trainExecution.owner() return the same mock; add a separate
test that exercises the receiver branch by making unit.owner() and
trainExecution.owner() return different owner mocks (or different IDs), mock
trainExecution.sender()/receiver() return values appropriately, and then call
TrainStation.onTrainStop so trainGold spy (from game.config.trainGold) is
invoked with the receiver flag true (e.g.,
expect(trainGoldSpy).toHaveBeenCalledWith(expect.any(String), 3, true));
reference TrainStation.onTrainStop, unit.owner(), trainExecution.owner(),
trainExecution.tradeStopsVisited, and the trainGold spy to locate and implement
the new case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ab58cd2-6a96-48c1-b1c2-ff7062f408e4
📒 Files selected for processing (4)
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/game/TrainStation.tstests/core/game/TrainStation.test.ts
| trainGold( | ||
| rel: "self" | "team" | "ally" | "other", | ||
| citiesVisited: number, | ||
| isReceiver?: boolean, | ||
| ): Gold; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect no matches after making the third argument mandatory everywhere.
# Direct 2-arg call sites.
rg -nP -U --type ts '\.trainGold\(\s*[^,()]+\s*,\s*[^,()]+\s*\)'
# Object-literal / mock implementations that still take only 2 params.
rg -nP -U --type ts 'trainGold\s*:\s*\(\s*[^,()]+\s*,\s*[^,()]+\s*\)\s*=>'
# Method/interface declarations that still stop at 2 params.
rg -nP -U --type ts 'trainGold\s*\(\s*[^,()]+\s*,\s*[^,()]+\s*\)\s*[:{]'Repository: openfrontio/OpenFrontIO
Length of output: 1426
🏁 Script executed:
# Find and inspect the DefaultConfig implementation
fd -t f "DefaultConfig" --type tsRepository: openfrontio/OpenFrontIO
Length of output: 236
🏁 Script executed:
# Find DefaultConfig file
fd "DefaultConfig" -t fRepository: openfrontio/OpenFrontIO
Length of output: 106
🏁 Script executed:
# Read the DefaultConfig.ts file to find trainGold implementation
cat -n src/core/configuration/DefaultConfig.ts | head -200Repository: openfrontio/OpenFrontIO
Length of output: 6547
🏁 Script executed:
# Find trainGold implementation in DefaultConfig.ts
rg -n "trainGold" src/core/configuration/DefaultConfig.ts -A 10Repository: openfrontio/OpenFrontIO
Length of output: 476
🏁 Script executed:
# Read more of the trainGold implementation to see the full logic
rg -n "trainGold" src/core/configuration/DefaultConfig.ts -A 30Repository: openfrontio/OpenFrontIO
Length of output: 1145
Make isReceiver a required parameter.
The optional isReceiver: boolean = false allows old code to call trainGold() with two arguments and silently default to sender payout. This happens already in tests (tests/core/game/TrainStation.test.ts and tests/NationStructureBehavior.test.ts). The split logic matters: ally trades pay 35k (sender) or 30k (receiver); team/other trades pay 25k or 20k. Make the parameter required in both Config.ts and DefaultConfig.ts so all callers explicitly choose the payout side. This forces updates to compile and prevents silent correctness bugs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/configuration/Config.ts` around lines 135 - 139, Update the
trainGold signature to require the isReceiver boolean rather than optional:
change trainGold(rel: "self" | "team" | "ally" | "other", citiesVisited: number,
isReceiver?: boolean) to trainGold(rel: "self" | "team" | "ally" | "other",
citiesVisited: number, isReceiver: boolean) in Config.ts and mirror the change
in DefaultConfig.ts (remove any = false default). Ensure any implementations of
the Config interface (e.g., the DefaultConfig.trainGold method) accept the
required isReceiver parameter and update all callers (including tests like
tests/core/game/TrainStation.test.ts and tests/NationStructureBehavior.test.ts)
to pass true/false explicitly so sender/receiver payout logic remains correct.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/game/TrainStation.ts (1)
35-39:⚠️ Potential issue | 🟠 MajorCredit external trade stats to the receiver player, not the train owner.
At Line 38,
receiverGoldis awarded tostationOwner, but at Line 39 the stat is recorded fortrainOwner. This misattributes the payout metric for external trades.Proposed fix
- mg.stats().trainExternalTrade(trainOwner, receiverGold); + mg.stats().trainExternalTrade(stationOwner, receiverGold);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/TrainStation.ts` around lines 35 - 39, The external trade payout is being credited to the wrong player in the stats call: you calculate receiverGold and add it to stationOwner via stationOwner.addGold(receiverGold, station.tile()) but then call mg.stats().trainExternalTrade(trainOwner, receiverGold); change the stats call to credit the station owner (use the same receiver entity used to receive gold) by passing the stationOwner (or its player id if that API expects an id) instead of trainOwner so the metric reflects the actual recipient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/game/TrainStation.ts`:
- Around line 35-39: The external trade payout is being credited to the wrong
player in the stats call: you calculate receiverGold and add it to stationOwner
via stationOwner.addGold(receiverGold, station.tile()) but then call
mg.stats().trainExternalTrade(trainOwner, receiverGold); change the stats call
to credit the station owner (use the same receiver entity used to receive gold)
by passing the stationOwner (or its player id if that API expects an id) instead
of trainOwner so the metric reflects the actual recipient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5e8861c-a14c-46e0-8f69-2854b36f3da4
📒 Files selected for processing (1)
src/core/game/TrainStation.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/execution/nation/NationStructureBehavior.ts (1)
700-738: Split connectivity weighting by role (sender vs receiver).Line 701, Line 712, and Line 738 always pass
false, so city connectivity scoring also uses sender-side gold values. With asymmetric rewards, this can overvalue city-side trade and dilute the factory incentive.♻️ Refactor sketch
- const reachableStations = useConnectionScore - ? this.getOrBuildReachableStations() - : []; + const reachableStations = useConnectionScore + ? this.getOrBuildReachableStations(false) // factory = sender + : []; ... - const reachableStations = useConnectionScore - ? this.getOrBuildReachableStations() - : []; + const reachableStations = useConnectionScore + ? this.getOrBuildReachableStations(true) // city = receiver + : []; - private getOrBuildReachableStations(): Array<...> { - this.reachableStationsCache ??= this.buildReachableStations(); - return this.reachableStationsCache; + private getOrBuildReachableStations(isReceiver: boolean): Array<...> { + // keep separate cache per role + ... } - private buildReachableStations(): Array<...> { + private buildReachableStations(isReceiver: boolean): Array<...> { - Number(game.config().trainGold("ally", 0, false)) + Number(game.config().trainGold("ally", 0, isReceiver)) ... - Number(game.config().trainGold(relType, 0, false)) / maxTradeGold + Number(game.config().trainGold(relType, 0, isReceiver)) / maxTradeGold }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/nation/NationStructureBehavior.ts` around lines 700 - 738, The connectivity weighting currently always uses sender-side values because every call to game.config().trainGold(..., 0, false) passes false; fix this by computing maxTradeGold from both sender and receiver values (take Math.max of trainGold(..., false) and trainGold(..., true) and 1) and by using sender-side trainGold(..., 0, false) for selfWeight while using receiver-side trainGold(relType, 0, true) when computing neighbor weight; update references in the maxTradeGold computation and the neighbor weight assignment (symbols: game.config().trainGold, maxTradeGold, selfWeight, weight) so receiver incentives (e.g. factories) are not diluted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/execution/nation/NationStructureBehavior.ts`:
- Around line 700-738: The connectivity weighting currently always uses
sender-side values because every call to game.config().trainGold(..., 0, false)
passes false; fix this by computing maxTradeGold from both sender and receiver
values (take Math.max of trainGold(..., false) and trainGold(..., true) and 1)
and by using sender-side trainGold(..., 0, false) for selfWeight while using
receiver-side trainGold(relType, 0, true) when computing neighbor weight; update
references in the maxTradeGold computation and the neighbor weight assignment
(symbols: game.config().trainGold, maxTradeGold, selfWeight, weight) so receiver
incentives (e.g. factories) are not diluted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed050249-5816-4507-8c96-8d8ca4378eee
📒 Files selected for processing (5)
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/nation/NationStructureBehavior.tssrc/core/game/TrainStation.tstests/core/game/TrainStation.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/core/game/TrainStation.test.ts
- src/core/configuration/DefaultConfig.ts
Apply open PRs from openfrontio/OpenFrontIO: - openfrontio#3425: Asymmetric train gold rewards (factory owner +5k bonus) - openfrontio#3397: Game speed + pause keybinds (P, <, >) - openfrontio#3383: Puerto Rico + Panama maps with nations and flags - openfrontio#3430: UI quality of life (moveable HUD, diplomacy panel, rate limiting) Add upstream-pr patch category with source tracking, merge status, and lifecycle management. All existing custom patches verified intact.
2 participants
Description:
Currently, train trades pay the same gold to both the factory owner (sender) and the city/port owner (receiver). This PR makes the payout asymmetric: the factory owner earns 5,000 more gold per trade than the city owner, incentivizing players to build and manage factories while still rewarding players who host trade stations.
New gold values per trade (at 0–5 stops, before distance penalty):
Distance penalty: −5,000 per stop beyond 5, floored at 5,000.
Changes:
Config.ts— Added requiredisReceiver: booleanparameter totrainGold()interface (required, not optional, to force all callers to explicitly choose a payout side)DefaultConfig.ts— Implemented asymmetric payout logic intrainGold()using theisReceiverflagTrainStation.ts— UpdatedTradeStationStopHandler.onStop()to calltrainGoldseparately for sender and receiver; fixedtrainExternalTradestat to be credited tostationOwner(the actual gold recipient) instead oftrainOwnerNationStructureBehavior.ts— Updated 3 weight-scoring calls to pass explicitfalse(sender perspective for normalization)TrainStation.test.ts— Updated alltrainGoldcalls to pass explicitisReceiverargumentChecklist: