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

Add IERC5313.sol #4013

Merged
merged 4 commits into from
Feb 3, 2023
Merged

Add IERC5313.sol #4013

merged 4 commits into from
Feb 3, 2023

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jan 30, 2023

EIP is final: https://eips.ethereum.org/EIPS/eip-5313

Should we inherit that in Ownable? AccessControlAdmin?

PR Checklist

  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Jan 30, 2023

🦋 Changeset detected

Latest commit: d036a21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

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

contracts/interfaces/IERC5313.sol Outdated Show resolved Hide resolved
@Amxx
Copy link
Collaborator Author

Amxx commented Jan 31, 2023

@frangio Do we want the interface folder to contain:

  • exact copied of the interfaces defined in the EIPs (and not properly show up in our docs)
  • modified versions that are not exact copies of the EIP document, but that show up in our docs.

@ernestognw
Copy link
Member

@frangio Do we want the interface folder to contain:

  • exact copied of the interfaces defined in the EIPs (and not properly show up in our docs)
  • modified versions that are not exact copies of the EIP document, but that show up in our docs.

It's already modified, we're not keeping the Solidity version and LICENSE. I know it's a minor detail but since we're doing that, why not keep our doc slightly more explicit?

@frangio
Copy link
Contributor

frangio commented Jan 31, 2023

I don't think we need interfaces to be copied exactly from the EIPs. We can definitely improve the documentation.

@frangio
Copy link
Contributor

frangio commented Feb 1, 2023

Should we inherit that in Ownable?

It doesn't really make any difference does it? Either way is fine with me.

AccessControlAdmin?

Yes I think so. By the way, that may be a good name rather than AccessControlAdminRules.

Co-authored-by: Ernesto García <ernestognw@gmail.com>
@Amxx Amxx enabled auto-merge (squash) February 3, 2023 09:03
@frangio
Copy link
Contributor

frangio commented Feb 3, 2023

Upstream Foundry/Solidity error in foundry tests.

@frangio frangio disabled auto-merge February 3, 2023 18:57
@frangio frangio merged commit 132e5aa into OpenZeppelin:master Feb 3, 2023
@Amxx Amxx deleted the interfaces/erc/5313 branch February 3, 2023 20:16
@Amxx Amxx mentioned this pull request Apr 26, 2024
3 tasks
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.

None yet

3 participants