Skip to content

Apply faction-specific tariffs and list received tariffs in the ledger - #67

Merged
Drefvelin merged 1 commit into
mainfrom
fix/faction-specific-tariffs
Sep 27, 2026
Merged

Drefvelin merged 1 commit into
mainfrom
fix/faction-specific-tariffs

Conversation

@Drefvelin

@Drefvelin Drefvelin commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Nations reported that faction-specific tariffs had no effect. For example, The Holy Order has a 15% base tariff and a 0% tariff for FIG, but FIG still paid 15%.

  • ProvinceManager.getIncome asks for TaxTarget.TARIFFS with the guild's faction id, but TaxHandler.getTaxRate returned the base rate for TARIFFS and ignored the id. Guild and vassal taxes already resolve overrides, but tariffs didn't.
  • Tariffs were only charged when the base rate was above 0, so an override on a 0% base (for example, 20% on one rival) collected nothing.
  • The tariff proposal preview applied a faction-specific rate to every foreign guild.
  • The ledger's Tariffs item listed the tariffs your guild pays, not the ones your faction receives.

Changes

  • TaxHandler.getTaxRate(TARIFFS, id) now returns the faction-specific tariff when one exists.
  • Income charges each guild the rate for its faction, without the base-rate gate. The unused hasTariffs() is removed; preview/law clamping still applies through getTaxRate.
  • A tariff bracket now also clamps specific tariffs (as guild/vassal brackets do), so losing the tariffs rule zeroes the overrides too.
  • previewTariffRateChange takes the target faction id. A specific change only affects that faction, and a base change skips factions with their own rate.
  • createLedgerTariffsItem adds up what every guild pays this faction, grouped by the payer's faction, and lists the top 5.

Testing

  • New TaxHandlerTariffTest (override below the base, override on a 0% base, bracket clamping).
  • mvn test passes.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Tariff previews now reflect the rate change for the specific faction, including its tariff override.
    • Tariff calculations now apply to eligible provinces even when no general tariff rate is set.
    • Tariff payment summaries now show the top five payer factions by total amount received.
    • Faction-specific tariff rates are clamped when tariffs are closed.

Income lookups asked for the base TARIFFS rate with the guild's faction id,
but the handler ignored the id for tariffs, so per-faction tariff overrides
were never charged. Tariffs were also gated on the base rate being above
zero, so an override on a 0% base did nothing.

- Resolve faction-specific tariffs in TaxHandler.getTaxRate
- Charge the per-guild rate without the base-rate gate
- Clamp specific tariffs when a tariff bracket applies
- Scope the tariff proposal preview to the targeted faction, and skip
  factions with their own rate when previewing a base change
- Ledger Tariffs item now lists factions paying us, not what we pay

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 6776fe3b-94bd-43bf-8e56-7c85534dadfe

📥 Commits

Reviewing files that changed from the base of the PR and between 78ed460 and 5c97493.

📒 Files selected for processing (7)
  • src/main/java/net/tfminecraft/simplefactions/government/proposal/Proposal.java
  • src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java
  • src/main/java/net/tfminecraft/simplefactions/managers/inventory/GuildCreator.java
  • src/main/java/net/tfminecraft/simplefactions/objects/handler/TaxHandler.java
  • src/main/java/net/tfminecraft/simplefactions/utils/EconomicImpact.java
  • src/main/java/net/tfminecraft/simplefactions/utils/LoreWriter.java
  • src/test/java/net/tfminecraft/simplefactions/objects/handler/TaxHandlerTariffTest.java

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Tariff rate handling now accounts for faction-specific overrides in rate lookup, bracket application, and targeted impact previews. Province income calculation and guild tariff ledger reporting also use updated tariff payment handling.

Changes

Tariff Handling

Layer / File(s) Summary
Tariff rate resolution and validation
src/main/java/net/tfminecraft/simplefactions/objects/handler/TaxHandler.java, src/test/java/net/tfminecraft/simplefactions/objects/handler/TaxHandlerTariffTest.java
getTaxRate uses a faction-specific tariff when one exists. Tariff brackets also apply to specific overrides. Tests cover overrides, fallback rates, and clamping.
Targeted tariff preview and income calculation
src/main/java/net/tfminecraft/simplefactions/utils/EconomicImpact.java, src/main/java/net/tfminecraft/simplefactions/managers/ProvinceManager.java, src/main/java/net/tfminecraft/simplefactions/government/proposal/Proposal.java, src/main/java/net/tfminecraft/simplefactions/utils/LoreWriter.java
Tariff impact calls pass a target ID to the preview. The preview filters guilds by target, or excludes guilds with specific rates during a base-rate change. Province income calculation no longer checks hasTariffs() and records breakdown entries only for positive saved tariffs.
Guild tariff ledger totals
src/main/java/net/tfminecraft/simplefactions/managers/inventory/GuildCreator.java
The ledger sums positive tariff payments by payer faction across guilds, sorts the totals, and displays up to five payers.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: ryanbarlow97

Merge Risk: ⚪ Minimal · up to 5c974

No identified tariff-rate, preview, income, or ledger issue remains to address before normal merge checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 5c974

Tariff changes now affect payments across faction boundaries. The review found a potential mismatch between tariff policy limits and rates used for collection, and a proposal-preview path that may understate a base-rate change. The available evidence does not establish that either path is exploitable by an ordinary player.

Retained concerns

  • Medium · security · inferred: Faction-specific tariffs are now collected, but their write path does not enforce the active tariff bracket. A later out-of-bracket write could therefore be charged until a bracket is reapplied; whether ordinary proposal input permits such a write remains unverified.
  • Medium · architecture · inferred: The preview interprets any non-null proposal ID as a specific faction. If a submitted base-tariff proposal retains an “all” ID, its displayed impact would exclude ordinary payer factions. The submitted base-proposal ID was not verified.
Security review details

Security Blast Radius

  • inferred — A faction’s effective tariff override can affect each income-producing foreign province traded in by guilds of the targeted faction, rather than a single guild or province. The same-realm check and exact faction-ID lookup constrain that scope.

Security Findings and Attack Paths

  • inferred — If an authorized proposal or another writer stores a specific tariff outside the active bracket, the newly effective lookup can carry that rate into foreign-guild charges. The inspected apply path does not clamp it; reachability through ordinary player input remains unverified.

Trust Boundaries and Controls

  • observed — The inspected proposal application writes through the owning faction’s TaxHandler. The displayed specific-tax option checks whether the player can propose and whether a proposal can be made for the target; those UI checks alone do not establish rate validation at application.

Resilience and Maintainability Implications

  • inferred — A failure between clearing and completing a saved breakdown recomputation could expose partial tariff entries to settlement until another successful recomputation. The incremental update pattern predates this change; the PR increases the amounts and recipients it may represent, but failure scheduling was not established.

Hardening Proposals

  • proposed — Enforce the current tariff bracket at the authoritative specific-rate write or read boundary, and verify submitted base proposals use the preview’s null target convention.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 7 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the two main changes: applying faction-specific tariffs and listing received tariffs in the ledger.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

A rabbit checks each tariff rate,
And passes IDs through previews straight.
Guild payments gather, sum, and sort,
The ledger lists the largest cohort.
Then hops away beneath the moon.

Comment @coderabbitai help to get the list of available commands.

@Drefvelin
Drefvelin merged commit d4d1bea into main Sep 27, 2026
2 checks passed
@Drefvelin
Drefvelin deleted the fix/faction-specific-tariffs branch September 27, 2026 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants