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

Remove deedName(), change visibility of ownerOf() and countOfDeeds() #1

Closed
wants to merge 2 commits into from

Conversation

Nanolucas
Copy link

@Nanolucas Nanolucas commented Feb 5, 2018

deedName() data should be removed entirely or considered optional as not all deeds will have distinct names, even when ERC721Metadata is inherited. Collection name and symbol are important information, but everything about a specific deed should be optional and returned from deedURI data.

This function seems like an unnecessary and potentially confusing overlap/double-up of data that can already be part of deedUri() return data.

deedName() data should be optional as not all deeds will have distinct names, even when ERC721Metadata is inherited. Collection name and symbol are important information, everything about a specific deed should be optional and returned from deedURI data.
ownerOf() and countOfDeeds() are both needed to be called internally in the implementation and as such should be public instead of external
@Nanolucas
Copy link
Author

ownerOf() and countOfDeeds() are both needed to be called internally in the implementation and as such should be public instead of external

@Nanolucas Nanolucas changed the title Remove deedName() Remove deedName(), change visibility of ownerOf() and countOfDeeds() Feb 5, 2018
@fulldecent
Copy link
Owner

Regarding external / public for interfaces. I am currently working with the Solidity team to nail down best practices.

Read more here: ethereum/solidity#3458

See also ethereum/solidity#3038


So I think our code is already correct, it is solidity that is wrong.

Also see caveats section in 841.

@fulldecent
Copy link
Owner

fulldecent commented Feb 5, 2018

Regarding deedName.

"" is a valid string. The function is required. This is a cop-out way to meet the requirement without doing anything useful. So basically optional.


But I agree with you, this shouldn't even be optional. It should be out of the spec. DeedUri is preferred.


But right now I do not want to make any changes to the metadata section. It is listed as an open item. But there is definitely contention on this item.

My plan is to get on the phone with somebody that will be the consumer of this standard. If I can convince them on the phone that DeedURI is good and DeedName is bad that will be great. And if I can get MetaMask on the record here saying they will support this standard with DeedURL and not DeedName then my work is done.

Until then, I want to keep this open for discussion.

@Nanolucas
Copy link
Author

That interface visibility discussion is quite interesting, thanks for linking it. I had no idea they were functionally identical ABIs, so I guess it doesn't really matter what visibility I use when implementing, good to know!

@fulldecent
Copy link
Owner

Also, for real, 10+ issues submitted to Solidity studying 721. There really is a lot of studying. I am still working hard here to keep it moving.

gcolvin pushed a commit that referenced this pull request Mar 29, 2018
@fulldecent fulldecent closed this Sep 4, 2018
fulldecent pushed a commit that referenced this pull request Nov 12, 2019
* Proposed EIP for address and ERC20 transfer rules

* Update eip-X.md

Updating creation date

* Update eip-X.md (#1)

* Update eip-X.md

* Update eip-X.md

* Update eip-X.md

Rule -> IRule consistently
fix missing links
improve abstract

* Update eip-X.md

typos
small improvements
adds implementation section
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.

2 participants