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

Use ERC721Holder & ERC1155Holder in the TimelockController #4284

Merged
merged 7 commits into from Jun 15, 2023
Merged

Use ERC721Holder & ERC1155Holder in the TimelockController #4284

merged 7 commits into from Jun 15, 2023

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented May 26, 2023

Fixes #3936
Fixes LIB-838

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@Amxx Amxx added this to the 5.0 milestone May 26, 2023
@Amxx Amxx requested review from frangio and ernestognw May 26, 2023 13:58
@changeset-bot
Copy link

changeset-bot bot commented May 26, 2023 •
edited

⚠️ No Changeset found

Latest commit: e2cf54a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@socket-security
Copy link

socket-security bot commented May 26, 2023 •
edited

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@Amxx Amxx changed the base branch from master to next-v5.0 May 26, 2023 13:59
frangio
frangio reviewed May 26, 2023
View reviewed changes
contracts/governance/Governor.sol Outdated
@@ -25,7 +25,7 @@ import "./IGovernor.sol";
*
* _Available since v4.3._
*/
abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receiver, IERC1155Receiver {
abstract contract Governor is Context, ERC165, EIP712, IGovernor, ERC721Holder, ERC1155Holder {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is our convention for ordering of interfaces within inheritance lists? I would like to include this in the guidelines doc.

I would always put interfaces first but I think you always put them last. In this case there is an added weirdness though because IGovernor is an abstract contract.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My main goal was to have the inheritance ordering tests pass. I first I wanted to reorder some things, and I spent ~2h down the rabit hole of fixing ordering when the solution was staring at me from the start.

scripts/checks/inheritance-ordering.js Outdated Show resolved Hide resolved
@Amxx @frangio
Apply suggestions from code review
64ad8cd 
Co-authored-by: Francisco <fg@frang.io>
@frangio
Copy link
Contributor

frangio commented May 27, 2023

Wow, I've been trying for a while to get the inheritance ordering to be consistent and I can't get it to work.

@Amxx
Copy link
Collaborator Author

Amxx commented May 29, 2023 •
edited

Wow, I've been trying for a while to get the inheritance ordering to be consistent and I can't get it to work.

Yes, I sent over 2h on that too. We need a good guideline (IMO interfaces should come before implementations)

EDIT: my proposal (before your changes) was OK on my machine. I'm not sure why its failing here.

ernestognw
ernestognw reviewed May 30, 2023
View reviewed changes
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

I don't see a major issue with this, but I see there's a conversation around the inheritance order, can you explain me real quick why is the inheritance an issue here?

Consider this an LGTM if you sort it out.

This was referenced Jun 5, 2023
Merged
Closed
@Amxx
Copy link
Collaborator Author

Amxx commented Jun 5, 2023

I'm starting to doubt the value of this PR if the hooks are overridden by #4314 anyway.

@ernestognw
Copy link
Member

I'm starting to doubt the value of this PR if the hooks are overridden by #4314 anyway.

Just got back to this issue after reviewing tests for #4348, why would be the problem of being overridden by #4314? We'll still need it for Timelock independently I'd say

@Amxx
Copy link
Collaborator Author

Amxx commented Jun 14, 2023

I'm starting to doubt the value of this PR if the hooks are overridden by #4314 anyway.

Just got back to this issue after reviewing tests for #4348, why would be the problem of being overridden by #4314? We'll still need it for Timelock independently I'd say

Right, the timelock part (that is not actually difficult) makes sens. The governor part (which is the one we struggle with) not so much.

Should we only keep the timelock part of this PR and merge it?

@frangio
Copy link
Contributor

frangio commented Jun 14, 2023

Should we only keep the timelock part of this PR and merge it?

This sounds good to me.

@Amxx Amxx changed the title Use ERC721Holder & ERC1155Holder for Governor, Timelock Use ERC721Holder & ERC1155Holder in the TimelockController Jun 14, 2023
@Amxx Amxx requested review from ernestognw and frangio June 14, 2023 19:10
frangio
frangio previously approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

LGTM. Storage layout check is failing but this is expected.

ernestognw
ernestognw previously approved these changes Jun 15, 2023
View reviewed changes
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Amxx!

@Amxx Amxx changed the base branch from next-v5.0 to master June 15, 2023 20:33
@Amxx Amxx dismissed stale reviews from ernestognw and frangio June 15, 2023 20:33

The base branch was changed.

@Amxx
Copy link
Collaborator Author

Amxx commented Jun 15, 2023

This PR was still targeting next-v5.0.

I had to retarget and resolve conflicts, which dismissed the reviews :/

@Amxx Amxx requested review from frangio and ernestognw June 15, 2023 20:35
@frangio frangio merged commit c014c8f into OpenZeppelin:master Jun 15, 2023
13 of 14 checks passed
@Amxx Amxx deleted the refactro/governor-is-erc721-holder branch June 16, 2023 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frangio frangio frangio approved these changes

@ernestognw ernestognw Awaiting requested review from ernestognw

Assignees
No one assigned
Labels
ignore-changeset
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.

Use ERC721Holder & ERC1155Holder for Governor, Timelock
3 participants
@Amxx @frangio @ernestognw