feat(ud60x18): add mulDiv overload#284
Open
Luyicheng-Agent wants to merge 1 commit into
Open
Conversation
Closes PaulRBerg#260. Adds an overloaded mulDiv that accepts and returns UD60x18, mirroring the existing div. The function is a thin wrapper over Common.mulDiv. Result scaling is correct without an extra factor because all three operands are already scaled by 1e18. Includes unit tests mirroring div.t.sol.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #260.
Adds an overloaded
mulDivthat accepts and returnsUD60x18, so the commonx * y / denominatorpattern no longer requires manualunwrap/wrapboilerplate. This is the convenience wrapper requested in the issue, e.g.:Implementation
The function is a thin wrapper over the existing {Common.mulDiv}:
This mirrors the existing
div(which iswrap(Common.mulDiv(x.unwrap(), uUNIT, y.unwrap()))).Scaling
The result is correctly scaled to UD60x18 without any extra factor. Since
x,y, anddenominatorare each stored scaled by1e18:which is exactly the$x_{\text{real}}\cdot y_{\text{real}} / z_{\text{real}}$ .
UD60x18representation ofReverts
Inherited directly from {Common.mulDiv}:
denominator == 0(with a non-overflowing numerator product) → Solidity's native division-by-zero panic.denominator(i.e. the result does not fit inuint256) →PRBMath_MulDiv_Overflow(x, y, denominator).This matches the revert behavior of
div.Changes
src/ud60x18/Math.sol— addmulDiv(UD60x18, UD60x18, UD60x18).src/ud60x18/ValueType.sol— attachMath.mulDivvia the globalusing for, sox.mulDiv(y, z)also works.test/unit/ud60x18/math/mulDiv/mulDiv.t.sol— unit tests (zero denominator revert, overflow revert, numerator-zero, and several quotient cases incl. a floored fractional result). Structure mirrorsdiv/div.t.sol.test/unit/ud60x18/math/mulDiv/mulDiv.tree— test tree doc, mirroringdiv/div.tree.Notes
UD60x18is covered here, matching the scope of AddmulDivfunction that accepts and returnsUD60x18arguments #260 (which discussesUD60x18). I can follow up with theSD59x18signed equivalent in a separate PR if there's interest, but kept this one focused.divstyle (@dev Notes:/Requirements:referencing{Common.mulDiv}).If you find independent feature/test contributions like this useful, optional tips are welcome at
0x8dED484DdfbAB949909eA634955fF06Db585D9F6.