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.

Sign up for GitHub

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

Jump to bottom

Issue-#620: Review that all smart contracts implements its interface #626

Merged
merged 21 commits into from
May 9, 2022
Merged

Issue-#620: Review that all smart contracts implements its interface #626

merged 21 commits into from
May 9, 2022

Conversation

miquelcabot
Copy link
Contributor

Fixes #620

Changes proposed in this PR:

  • Review that all smart contracts implements its interface

@miquelcabot miquelcabot self-assigned this May 3, 2022
@miquelcabot miquelcabot marked this pull request as ready for review May 4, 2022 07:55
trentmc
trentmc previously requested changes May 4, 2022
View reviewed changes
Copy link
Member

@trentmc trentmc left a comment

Choose a reason for hiding this comment

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

I gave several inline comments. Overall I was not super-thorough to ensure each interface is met - to that I defer to other reviewers doing backend stuff. Rather, I focused on a few things that caught my eye.

contracts/ERC721Factory.sol Outdated Show resolved Hide resolved
contracts/communityFee/OPFCommunityFeeCollector.sol Outdated Show resolved Hide resolved
contracts/communityFee/OPFCommunityFeeCollector.sol Outdated Show resolved Hide resolved
contracts/pools/FactoryRouter.sol Show resolved Hide resolved
@alexcos20
Copy link
Member

Please install this VSC extension: JuanBlanco.solidity
And do a npm i, so you can install solhint as well

@miquelcabot miquelcabot requested a review from trentmc May 5, 2022 09:30
@trentmc
Copy link
Member

trentmc commented May 5, 2022

For remaining review: I defer to Alex. And I guess you have a first action from him (JuanBlanco.solidity and solhint).

@miquelcabot
Copy link
Contributor Author

@alexcos20 @trentmc I had already installed JuanBlanco.solidity extension.
I think that this PR is ready to be reviewed.
Thanks!

alexcos20
alexcos20 requested changes May 5, 2022
View reviewed changes
contracts/interfaces/IFactoryRouter.sol Show resolved Hide resolved
contracts/interfaces/IFactoryRouter.sol Show resolved Hide resolved
contracts/interfaces/ISideStaking.sol Show resolved Hide resolved
contracts/interfaces/ISideStaking.sol Show resolved Hide resolved
@alexcos20 alexcos20 dismissed trentmc’s stale review May 9, 2022 05:27

issues are fixed

@alexcos20 alexcos20 self-requested a review May 9, 2022 05:27
@alexcos20 alexcos20 added the Status: DoNotMerge Don't merge this PR label May 9, 2022
@alexcos20 alexcos20 removed the Status: DoNotMerge Don't merge this PR label May 9, 2022
@alexcos20 alexcos20 merged commit d396efc into v4main May 9, 2022
@alexcos20 alexcos20 deleted the issue-620-review-implementation-of-interfaces branch May 9, 2022 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcos20 alexcos20 alexcos20 approved these changes

@trentmc trentmc Awaiting requested review from trentmc

Assignees

@miquelcabot miquelcabot

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Review that all smart contracts implements its interface
3 participants
@miquelcabot @alexcos20 @trentmc