Skip to content
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

Merged
merged 7 commits into from
Mar 9, 2023
Merged

Conversation

Amxx
Copy link
Contributor

@Amxx Amxx commented Feb 23, 2023

Note: this can't pass checks until prettier supports user defined operators.

Closes #167.

@PaulRBerg
Copy link
Owner

Thanks, Hadrien! I will review this during the weekend.

Indeed, the link checks will pass only after prettier-plugin-solidity and @solidity-parser/parser add support for Solidity v0.8.19 and UDVT operators:

solidity-parser/parser#80

@fvictorio
Copy link

@PaulRBerg Let me know if that's urgent for you. I opened an issue in Prettier Solidity but didn't implement it yet.

@PaulRBerg
Copy link
Owner

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).

Copy link
Owner

@PaulRBerg PaulRBerg left a 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.

src/sd59x18/ValueType.sol Outdated Show resolved Hide resolved
src/sd59x18/Helpers.sol Outdated Show resolved Hide resolved
src/ud60x18/ValueType.sol Outdated Show resolved Hide resolved
test/sd59x18/helpers/Helpers.t.sol Show resolved Hide resolved
src/sd59x18/ValueType.sol Outdated Show resolved Hide resolved
src/ud60x18/Helpers.sol Outdated Show resolved Hide resolved
@PaulRBerg
Copy link
Owner

@Amxx I have just pushed a commit to address all points above myself.

I am just waiting for prettier-plugin-solidity to add support for Solidity v0.8.19, and then we can merge this.

@Amxx
Copy link
Contributor Author

Amxx commented Feb 28, 2023

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.
(ordering is transparent to me, I let you do how you prefer)

@PaulRBerg
Copy link
Owner

No worries! Thanks for the additional color.

@fvictorio
Copy link

@PaulRBerg
Copy link
Owner

Thank you so, so much, @fvictorio.

But now I just realized that the issue wasn't due to prettier-plugin-solidity, as I have recently moved this project to use forge fmt instead.

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? 🙏

Amxx and others added 6 commits March 9, 2023 00:55
docs: rewrite explanation for operators directive
refactor: order "not" function alphabetically
test: test reverts for operators "/" and "*"
Copy link
Owner

@PaulRBerg PaulRBerg left a 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!

@PaulRBerg PaulRBerg merged commit 8dc0873 into PaulRBerg:main Mar 9, 2023
@Amxx Amxx deleted the operators branch March 9, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Solidity v0.8.19 and implement operators for UDVTs
3 participants