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

Improve ERC1363 documentation #3993

Merged

Conversation

vittominacori
Copy link
Contributor

Noticed that the interface id to be returned in IERC1363 comment has a wrong value.

It splits the interface ids in 2 different values instead of an unique one as defined in the final EIP specification.

Then I also added some comments and changed the value param to amount to be ERC20 like.

I think that all the pending PRs (#3017, #3525) to add ERC1363 implementation are now obsolete so if you need some help to add ERC1363 implementation feel free to ask and I will try adding it opening a fresh new PR following the new OZ guidelines.

PR Checklist

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

@changeset-bot
Copy link

changeset-bot bot commented Jan 24, 2023

⚠️ No Changeset found

Latest commit: b52ffcd

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

Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

The IERC1363 interfaceId is indeed documented incorrectly.

For the value vs amount issue, I'm not sure if we want to deviate from the EIP document. Could it possibly break library that extracts names of arguments?

@vittominacori
Copy link
Contributor Author

For the value vs amount issue, I'm not sure if we want to deviate from the EIP document. Could it possibly break library that extracts names of arguments?

Also the IERC20 interface differs from the official EIP using to and amount instead of _to and _value.
I would like to keep the same signature of ERC20.
Let me know if you want to revert back to official EIP1363 signature.

@vittominacori
Copy link
Contributor Author

Any update?

@frangio frangio changed the title fix: wrong erc1363 interface identifier in comment Improve ERC1363 documentaiton Feb 24, 2023
@frangio frangio changed the title Improve ERC1363 documentaiton Improve ERC1363 documentation Feb 24, 2023
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.

Thank you @vittominacori.

Re: amount vs value. There is currently an inconsistency around the use of these terms that we will need to tackle (see #3864), but I agree with the reasoning for the change in this PR.

@frangio frangio merged commit 3f3774c into OpenZeppelin:master Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants