fix: preserve null gold in DonateGoldExecution so auto-donate works (BUG-03)#3944
fix: preserve null gold in DonateGoldExecution so auto-donate works (BUG-03)#3944berkelmali wants to merge 2 commits into
Conversation
…BUG-03) When goldNum is null, the constructor was coercing it to 0 via 'goldNum ?? 0', which prevented the init() fallback logic (this.gold ??= sender.gold() / 3n) from ever executing. Now null is preserved so the 1/3 gold donation correctly triggers.
Walkthrough
ChangesGold field nullability and tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/execution/DonateGoldExecution.ts`:
- Line 33: The change preserved null for this.gold in DonateGoldExecution to
enable the auto-donate fallback, but there is no test covering goldNum = null;
add a unit test that constructs DonateGoldExecution with goldNum = null and a
mock sender whose gold() returns a known BigInt, then assert that after
initialization (or after running the initializer method used in the class) the
actual donation amount equals sender.gold() / 3n; reference DonateGoldExecution,
the constructor behavior that sets this.gold, and the fallback calculation
sender.gold() / 3n when writing the test.
🪄 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: 081106a5-affb-422d-9709-81f91eff5982
📒 Files selected for processing (1)
src/core/execution/DonateGoldExecution.ts
| goldNum: number | null, | ||
| ) { | ||
| this.gold = toInt(goldNum ?? 0); | ||
| this.gold = goldNum === null ? null : toInt(goldNum); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for tests covering DonateGoldExecution with null gold amount
# Find test files that import or reference DonateGoldExecution
rg -l 'DonateGoldExecution' --glob '*test*' --glob '*spec*'
# Search for test cases that might cover null gold or auto-donate behavior
rg -n -C3 'DonateGold.*null|auto.*donate|goldNum.*null' --glob '*test*' --glob '*spec*'Repository: openfrontio/OpenFrontIO
Length of output: 119
🏁 Script executed:
# Check test files for DonateGoldExecution coverage
head -100 tests/Donate.test.ts
# Check the source file to understand the context
cat -n src/core/execution/DonateGoldExecution.ts | head -60Repository: openfrontio/OpenFrontIO
Length of output: 4882
🏁 Script executed:
# Find the line where DonateGoldExecution is instantiated in the test
rg -n 'new DonateGoldExecution' tests/Donate.test.ts -A 2
# Also check the AllianceDonation test file
rg -n 'new DonateGoldExecution' tests/AllianceDonation.test.ts -A 2Repository: openfrontio/OpenFrontIO
Length of output: 433
🏁 Script executed:
# Search all test files for any DonateGoldExecution calls with null or without goldNum param
rg 'DonateGoldExecution\(' tests/ -A 1 | grep -v '^\s*[0-9]' | head -50
# Count total DonateGoldExecution calls
rg 'new DonateGoldExecution' tests/ | wc -l
# Check if there's a test specifically for auto-donate or null goldNum
rg -i 'auto.*donate|goldNum.*null|gold.*null' tests/Repository: openfrontio/OpenFrontIO
Length of output: 605
Add test for auto-donate scenario (null goldNum).
The fix correctly preserves null at line 33 to enable the auto-donate fallback at line 49. However, no test cases cover the scenario where goldNum is null. Per coding guidelines, all changes to src/core/ must include tests. Please add a test case that verifies: when DonateGoldExecution is instantiated with goldNum = null, the donation amount defaults to sender.gold() / 3n during initialization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/execution/DonateGoldExecution.ts` at line 33, The change preserved
null for this.gold in DonateGoldExecution to enable the auto-donate fallback,
but there is no test covering goldNum = null; add a unit test that constructs
DonateGoldExecution with goldNum = null and a mock sender whose gold() returns a
known BigInt, then assert that after initialization (or after running the
initializer method used in the class) the actual donation amount equals
sender.gold() / 3n; reference DonateGoldExecution, the constructor behavior that
sets this.gold, and the fallback calculation sender.gold() / 3n when writing the
test.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Donate.test.ts`:
- Around line 248-250: Replace the weak positivity check with a precise
assertion against the expected auto-donate amount: compute expectedLoss =
donorGoldBefore / 3n (or a lower-bound adjusted if rounding is desired) and
replace expect(donorLost).toBeGreaterThan(0n) with
expect(donorLost).toBeGreaterThanOrEqual(expectedLoss); reference the existing
variables donorGoldBefore, donor.gold(), and donorLost when making the change so
the test verifies the ~1/3 deduction exactly.
🪄 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: fda7c460-da65-4180-950a-7d10ea34856d
📒 Files selected for processing (1)
tests/Donate.test.ts
| // Check donor lost roughly 1/3 of their gold | ||
| const donorLost = donorGoldBefore - donor.gold(); | ||
| expect(donorLost).toBeGreaterThan(0n); |
There was a problem hiding this comment.
Assert the expected auto-donate amount, not just > 0n.
This regression can still pass if donation is incorrect but positive. Please assert against donorGoldBefore / 3n (or at least a strong lower bound tied to it) so BUG-03 is truly locked.
Suggested tightening
- // Check donor lost roughly 1/3 of their gold
+ // Check null path donates one-third of sender gold
+ const expectedAutoDonate = donorGoldBefore / 3n;
const donorLost = donorGoldBefore - donor.gold();
+ const recipientGained = recipient.gold() - recipientGoldBefore;
- expect(donorLost).toBeGreaterThan(0n);
+ expect(donorLost).toBeGreaterThan(0n);
+ expect(recipientGained).toBeGreaterThanOrEqual(expectedAutoDonate);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check donor lost roughly 1/3 of their gold | |
| const donorLost = donorGoldBefore - donor.gold(); | |
| expect(donorLost).toBeGreaterThan(0n); | |
| // Check null path donates one-third of sender gold | |
| const expectedAutoDonate = donorGoldBefore / 3n; | |
| const donorLost = donorGoldBefore - donor.gold(); | |
| const recipientGained = recipient.gold() - recipientGoldBefore; | |
| expect(donorLost).toBeGreaterThan(0n); | |
| expect(recipientGained).toBeGreaterThanOrEqual(expectedAutoDonate); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Donate.test.ts` around lines 248 - 250, Replace the weak positivity
check with a precise assertion against the expected auto-donate amount: compute
expectedLoss = donorGoldBefore / 3n (or a lower-bound adjusted if rounding is
desired) and replace expect(donorLost).toBeGreaterThan(0n) with
expect(donorLost).toBeGreaterThanOrEqual(expectedLoss); reference the existing
variables donorGoldBefore, donor.gold(), and donorLost when making the change so
the test verifies the ~1/3 deduction exactly.
1 participant
Description:
When
goldNumisnull(sender wants to auto-donate 1/3 of their gold), the constructor coerces it to0viagoldNum ?? 0, producing0n(BigInt zero). Since0nis not nullish, the??=fallback ininit()(this.gold ??= sender.gold() / 3n) never triggers - resulting in a 0-gold donation instead of the intended 1/3.Fix: Preserve
nullthrough the constructor so the??=operator correctly triggers duringinit().Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires