-
-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set operators for UD60x18 and SD59x18 #168
Conversation
Thanks, Hadrien! I will review this during the weekend. Indeed, the link checks will pass only after |
@PaulRBerg Let me know if that's urgent for you. I opened an issue in Prettier Solidity but didn't implement it yet. |
Thanks, @fvictorio. This is relatively urgent, indeed, as we're prepping up for an audit of a code base that uses PRBMath, and it would be great to have this PR merged in before the audit starts (we'd prefer to use UDVT operators). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just finished reviewing this. Overall it looks pretty good!
Besides the alphabetical ordering issues mentioned below, there's just one other thing that needs to be added:
Duplicate the revert test in div
and mul
so that both the function and the operator are covered.
@Amxx I have just pushed a commit to address all points above myself. I am just waiting for |
I did not realize that they were ordered alphabetically. Note that the order I used for operator is the one provided by solidity in its documentation. |
No worries! Thanks for the additional color. |
Thank you so, so much, @fvictorio. But now I just realized that the issue wasn't due to The issue is with Solhint :( This command fails: yarn solhint "{src,test}/**/*.sol" Any chance you could also bump the Solidity parser in Solhint to fix this? 🙏 |
docs: rewrite explanation for operators directive refactor: order "not" function alphabetically test: test reverts for operators "/" and "*"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI passed after upgrading to the latest version of Solhint. Merging now.
Thanks for your contribution, Hadrien!
Note: this can't pass checks until prettier supports user defined operators.
Closes #167.