Fix: Remove redundant division in FEE_UI scaling logic#93
Fix: Remove redundant division in FEE_UI scaling logic#93Rav1Chauhan wants to merge 1 commit intoDjedAlliance:mainfrom
Conversation
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
djed-sdk/src/djed/tradeUtils.js (1)
16-19: Add a regression test for the 1% UI-fee path.Given this bug was subtle and high impact, please add a unit test that asserts Line 17 unscales
FEE_UI = 0.01correctly forSCALING_DECIMALSand thatcalculateTxFees(...).f_uireflects 1% (not 0.01%).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@djed-sdk/src/djed/tradeUtils.js` around lines 16 - 19, Add a regression unit test that verifies FEE_UI_UNSCALED is computed correctly from FEE_UI = 0.01 using decimalUnscaling with SCALING_DECIMALS and that calculateTxFees(...).f_ui returns 1% (0.01 as a fraction) not 0.0001; specifically, import FEE_UI, SCALING_DECIMALS, FEE_UI_UNSCALED and decimalUnscaling, assert decimalUnscaling(FEE_UI.toString(), SCALING_DECIMALS) equals FEE_UI_UNSCALED, then call calculateTxFees with a representative amount and assert the returned .f_ui equals the expected 0.01 (or equivalent fraction) to prevent the 0.01% regression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@djed-sdk/src/djed/tradeUtils.js`:
- Around line 16-19: Add a regression unit test that verifies FEE_UI_UNSCALED is
computed correctly from FEE_UI = 0.01 using decimalUnscaling with
SCALING_DECIMALS and that calculateTxFees(...).f_ui returns 1% (0.01 as a
fraction) not 0.0001; specifically, import FEE_UI, SCALING_DECIMALS,
FEE_UI_UNSCALED and decimalUnscaling, assert decimalUnscaling(FEE_UI.toString(),
SCALING_DECIMALS) equals FEE_UI_UNSCALED, then call calculateTxFees with a
representative amount and assert the returned .f_ui equals the expected 0.01 (or
equivalent fraction) to prevent the 0.01% regression.
Addressed Issues:
Fixes #66
Fix: Correct UI fee scaling logic
Problem
FEE_UI_UNSCALED divided FEE_UI by 100 before applying decimalUnscaling.
If FEE_UI already represents a decimal percentage (e.g., 0.01 for 1%), this resulted in a 100× smaller fee being sent to the contract.
Solution
Removed the redundant / 100 and passed FEE_UI directly to decimalUnscaling.
Result
The UI fee is now correctly scaled according to SCALING_DECIMALS.
Screenshots/Recordings:
Not applicable.
This change affects internal fee scaling logic in:
djed-sdk/src/djed/tradeUtils.js
No UI components were modified.
Additional Notes:
Problem
FEE_UI_UNSCALED was calculated as:
decimalUnscaling((FEE_UI / 100).toString(), SCALING_DECIMALS)
If FEE_UI already represents a decimal percentage (e.g., 0.01 for 1%), dividing by 100 again reduces the effective fee by 100×.
Example:
FEE_UI = 0.01 (intended 1%)
(FEE_UI / 100) = 0.0001
Resulting contract-side fee = 0.01% instead of 1%
This leads to incorrect fee scaling at the protocol level.
Removed the redundant / 100 and passed FEE_UI directly into decimalUnscaling:
export const FEE_UI_UNSCALED = decimalUnscaling(
FEE_UI.toString(),
SCALING_DECIMALS
);
Result
UI fee is now correctly scaled according to SCALING_DECIMALS
Prevents undercharging by 100×
Aligns SDK behavior with protocol expectations
No breaking API changes
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit