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-5615: Move to Review #5778

Merged
merged 6 commits into from Nov 25, 2022
Merged

Update EIP-5615: Move to Review #5778

merged 6 commits into from Nov 25, 2022

Conversation

Pandapip1
Copy link
Member

Makes a few clarifications as well

@github-actions github-actions bot added c-status Changes a proposal's status s-review This EIP is in Review t-erc labels Oct 11, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Oct 11, 2022

A critical exception has occurred:
Message: pr 5778 is already merged; quitting
(cc @alita-moore, @mryalamanchi)

EIPS/eip-5615.md Outdated Show resolved Hide resolved
Co-authored-by: xinbenlv <zzn-github@zzn.im>
EIPS/eip-5615.md Outdated Show resolved Hide resolved
Copy link
Contributor

@xinbenlv xinbenlv left a comment

Choose a reason for hiding this comment

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

I think it's ok to move to Review. I'd love to hear more rationale of the choice of exists() method and alternatives that has been considered.

A separate note: while I personally think this is an useful EIP, and I disagree with the "suggestions" in determining whether something worth a EIP, made in #5737, I like to hear @fulldecent 's thought on how the suggestions will evaluate this particular EIP, putting that suggestion into tests with real world EIPs

@fulldecent
Copy link
Contributor

As per #5737, I expect the author here should cite some people that are using this already. At a minimum this should be in the official discussion record.

They started that by saying it is an OZ contract. But it would be nice if they actually did the work of going and finding some live implementations.

In addition to find some people implementing the producer of this function, they should also go find and documentation that is usefully querying this information.

Yes, this might even take a few minutes of time! But the goal here is to publish useful, helpful things, not just find the remaining function names in 4bytes, put your name on it, and "earn" one of the remaining 5,000 EIP numbers.

@Pandapip1
Copy link
Member Author

Pandapip1 commented Nov 6, 2022

As per #5737, I expect the author here should cite some people that are using this already. At a minimum this should be in the official discussion record.

I'll add a few relevant links. I don't think this should necessarily be included in the EIP itself, but the linking in the discussion thread is a no-brainer.

OpenSea supports both of these functions. This is undocumented but is clear from their example repository. It's also noted in the first thread I just linked to. I don't know whether to mention this or not. I'm leaning slightly towards no (it's undocumented!) but my mind can be easily changed on this.

OpenZeppelin, of course, has an implementation of this, as noted in the EIP itself.

I don't see an easy way to track support for this for deployed contracts.

@fulldecent
Copy link
Contributor

@Pandapip1 cool reference. 1155 requires strict token reporting so totalsupply function should be duplicative of the token transfers. If OS does not say they are using this function to collect data for their production website we should assume they are not using it

@Pandapip1
Copy link
Member Author

Pandapip1 commented Nov 7, 2022

If OS does not say they are using this function to collect data for their production website we should assume they are not using it

OpenSea doesn't have any documentation about support for EIP-1155 at all, except for a page in the sidebar with a "documentation soon™." It's well-known that OS supports EIP-1155, though.

@Pandapip1
Copy link
Member Author

It would be nice if someone from @ProjectOpenSea could weigh in.

SamWilsn
SamWilsn previously approved these changes Nov 15, 2022
@Pandapip1 Pandapip1 dismissed stale reviews from SamWilsn and ghost via 2432605 November 16, 2022 14:07
@github-actions github-actions bot added c-new Creates a brand new proposal c-update Modifies an existing proposal e-number Waiting on EIP Number assignment s-draft This EIP is a Draft s-final This EIP is Final labels Nov 16, 2022
@Pandapip1
Copy link
Member Author

That did not work :|

@github-actions github-actions bot removed c-new Creates a brand new proposal c-update Modifies an existing proposal t-core t-interface t-meta t-networking t-process e-number Waiting on EIP Number assignment s-draft This EIP is a Draft s-final This EIP is Final s-stagnant This EIP is Stagnant labels Nov 16, 2022
@Pandapip1
Copy link
Member Author

There we are.

@eth-bot eth-bot enabled auto-merge (squash) November 25, 2022 21:22
@eth-bot eth-bot merged commit b0fb1ef into master Nov 25, 2022
@eth-bot eth-bot deleted the eip-5615-review branch November 25, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-status Changes a proposal's status s-review This EIP is in Review t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants