Skip to content

fix(staker): replace wrapping Mul with MulDiv in reward scaling#1233

Open
notJoon wants to merge 1 commit intomainfrom
fix/staker-reward-mul-overflow
Open

fix(staker): replace wrapping Mul with MulDiv in reward scaling#1233
notJoon wants to merge 1 commit intomainfrom
fix/staker-reward-mul-overflow

Conversation

@notJoon
Copy link
Copy Markdown
Member

@notJoon notJoon commented Apr 2, 2026

Description

  • Replace Mul(rewardAcc, liquidity) with MulDiv(rewardAcc, liquidity * rewardPerSecond, q128) to use a 512 bit intermediate instead of a 256-bit wrapping multiplication

Summary by CodeRabbit

  • Bug Fixes

    • Improved precision in reward scaling calculations by optimizing the arithmetic computation path to prevent intermediate overflow issues.
  • Tests

    • Added test coverage for reward scaler precision validation to ensure calculation accuracy across various scenarios.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 2, 2026

Walkthrough

Refactored reward scaling computation in RewardState.rewardPerWarmup to consolidate two arithmetic steps into a single MulDiv operation via new helper method scaleReward. This prevents intermediate wrapping from separate multiplications. Added test TestRewardScalerPrecision to validate the new approach and detect divergence from the previous computation path.

Changes

Cohort / File(s) Summary
Reward Scaling Refactoring
contract/r/gnoswap/staker/v1/reward_calculation_pool.gno
Extracted reward scaling logic into new helper method scaleReward that combines deposit.Liquidity() * rewardPerSecond and performs a single MulDiv operation, replacing the prior two-step multiplication approach to avoid intermediate wrapping.
Test Suite Updates
contract/r/gnoswap/staker/v1/reward_calculation_pool_test.gno
Updated imports and added TestRewardScalerPrecision test that validates the new combined scaler approach, verifies expected u256 outputs, and asserts divergence from the old two-step approach when wrapping occurs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: replacing a wrapping multiplication with MulDiv in reward scaling logic within the staker module.
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.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/staker-reward-mul-overflow

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.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Apr 2, 2026

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
contract/r/gnoswap/staker/v1/reward_calculation_pool_test.gno (1)

952-962: Exercise the production helper here.

Lines 952-962 re-derive the formula instead of calling RewardState.scaleReward or rewardPerWarmup, so this test can stay green even if the implementation under review regresses or starts reading the wrong liquidity source.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f80fe1ed-4ddc-481e-a051-788c596f025d

📥 Commits

Reviewing files that changed from the base of the PR and between bef2541 and a5523f9.

📒 Files selected for processing (2)
  • contract/r/gnoswap/staker/v1/reward_calculation_pool.gno
  • contract/r/gnoswap/staker/v1/reward_calculation_pool_test.gno

Comment thread contract/r/gnoswap/staker/v1/reward_calculation_pool_test.gno
@notJoon notJoon added the DO NOT MERGE do not merge this PR label Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DO NOT MERGE do not merge this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant