Skip to content

Make fee conversion rounding explicit (always round up)#238

Merged
jhelison merged 2 commits into
KiiChain:mainfrom
OneNov0209:patch-1
Jan 19, 2026
Merged

Make fee conversion rounding explicit (always round up)#238
jhelison merged 2 commits into
KiiChain:mainfrom
OneNov0209:patch-1

Conversation

@OneNov0209

@OneNov0209 OneNov0209 commented Jan 19, 2026

Copy link
Copy Markdown
Contributor

Description

This PR makes the ERC20 fee conversion rounding behavior explicit and conservative.

Previously, the code used:

amountEquivalentInt := amountEquivalent.RoundInt()

RoundInt() uses bankers rounding, which can round up or down depending on the value.
For fee calculation paths, this can theoretically allow very small underpayment in edge cases.
This PR changes the logic to always round up:

amountEquivalentInt := amountEquivalent.Ceil().TruncateInt()

This guarantees that:

  • Fee payment is never under-collected
  • Rounding behavior is explicit and deterministic
  • Behavior is conservative and safe for fee accounting

The economic difference is negligible (at most 1 unit of smallest denomination), but this removes ambiguity and makes the policy clear.

Related discussion: #227

@OneNov0209 OneNov0209 requested a review from jhelison as a code owner January 19, 2026 13:33
@coderabbitai

coderabbitai Bot commented Jan 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Walkthrough

The change updates convertERC20ForFees in x/feeabstraction/keeper/fee.go to compute amountEquivalentInt using Ceil().TruncateInt() instead of RoundInt(). This causes the derived integer fee amount to be rounded up rather than to the nearest integer. The conversion attempt flow and error handling remain unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Possibly related issues

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and specifically describes the main change: making fee conversion rounding explicit by always rounding up.
Description check ✅ Passed The description is directly related to the changeset, providing context about the rounding behavior change, the technical implementation, and the rationale for the update.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

❤️ Share

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

@codecov

codecov Bot commented Jan 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
x/feeabstraction/keeper/fee.go 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@jhelison

jhelison commented Jan 19, 2026

Copy link
Copy Markdown
Contributor

Hey @OneNov0209
Can you update the changelog?

@OneNov0209

OneNov0209 commented Jan 19, 2026

Copy link
Copy Markdown
Contributor Author

Hey @OneNov0209 Can you update the changelog?

Thanks for the review.

I’ve updated the implementation to make the rounding policy explicit (always round up) and added a changelog entry under UNRELEASED -> Fixed.

This keeps the behavior deterministic and avoids any possibility of fee underpayment while staying consistent and simple.

Let me know if you prefer a different rounding policy.

@jhelison jhelison mentioned this pull request Jan 19, 2026
Closed
@OneNov0209
changelog: add entry for fee rounding fix
8525447 
Clarified ERC20 fee conversion rounding behavior.
@OneNov0209

OneNov0209 commented Jan 19, 2026

Copy link
Copy Markdown
Contributor Author

Done, I've added the changelog entry and pushed it on top of this PR. Thanks!

@jhelison jhelison linked an issue Jan 19, 2026 that may be closed by this pull request
[BUG] FeeAbstraction: RoundInt() can cause fee underpayment / overpayment in ERC20 fee conversion #227
Closed
@jhelison jhelison merged commit bf816d4 into KiiChain:main Jan 19, 2026
8 of 9 checks passed
@OneNov0209

OneNov0209 commented Jan 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@jhelison

Thanks mate. I already create ticket on discord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jhelison jhelison jhelison approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] FeeAbstraction: RoundInt() can cause fee underpayment / overpayment in ERC20 fee conversion

2 participants

@OneNov0209 @jhelison