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

refactor: General Optimizations #80

Merged
merged 31 commits into from
Apr 1, 2022
Merged

Conversation

Zer0dot
Copy link
Contributor

@Zer0dot Zer0dot commented Mar 23, 2022

This PR contains general optimizations and is being actively worked on.

@Zer0dot Zer0dot marked this pull request as ready for review March 25, 2022 20:48
Copy link
Member

@donosonaumczuk donosonaumczuk left a 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

contracts/core/LensHub.sol Outdated Show resolved Hide resolved
donosonaumczuk
donosonaumczuk previously approved these changes Mar 28, 2022
Copy link
Member

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

contracts/core/FollowNFT.sol Outdated Show resolved Hide resolved
contracts/core/modules/follow/ApprovalFollowModule.sol Outdated Show resolved Hide resolved
contracts/core/modules/follow/ApprovalFollowModule.sol Outdated Show resolved Hide resolved
contracts/misc/LensPeripheryDataProvider.sol Outdated Show resolved Hide resolved
@LHerskind
Copy link
Contributor

The internal functions do not have natspec documentation

contracts/core/CollectNFT.sol Show resolved Hide resolved
contracts/core/FollowNFT.sol Show resolved Hide resolved
contracts/core/LensHub.sol Show resolved Hide resolved
@@ -892,7 +894,7 @@ contract LensHub is ILensHub, LensNFTBase, VersionedInitializable, LensMultiStat
_collectModuleWhitelisted,
_referenceModuleWhitelisted
);
_profileById[vars.profileId].pubCount++;
++_profileById[vars.profileId].pubCount;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 90d6c69!

contracts/core/LensHub.sol Show resolved Hide resolved
contracts/interfaces/IFollowModule.sol Show resolved Hide resolved
contracts/libraries/InteractionLogic.sol Show resolved Hide resolved
@miguelmtzinf miguelmtzinf self-requested a review March 30, 2022 21:01
miguelmtzinf
miguelmtzinf previously approved these changes Mar 30, 2022
@Zer0dot
Copy link
Contributor Author

Zer0dot commented Mar 30, 2022

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 inheritdoc with internals.

…d manually prevent self-referencing comments.
Copy link
Member

@donosonaumczuk donosonaumczuk left a 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

@Zer0dot Zer0dot merged commit 2799970 into main Apr 1, 2022
@Zer0dot Zer0dot deleted the refactor/general-optimizations branch April 1, 2022 17:11
donosonaumczuk added a commit that referenced this pull request Jul 17, 2023
misc: Fix namespace resolve functions
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.

4 participants