-
Notifications
You must be signed in to change notification settings - Fork 869
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
refactor: General Optimizations #80
Conversation
…-protocol into refactor/general-optimizations
…profile image URI setters to calldata.
…coveredAddress()`.
…ted with prettier.
…sible overflow cases.
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 went commit by commit, everything looks fine. Just one question and we are ready to merge
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.
Well now looks perfect to me, approving it!
The internal functions do not have natspec documentation |
contracts/core/LensHub.sol
Outdated
@@ -892,7 +894,7 @@ contract LensHub is ILensHub, LensNFTBase, VersionedInitializable, LensMultiStat | |||
_collectModuleWhitelisted, | |||
_referenceModuleWhitelisted | |||
); | |||
_profileById[vars.profileId].pubCount++; | |||
++_profileById[vars.profileId].pubCount; |
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.
Could this increment be done in line 891?
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.
No because this introduces a vuln that allows the comment to point to itself.
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.
PublishingLogic::createComment
performs external calls, so there might be a reentrancy that could overwrite the comment content and emit two different comments for the same publication id.
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.
Yeah will address this and then merge
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.
Addressed in 90d6c69!
…`LensPeriphery`, including the name used in the domain separator.
As for natspec in internals, I think keeping it in the externals and publics is fine, we don't want to overdo it with the comments imo-- especially because we cannot |
…d manually prevent self-referencing comments.
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 left a comment about renaming _shots
to something more clear. I'll approve anyways and let @Zer0dot find a better name or just merge this
misc: Fix namespace resolve functions
This PR contains general optimizations and is being actively worked on.