Use ERC721Holder & ERC1155Holder in the TimelockController by Amxx · Pull Request #4284 · OpenZeppelin/openzeppelin-contracts (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits7 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fixes #3936
Fixes LIB-838
PR Checklist
- Tests
- Documentation
- Changeset entry (run
npx changeset add)
No dependency changes detected. Learn more about Socket for GitHub ↗︎
👍 No dependency changes detected in pull request
| @@ -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 { |
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.
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.
Co-authored-by: Francisco fg@frang.io
Wow, I've been trying for a while to get the inheritance ordering to be consistent and I can't get it to work.
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.
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
I'm starting to doubt the value of this PR if the hooks are overridden by #4314 anyway.
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
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?
Should we only keep the timelock part of this PR and merge it?
This sounds good to me.
Use ERC721Holder & ERC1155Holder for Governor, Timelock Use ERC721Holder & ERC1155Holder in the TimelockController
frangio previously approved these changes Jun 14, 2023
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR was still targeting next-v5.0.
I had to retarget and resolve conflicts, which dismissed the reviews :/