Fix nation relation exploit 🔧 #2523
Conversation
WalkthroughDonateGoldExecution and DonateTroopExecution now store the Game reference, seed a PseudoRandom with game ticks, and use Difficulty + randomness to compute donation thresholds; relation changes are applied only when donated gold/troops meet the computed threshold. Changes
Sequence Diagram(s)sequenceDiagram
participant Donor
participant Exec as DonateExecution
participant Game as Game(mg)
participant Rand as PseudoRandom
participant Recipient
Donor->>Exec: init(mg, request)
Exec->>Game: read ticks, difficulty
Game-->>Exec: ticks, difficulty
Exec->>Rand: seed(ticks)
Exec->>Rand: compute threshold (difficulty, recipient)
Rand-->>Exec: threshold
Exec->>Recipient: attemptDonation(amount)
Recipient-->>Exec: donationResult(success/failure)
alt success and amount >= threshold
Exec->>Recipient: updateRelation(+computed)
Recipient-->>Exec: relationUpdated
else success but amount < threshold
Exec-->>Recipient: no relation change
end
Exec-->>Donor: finish(status)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (2)
src/core/execution/DonateTroopExecution.ts (1)
54-86: Consider making this method private.
getMinTroopsForRelationUpdate()is an internal calculation used only withintick(). Marking itprivatekeeps the public API surface clean.- getMinTroopsForRelationUpdate(): number { + private getMinTroopsForRelationUpdate(): number {src/core/execution/DonateGoldExecution.ts (1)
62-72: Consider making this method private.
getMinGoldForRelationUpdate()is only used internally withintick(). Marking itprivatekeeps the API surface clean and consistent.- getMinGoldForRelationUpdate(): number { + private getMinGoldForRelationUpdate(): number {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/execution/DonateGoldExecution.ts(4 hunks)src/core/execution/DonateTroopExecution.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/execution/DonateGoldExecution.tssrc/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/execution/DonateGoldExecution.tssrc/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/DonateGoldExecution.tssrc/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/DonateTroopExecution.ts
🧬 Code graph analysis (2)
src/core/execution/DonateGoldExecution.ts (2)
src/core/game/Game.ts (3)
Execution(350-355)Player(525-661)Game(663-747)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)
src/core/execution/DonateTroopExecution.ts (2)
src/core/game/Game.ts (3)
Execution(350-355)Player(525-661)Game(663-747)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)
🔇 Additional comments (5)
src/core/execution/DonateTroopExecution.ts (2)
1-7: LGTM on imports and new fields.Clean additions for
Difficulty,PseudoRandom, and the instance fields needed for the randomized threshold logic.
17-27: LGTM on initialization.Seeding
PseudoRandomwithmg.ticks()ensures deterministic behavior across all clients for the same game state. Warning message correctly referencesDonateTroopExecution.src/core/execution/DonateGoldExecution.ts (3)
1-15: LGTM on imports and new fields.Clean imports and field additions. Pattern matches
DonateTroopExecutionnicely.
28-42: LGTM on initialization.Deterministic seeding with
mg.ticks()and correct warning message referencingDonateGoldExecution.
50-53: Relation update logic looks good.Unlike the troop version, gold thresholds are fixed values, so no timing issue here. The comparison correctly converts the threshold to
BigIntfor comparison withGold.
…xploit' into flopinguin-fix-nation-relation-exploit
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/execution/DonateTroopExecution.ts (1)
57-89: Fallbackreturn 0re-enables the exploit for unknown difficulty values.If the difficulty somehow doesn't match any known value, returning 0 means any donation would trigger a relation update, which defeats the exploit fix. Consider throwing an error or defaulting to the strictest threshold (Impossible).
Apply this diff to use a safer fallback:
// ~14.3k - ~20k troops (for 100k troops) if (difficulty === Difficulty.Impossible) return this.random.nextInt( this.sender.troops() / 7, this.sender.troops() / 5, ); - return 0; + // Fallback to strictest threshold for any unexpected difficulty + return this.random.nextInt( + this.sender.troops() / 7, + this.sender.troops() / 5, + ); }Alternatively, use a switch statement with exhaustive handling to catch unhandled enum values at compile time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/execution/DonateTroopExecution.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/DonateTroopExecution.ts
🧬 Code graph analysis (1)
src/core/execution/DonateTroopExecution.ts (2)
src/core/game/Game.ts (3)
Execution(350-355)Player(525-661)Game(663-747)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)
🔇 Additional comments (4)
src/core/execution/DonateTroopExecution.ts (4)
1-7: LGTM!Clean imports with no unnecessary dependencies. The new private members for
randomandmgare appropriate for the threshold calculation logic.
17-34: LGTM!Using
mg.ticks()as the random seed keeps behavior deterministic for replay and multiplayer sync.
36-55: LGTM!The threshold is now computed before
donateTroops()runs, so the minimum is based on the sender's pre-donation troop count. This correctly addresses the previous review feedback.
91-98: LGTM!Standard execution lifecycle methods, no issues.
| tick(ticks: number): void { | ||
| if (this.troops === null) throw new Error("not initialized"); | ||
|
|
||
| const minTroops = this.getMinTroopsForRelationUpdate(); |
There was a problem hiding this comment.
I think this variable can be inlined
There was a problem hiding this comment.
I had it inlined, but then coderabbit rightfully said that we need to calculate the troops BEFORE donating them.
| this.active = false; | ||
| } | ||
|
|
||
| getMinTroopsForRelationUpdate(): number { |
There was a problem hiding this comment.
Can you use a switch case for this
| // ~7.7k - ~9.1k troops (for 100k troops) | ||
| if (difficulty === Difficulty.Easy) | ||
| return this.random.nextInt( | ||
| this.sender.troops() / 13, |
There was a problem hiding this comment.
What do you think about making it a function of the recipient max troops? This way you need to give more troops to be in favor of larger nations.
There was a problem hiding this comment.
Good idea, I added that
|
|
||
| getMinGoldForRelationUpdate(): number { | ||
| const { difficulty } = this.mg.config().gameConfig(); | ||
| if (difficulty === Difficulty.Easy) return this.random.nextInt(0, 25_000); |
There was a problem hiding this comment.
this can be a switch statement.
| this.active = false; | ||
| } | ||
|
|
||
| getMinGoldForRelationUpdate(): number { |
There was a problem hiding this comment.
instead of a binary update or not, what if we updated relations proportionally to the gold sent?
There was a problem hiding this comment.
Good idea, I added that
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/core/execution/DonateGoldExecution.ts (2)
50-86: GuardupdateRelationon positive update and check BigInt→Number castHere
calculateRelationUpdatecan return0whengoldSent < chunkSize, but you still callupdateRelationwith0. It is harmless but noisy and hides the intent.You can make the intent clearer and avoid the no-op call:
- const relationUpdate = this.calculateRelationUpdate(Number(this.gold)); - this.recipient.updateRelation(this.sender, relationUpdate); + const relationUpdate = this.calculateRelationUpdate(Number(this.gold)); + if (relationUpdate > 0) { + this.recipient.updateRelation(this.sender, relationUpdate); + }Also,
goldis effectively abigint(seethis.sender.gold() / 3n). Casting toNumberwill lose precision if gold can grow beyondNumber.MAX_SAFE_INTEGER. If that can happen in long games, consider keeping the math inbigintand only converting the final capped result (≤100) tonumber.
28-32: Random seed and chunk ranges – confirm they match the exploit-fix intentSeeding
PseudoRandomwithmg.ticks()means everyDonateGoldExecutioncreated in the same tick will get the same first random value, so all of them will use the same chunk size per difficulty. That is deterministic, but donations in the same tick are perfectly correlated. If you want more per-donation variety, you may want to mix in something like sender/recipient identity into the seed (while still keeping it deterministic).On Easy,
getGoldChunkSize()can be as low as1, so in rare cases a 1‑gold donation will still give 5 relation points. Given the exploit you are fixing (very small donations buying a Friendly relation), please double‑check that this “tiny but low‑odds” reward is intended and that you do not want a higher lower bound here (for example a few thousand gold).Also applies to: 61-75
src/core/execution/DonateTroopExecution.ts (1)
36-55: Threshold logic for troop donations looks good; consider handling unknown Difficulty explicitlyComputing
minTroopsonce beforedonateTroopsand basing it onrecipientMaxTroopsfixes the earlier “post‑donation threshold” issue and matches the idea that larger nations need larger donations. The exploit is much harder now, especially on higher difficulties.One small thing:
getMinTroopsForRelationUpdate()returns0in thedefaultbranch. If a newDifficultyvalue is ever added and not covered in this switch, any positive donation would again give +50 relation. You might want to:
- Treat unknown difficulties as one of the existing ones (for example Medium or Hard), or
- Log a warning and pick a conservative range, or
- Use a helper like
assertNever(difficulty)so the compiler forces you to handle new enum values.This keeps the exploit fix safe against future Difficulty additions.
Also applies to: 57-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/execution/DonateGoldExecution.ts(4 hunks)src/core/execution/DonateTroopExecution.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/execution/DonateGoldExecution.tssrc/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/execution/DonateGoldExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/DonateGoldExecution.tssrc/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/DonateTroopExecution.ts
🧬 Code graph analysis (2)
src/core/execution/DonateGoldExecution.ts (2)
src/core/game/Game.ts (3)
Execution(350-355)Player(525-661)Game(663-747)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)
src/core/execution/DonateTroopExecution.ts (2)
src/core/game/Game.ts (3)
Execution(350-355)Player(525-661)Game(663-747)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)
🔇 Additional comments (1)
src/core/execution/DonateTroopExecution.ts (1)
21-24: Fix typo in warning prefix (DonateTroopExecution→DonateTroopsExecution)Small nit: the log string uses
DonateTroopExecutionwhile the class name isDonateTroopsExecution. This can make log filtering and grepping harder.- console.warn( - `DonateTroopExecution recipient ${this.recipientID} not found`, - ); + console.warn( + `DonateTroopsExecution recipient ${this.recipientID} not found`, + );Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/core/execution/DonateGoldExecution.ts (3)
29-35: Consider using theticksargument to seedPseudoRandom
new PseudoRandom(mg.ticks())works, but seeding from the explicitticksparameter would better follow theExecution.init(mg, ticks)contract and make the time dependency clearer (and a bit easier to test). The updatedconsole.warnmessage is correct and more precise.
63-77: Difficulty-based chunk sizing is clear; consider caching per executionThe difficulty ranges and use of
PseudoRandom.nextIntavoid a zero chunk size and look good. If this method ever gets called more than once per donation, consider drawing once and storing agoldChunkSize: Goldfield so the threshold for a single donation cannot change mid-flow.
79-89: Keep chunk arithmetic inbigintand cast only at the end
Goldis abigint, so convertinggoldSent / chunkSizetonumberis safe here (because you cap at 100), but you can keep all math inbigintand only cast the small final value, which is more idiomatic and future‑proof:- calculateRelationUpdate(goldSent: Gold): number { - const chunkSize = this.getGoldChunkSize(); - // Calculate how many complete chunks were donated - // BigInt division automatically truncates (integer division) - const chunks = Number(goldSent / chunkSize); - // Each chunk gives 5 relation points - const relationUpdate = chunks * 5; - // Cap at 100 relation points - if (relationUpdate > 100) return 100; - return relationUpdate; - } + calculateRelationUpdate(goldSent: Gold): number { + const chunkSize = this.getGoldChunkSize(); + const chunks = goldSent / chunkSize; // bigint + + if (chunks <= 0n) return 0; + if (chunks >= 20n) return 100; // 20 * 5 = 100 (cap) + + return Number(chunks * 5n); + }This keeps the core logic in one numeric domain and only uses
numberfor the small, bounded relation result.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/execution/DonateGoldExecution.ts(4 hunks)src/core/execution/DonateTroopExecution.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/execution/DonateTroopExecution.tssrc/core/execution/DonateGoldExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/DonateTroopExecution.tssrc/core/execution/DonateGoldExecution.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/DonateTroopExecution.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/execution/DonateTroopExecution.tssrc/core/execution/DonateGoldExecution.ts
🧬 Code graph analysis (2)
src/core/execution/DonateTroopExecution.ts (2)
src/core/game/Game.ts (2)
Execution(350-355)Game(663-747)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)
src/core/execution/DonateGoldExecution.ts (1)
src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)
🔇 Additional comments (7)
src/core/execution/DonateGoldExecution.ts (2)
1-15: Imports and new fields are wired correctlyBringing in
Difficulty,Game,Gold, andPseudoRandom, and addingrandom/mgfields, matches the pattern inDonateTroopExecutionand keeps the execution self-contained; no issues here.
50-54: Relation scaling with donated gold closes the exploitUsing
calculateRelationUpdate(this.gold)and skipping updates when the result is0cleanly removes the “1 gold ⇒ big relation jump” exploit while still keeping the logic simple and easy to reason about.src/core/execution/DonateTroopExecution.ts (5)
1-2: LGTM! Clean imports for the new threshold logic.The additions of
DifficultyandPseudoRandomsupport the exploit fix by enabling difficulty-based, randomized donation thresholds.
6-7: Verify that identical seeds for same-tick donations are intended.The
PseudoRandominstance is seeded withmg.ticks(), which means all troop donations executed at the same tick will compute identical minimum thresholds. This creates deterministic behavior for replays but reduces per-player randomness.If unique thresholds per execution are desired, consider including sender/recipient IDs in the seed:
- this.random = new PseudoRandom(mg.ticks()); + this.random = new PseudoRandom( + mg.ticks() + this.sender.id().hashCode() + this.recipientID.hashCode() + );Otherwise, document that same-tick donations share thresholds by design.
Also applies to: 18-19
22-24: LGTM! Improved warning message specificity.Adding "DonateTroopExecution" to the warning helps debugging by clearly identifying the execution type.
39-39: Excellent fix for the relation exploit!Computing
minTroopsbefore the donation (line 39) and conditionally updating the relation (lines 46-48) correctly prevents the exploit where tiny percentage-based donations could immediately grant Friendly status. The pre-donation calculation ensures the threshold reflects the donor's actual capacity.Also applies to: 45-48
57-89: LGTM! Clean difficulty-based threshold calculation.The switch case cleanly implements escalating minimum thresholds tied to recipient capacity. The ranges are well-calibrated and comments accurately document the thresholds for a 100k max troop recipient. Using
recipientMaxTroopsas the base (per previous feedback) ensures larger nations require proportionally larger donations.The default fallback handles unexpected difficulty values gracefully.
|
|
||
| getGoldChunkSize(): Gold { | ||
| const { difficulty } = this.mg.config().gameConfig(); | ||
| switch (difficulty) { |
There was a problem hiding this comment.
Would it be better to put this into defaultConfig?
You could also use the unused difficultyModifier function if you want, but it's not needed.
Some goes for the troop switch
There was a problem hiding this comment.
Uh I don't know... Putting something in defaultConfig is probably most helpful if its used in multiple places?
If we want all constants in defaultConfig then a lot is missing there
| const { difficulty } = this.mg.config().gameConfig(); | ||
| switch (difficulty) { | ||
| case Difficulty.Easy: | ||
| return BigInt(this.random.nextInt(1, 2_500)); |
There was a problem hiding this comment.
Could we scale these values with tick number, or with number of factories/ports on the map?
The more economy buildings there are, the less valuable donations become, and in the endgame,
There was a problem hiding this comment.
Thats an interesting idea, but then players would have to do calculations if they want to "buy" a good relation with a nation.
I'm not sure if thats good or bad
There was a problem hiding this comment.
My thought was that the more it hurts you/the more it helps the nation, the more it raises the relation, so players wouldn't necessarily have to do calculations, they'd just give as much as they are comfortable with.
But this was just an idea to be clear, nothing necessary.
3 participants
Description:
Saw this in an Enzo video about beating impossible nations:
You can just donate 1% gold / 1% troops to a nation to get a friendly relation with them.
This PR adds randomized minimum donation requirements based on the difficulty.
Randomized in a way that there is a minimum someone has to donate to surely get the relation improvement, but you can also gamble and send less.
For troop donations, the minimum is calculated based on what percentage of their troops the sender donated.
For gold donations, it's fixed values. We cannot use percentages here because “having nearly no gold” is a usual case. Donating 100% of your gold wouldn’t hurt if you just spent all your gold on buildings.
I tried to add tests for this but it's really horrible. Because the test would have to wait until the relation update from the alliance accepting is gone (we need to have an alliance to send stuff), has returned to Neutral, and then changes back to Friendly after the donation.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
FloPinguin