Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/core/configuration/Config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ export interface Config {
unitInfo(type: UnitType): UnitInfo;
tradeShipShortRangeDebuff(): number;
tradeShipGold(dist: number, player: Player | PlayerView): Gold;
tradeShipSelfGoldMultiplier(): number;
tradeShipSpawnRate(
tradeShipSpawnRejections: number,
numTradeShips: number,
Expand Down
4 changes: 4 additions & 0 deletions src/core/configuration/DefaultConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,10 @@ export class DefaultConfig implements Config {
return BigInt(Math.floor(baseGold * this.goldMultiplierFor(player)));
}

tradeShipSelfGoldMultiplier(): number {
return 0.4;
}

// Probability of trade ship spawn = 1 / tradeShipSpawnRate
tradeShipSpawnRate(
tradeShipSpawnRejections: number,
Expand Down
19 changes: 15 additions & 4 deletions src/core/execution/PortExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,18 @@ export class PortExecution implements Execution {
const comp = this.mg.getWaterComponent(neighbor);
if (comp !== null) sourceComponents.add(comp);
}
const ports = this.mg
const owner = this.port!.owner();

// Other players' ports (existing logic)
const otherPorts = this.mg
.players()
.filter((p) => p !== this.port!.owner() && p.canTrade(this.port!.owner()))
.flatMap((p) => p.units(UnitType.Port))
.filter((p) => p !== owner && p.canTrade(owner))
.flatMap((p) => p.units(UnitType.Port));

// Own ports (excluding the source port itself)
const ownPorts = owner.units(UnitType.Port).filter((p) => p !== this.port!);

const ports = [...otherPorts, ...ownPorts]
.filter((p) => {
for (const comp of sourceComponents) {
if (this.mg.hasWaterComponent(p.tile(), comp)) return true;
Expand All @@ -123,8 +131,11 @@ export class PortExecution implements Execution {
const weightedPorts: Unit[] = [];

for (const [i, otherPort] of ports.entries()) {
const isSelfTrade = otherPort.owner() === owner;
const expanded = new Array(otherPort.level()).fill(otherPort);
// Self-trade ports get base weight only (no proximity/friendly bonuses)
weightedPorts.push(...expanded);
if (isSelfTrade) continue;
const tooClose =
this.mg.manhattanDist(this.port!.tile(), otherPort.tile()) <
this.mg.config().tradeShipShortRangeDebuff();
Expand All @@ -135,7 +146,7 @@ export class PortExecution implements Execution {
// to increase the chances of trading with it.
weightedPorts.push(...expanded);
}
if (!tooClose && this.port!.owner().isFriendly(otherPort.owner())) {
if (!tooClose && owner.isFriendly(otherPort.owner())) {
weightedPorts.push(...expanded);
}
}
Expand Down
38 changes: 34 additions & 4 deletions src/core/execution/TradeShipExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,17 @@ export class TradeShipExecution implements Execution {
private tilesTraveled = 0;
private motionPlanId = 1;
private motionPlanDst: TileRef | null = null;
private readonly isSelfTrade: boolean;

private static _staggerCounter = 0;

constructor(
private origOwner: Player,
private srcPort: Unit,
private _dstPort: Unit,
) {}
) {
this.isSelfTrade = srcPort.owner() === _dstPort.owner();
}

init(mg: Game, ticks: number): void {
this.mg = mg;
Expand Down Expand Up @@ -72,22 +75,30 @@ export class TradeShipExecution implements Execution {
}

// If a player captures another player's port while trading we should delete
// the ship.
if (dstPortOwner.id() === this.srcPort.owner().id()) {
// the ship — but not if it was intentionally a self-trade.
if (!this.isSelfTrade && dstPortOwner.id() === this.srcPort.owner().id()) {
this.tradeShip.delete(false);
this.active = false;
return;
}

if (
!this.wasCaptured &&
!this.isSelfTrade &&
(!this._dstPort.isActive() || !tradeShipOwner.canTrade(dstPortOwner))
) {
this.tradeShip.delete(false);
this.active = false;
return;
}

// For self-trade, still check if the destination port is active
if (this.isSelfTrade && !this._dstPort.isActive()) {
this.tradeShip.delete(false);
this.active = false;
return;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Comment on lines +77 to 91
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Captured self-trade ship cannot reroute when destination is also lost.

For non-self trades, the (!this.wasCaptured && (!dstActive || !canTrade)) guard lets a captured ship fall through to the rerouting block at lines 95–118, so the new owner's ship can pick another friendly port instead of being deleted. The self-trade branch has no !this.wasCaptured guard, so if the trade ship gets captured by player C and the destination port is also lost (deactivated, or taken by yet another player D), this block deletes the captured ship instead of letting the captured-ship logic find one of C's ports.

Mirror the non-self path so capture takes precedence over the self-trade ownership check.

🛡️ Suggested fix
-    const dstActive = this._dstPort.isActive();
-    const shouldCancel =
-      // Non-self-trade: cancel if destination port was captured back, or
-      // (not yet captured) if trade is no longer viable
-      (!this.isSelfTrade &&
-        (dstPortOwner.id() === this.srcPort.owner().id() ||
-          (!this.wasCaptured &&
-            (!dstActive || !tradeShipOwner.canTrade(dstPortOwner))))) ||
-      // Self-trade: cancel if destination port is no longer active or was captured
-      (this.isSelfTrade &&
-        (!dstActive || dstPortOwner !== this.srcPort.owner()));
-    if (shouldCancel) {
-      this.cancelTrade();
-      return;
-    }
+    const dstActive = this._dstPort.isActive();
+    const srcOwnerId = this.srcPort.owner().id();
+    let shouldCancel = false;
+    if (this.isSelfTrade) {
+      // Self-trade: cancel only if not yet captured and the round-trip
+      // is no longer possible (dst inactive or no longer owned by us).
+      shouldCancel =
+        !this.wasCaptured &&
+        (!dstActive || dstPortOwner.id() !== srcOwnerId);
+    } else if (this.wasCaptured) {
+      // Captured cross-player ship: only cancel if the source player
+      // already retook the destination.
+      shouldCancel = dstPortOwner.id() === srcOwnerId;
+    } else {
+      shouldCancel =
+        dstPortOwner.id() === srcOwnerId ||
+        !dstActive ||
+        !tradeShipOwner.canTrade(dstPortOwner);
+    }
+    if (shouldCancel) {
+      this.cancelTrade();
+      return;
+    }

This also flattens the condition into three clear cases, which is easier to read than the combined boolean expression.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/TradeShipExecution.ts` around lines 77 - 91, The current
shouldCancel boolean in TradeShipExecution lets a captured self-trade ship be
deleted when the destination port is lost; update the condition so capture takes
precedence as in the non-self branch: ensure the self-trade branch checks
!this.wasCaptured before cancelling (i.e., mirror the non-self clause that uses
!this.wasCaptured && (!dstActive || !tradeShipOwner.canTrade(...))). Refactor
the combined boolean into three explicit cases for clarity: 1) non-self trade
cancellation when dst was captured by src owner, 2) non-self cancellation when
NOT this.wasCaptured and (dst inactive or tradeShipOwner.canTrade(dstPortOwner)
is false), and 3) self-trade cancellation only when NOT this.wasCaptured and
(dst inactive or dstPortOwner !== this.srcPort.owner()); call this.cancelTrade()
and return when any case is true.


const curTile = this.tradeShip.tile();

if (
Expand Down Expand Up @@ -168,7 +179,7 @@ export class TradeShipExecution implements Execution {
private complete() {
this.active = false;
this.tradeShip!.delete(false);
const gold = this.mg
let gold = this.mg
.config()
.tradeShipGold(this.tilesTraveled, this.tradeShip!.owner());

Expand All @@ -188,6 +199,25 @@ export class TradeShipExecution implements Execution {
this.mg
.stats()
.boatCapturedTrade(this.tradeShip!.owner(), this.origOwner, gold);
} else if (this.isSelfTrade) {
// Self-trade: reduced gold, credited once
const multiplier = this.mg.config().tradeShipSelfGoldMultiplier();
gold = BigInt(Math.floor(Number(gold) * multiplier));
this.srcPort.owner().addGold(gold, this._dstPort.tile());
this.mg.displayMessage(
"events_display.received_gold_from_trade",
MessageType.RECEIVED_GOLD_FROM_TRADE,
this.srcPort.owner().id(),
gold,
{
gold: renderNumber(gold),
name: this.srcPort.owner().displayName(),
},
);
// Record stats
this.mg
.stats()
.boatArriveTrade(this.srcPort.owner(), this._dstPort.owner(), gold);
Comment on lines +197 to +215
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Self-trade message shows the player their own name.

The cross-player branch below shows the source the destination player's name and vice versa. Here the recipient is srcPort.owner() and the name placeholder is also srcPort.owner().displayName(), so the popup reads as if you traded with yourself by name. Functionally fine, but a small UX nit — consider using a self-trade-specific message key (e.g. events_display.received_gold_from_self_trade) or omitting the partner name so the wording reflects the new mechanic.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/TradeShipExecution.ts` around lines 202 - 220, The
self-trade branch in TradeShipExecution (inside the this.isSelfTrade block)
currently calls this.mg.displayMessage with the same message key and name
placeholder as cross-player trades, so the popup shows the player's own
displayName as the trade partner; change the display to a self-trade-specific
message by replacing the message key (e.g. use
"events_display.received_gold_from_self_trade") or remove the partner name
placeholder in the this.mg.displayMessage call for the this.isSelfTrade branch,
and update any corresponding stats/params so the payload no longer passes
srcPort.owner().displayName() as the partner name (adjust message params where
this.srcPort.owner().displayName() is passed).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, This should be fine.. Too many different chat labels might get confusing for the player

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zixer1, fair point — keeping the label count low is a reasonable UX decision. Acknowledged, no change needed here! 👍

(⳹´・ω・)ノ


🧠 Learnings used
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 3244
File: src/core/execution/nation/NationStructureBehavior.ts:621-625
Timestamp: 2026-02-19T08:49:23.183Z
Learning: In src/core/execution/nation/NationStructureBehavior.ts factoryValue() and TrainStation.ts, when determining trade relationship type for rail network connectivity scoring, the check order `player.isOnSameTeam(neighbor) ? "team" : player.isAlliedWith(neighbor) ? "ally" : "other"` is intentional. Team relationships take precedence over ally relationships, so players who are both on the same team AND allied receive the lower "team" trade gold, not "ally" gold. This reflects game design: team members are inherently cooperative (lower incentive needed), while allies from separate teams need higher incentives.

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.

Learnt from: andybellenie
Repo: openfrontio/OpenFrontIO PR: 3509
File: src/server/GameServer.ts:667-680
Timestamp: 2026-03-24T23:21:38.781Z
Learning: In openfrontio/OpenFrontIO, the clan-tag privacy feature in `src/server/GameServer.ts` (`gameInfoForClient()`, PR `#3509`) is intentionally a partial mitigation against tag sniping, not a complete fix. A larger clan system rework is in development. The bypass via rejoin with a different clanTag (identityUpdate path in Worker.ts) is a known, accepted limitation. Do not flag this as a critical issue in future reviews of this file.

Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 3292
File: src/core/execution/TrainExecution.ts:70-84
Timestamp: 2026-02-25T02:38:42.961Z
Learning: Preserve the intentional duplication of boundary tiles when constructing train motion plan paths. Specifically, do not deduplicate tiles at station boundaries in train motion plan logic. This affects both server-side tracking (TrainExecution.ts saveTraversedTiles) and client-side processing (GameView.ts train motion plan processing). Reviewers should ensure changes do not remove or consolidate boundary tiles, as duplicates are expected and documented in MotionPlans.ts, and are necessary for correct car spacing and positioning, especially when speed > 1.

Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 3549
File: tests/pathfinding/playground/server.ts:246-250
Timestamp: 2026-03-31T17:02:41.798Z
Learning: When migrating/implementing Express 5+ code, use the Express 5 `app.listen(PORT, (error) => { ... })` callback signature: Express 5 passes listen errors (e.g., `EADDRINUSE`) as the first argument to the callback. Handle the error inside that callback instead of relying on Node.js `http.Server`’s `'error'` event semantics.

Learnt from: baculinivan-web
Repo: openfrontio/OpenFrontIO PR: 3688
File: src/client/graphics/layers/GameRightSidebar.ts:235-248
Timestamp: 2026-04-22T08:54:50.345Z
Learning: In the OpenFrontIO/OpenFrontIO repository, do not raise review findings for standard web accessibility (a11y) keyboard/semantic issues that would normally require button semantics (e.g., using clickable `<div>` elements instead of `<button>`, or similar keyboard accessibility/ARIA patterns). This repository intentionally does not enforce those semantic/keyboard-accessibility conventions because gameplay requires touch or mouse input and is not operable via keyboard alone.

} else {
this.srcPort.owner().addGold(gold);
this._dstPort.owner().addGold(gold, this._dstPort.tile());
Expand Down
72 changes: 72 additions & 0 deletions tests/PortExecution.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,76 @@ describe("PortExecution", () => {

expect(ports.length).toBe(1);
});

test("Self-trade: own ports appear as destinations", () => {
game.config().proximityBonusPortsNb = () => 0;
game.config().tradeShipShortRangeDebuff = () => 0;
game.config().structureMinDist = () => 0;

player.conquer(game.ref(7, 5));
player.conquer(game.ref(7, 15));
const spawn1 = player.canBuild(UnitType.Port, game.ref(7, 5));
const spawn2 = player.canBuild(UnitType.Port, game.ref(7, 15));
if (spawn1 === false || spawn2 === false) {
throw new Error("Unable to build ports for test");
}
const port1 = player.buildUnit(UnitType.Port, spawn1, {});
player.buildUnit(UnitType.Port, spawn2, {});

const execution = new PortExecution(port1);
execution.init(game, 0);
execution.tick(0);

const ports = execution.tradingPorts();

// Should include the player's other port
expect(ports.length).toBeGreaterThanOrEqual(1);
expect(ports.some((p) => p.owner() === player)).toBe(true);
});

test("Self-trade: port does not trade with itself", () => {
game.config().proximityBonusPortsNb = () => 0;
game.config().tradeShipShortRangeDebuff = () => 0;

player.conquer(game.ref(7, 10));
const spawn = player.canBuild(UnitType.Port, game.ref(7, 10));
if (spawn === false) {
throw new Error("Unable to build port for test");
}
const port = player.buildUnit(UnitType.Port, spawn, {});

const execution = new PortExecution(port);
execution.init(game, 0);
execution.tick(0);

const ports = execution.tradingPorts();

// With only one own port and no other player ports, no destinations available
expect(ports.length).toBe(0);
});

test("Self-trade: own ports do not get proximity or friendly bonuses", () => {
game.config().proximityBonusPortsNb = () => 10;
game.config().tradeShipShortRangeDebuff = () => 0;
game.config().structureMinDist = () => 0;

player.conquer(game.ref(7, 5));
player.conquer(game.ref(7, 15));
const spawn1 = player.canBuild(UnitType.Port, game.ref(7, 5));
const spawn2 = player.canBuild(UnitType.Port, game.ref(7, 15));
if (spawn1 === false || spawn2 === false) {
throw new Error("Unable to build ports for test");
}
const port1 = player.buildUnit(UnitType.Port, spawn1, {});
player.buildUnit(UnitType.Port, spawn2, {});

const execution = new PortExecution(port1);
execution.init(game, 0);
execution.tick(0);

const ports = execution.tradingPorts();

// Own port at level 1 should appear once (base weight only, no bonuses)
expect(ports.length).toBe(1);
});
});
Loading