-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat: add AllocationExtension.sol #654
Conversation
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.
I had to create another mocks folder with a custom mock for the extension, since Smock does not work well with abstract contracts.
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.
Couple small comments, other than that looks good!
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.
Let's follow the example of the AlloUnit.t.sol with the naming, so it is easier as well to filter with forge only the unit tests. So we should change the file name to AllocationExtensionUnit.t.sol
// all tokens allowed | ||
if (allowedTokens[address(0)]) return true; | ||
|
||
if (allowedTokens[_token]) return true; |
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.
nitpick but worth ?
if (allowedTokens[_token]) return true; | |
if ( | |
allowedTokens[address(0)] || // all tokens allowed | |
allowedTokens[_token] | |
) return true; |
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.
Splitting the if
in two instead of using ||
saves a bit of gas if I remember correctly.
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.
Because of gas saving and readability I will keep it as is.
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.
lgtm. added a nitpick but not important
No description provided.