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

Update EIP-721: Use latest version of solidity and remove caveats #5887

Closed
wants to merge 1 commit into from

Conversation

Pandapip1
Copy link
Member

All of the caveats mentioned in the EIP have been implemented in solidity. Thus, they are no longer relevant if the solidity version is updated to the most recent one.

@github-actions github-actions bot added c-update Modifies an existing proposal s-final This EIP is Final t-erc labels Nov 7, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Nov 7, 2022

All tests passed; auto-merging...

(pass) eip-721.md

classification
updateEIP
  • passed!

@Pandapip1 Pandapip1 added the a-review Waiting on author to review label Nov 7, 2022
@eth-bot eth-bot enabled auto-merge (squash) November 7, 2022 13:01
@Pandapip1 Pandapip1 added this to the Manual Merge Queue milestone Nov 7, 2022
@@ -41,12 +41,12 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "S
**Every ERC-721 compliant contract must implement the `ERC721` and `ERC165` interfaces** (subject to "caveats" below):

```solidity
pragma solidity ^0.4.20;
pragma solidity ^0.8.17;
Copy link
Contributor

Choose a reason for hiding this comment

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

In safeTransferFrom:

TypeError: Data location must be "memory" or "calldata" for parameter in external function, but none was given.

Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

I am not sure that these changes "correct errata [or] add non-normative clarifications":

  • The caveats section still applies for Solidity 0.4.20, so these changes aren't errata.
  • Removing the caveats section obviously isn't adding clarifications.

I'd recommend that we discuss this on EIPIP.

@Pandapip1
Copy link
Member Author

This adds a non-normative clarification: an outdated version of solidity is removed, and a new version is added. To make this work, an outdated section describing the limitations of an old version of solidity has to removed - an additional clarification that is added.

These are clarifications that are added to EIP-721, even though they remove stuff.

@SamWilsn
Copy link
Contributor

Lets discuss on discord/EIPIP.

@SamWilsn
Copy link
Contributor

Closing as per discussion in EIPIP. If you'd like to continue discussion, feel free to re-open.

@SamWilsn SamWilsn closed this Nov 16, 2022
auto-merge was automatically disabled November 16, 2022 15:09

Pull request was closed

@Pandapip1 Pandapip1 deleted the eip-721-no-solidity-caveats branch November 16, 2022 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-update Modifies an existing proposal s-final This EIP is Final t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants