-
Notifications
You must be signed in to change notification settings - Fork 5
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
chore: running forge update and fixing OZ related methods #98
chore: running forge update and fixing OZ related methods #98
Conversation
This PR aims to make the `GetTradeableOrder` library easily inherited by other repos. Also it removes the unused `tradedAmountToken0` from the library. Pending issues addressed in [draft] #98: - [ ] installed OpenZeppelin is 4.9.0, Math Rounding enum are different than 5.0.2 ([latest](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/05f218fb6617932e56bf5388c3b389c3028a7b73/contracts/utils/math/Math.sol#L13)) - [ ] OZ 5.0.2 deprecated `safeApprove` (which brings tests errors on the "when approve returns false" expectations)
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.
Thanks for the work!
abi.encodeCall(IERC20.approve, (spenderBadApproval, type(uint256).max)), | ||
abi.encodeWithSelector(IERC20.approve.selector, spenderBadApproval), |
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.
notice: making the approve
call generically revert when called with spenderBadApproval
(reverts for every amount, instead of for type(uint256).max
)
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.
abi.encodeCall(IERC20.approve, (spenderBadApproval, type(uint256).max)), | |
abi.encodeWithSelector(IERC20.approve.selector, spenderBadApproval), | |
// Notice: we intentionally don't match the approved amount. | |
abi.encodeWithSelector(IERC20.approve.selector, spenderBadApproval), |
abi.encodeCall(IERC20.approve, (spenderBadApproval, type(uint256).max)), | ||
abi.encodeWithSelector(IERC20.approve.selector, spenderBadApproval), |
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.
abi.encodeCall(IERC20.approve, (spenderBadApproval, type(uint256).max)), | |
abi.encodeWithSelector(IERC20.approve.selector, spenderBadApproval), | |
// Notice: we intentionally don't match the approved amount. | |
abi.encodeWithSelector(IERC20.approve.selector, spenderBadApproval), |
As a second step from #97 in order to improve contracts inheritability from an external repo, this PR aims to update OZ to the latest version (5.0.2), which brought incompatibilities with repos using this version (as Math rounding enum was modified, for example).
Proposed changes:
forge update
to fetch latest packages (OZ and UniV2)safeApprove
withforceApprove
(note: different behaviour)Math.Rounding.Up/Down
forCeil/Floor
Pending:
testDeploymentRevertsIfApprovalReverts
test is failing with a different expectation than written on the test, this must come from the different behaviour thatforceApprove
has oversafeApprove
, this test expectation should be fixed before merging