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.

Sign up for GitHub

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

Jump to bottom

Update EIP-5615: Move to Last Call #6185

Merged
merged 3 commits into from
Apr 24, 2023
Merged

Update EIP-5615: Move to Last Call #6185

merged 3 commits into from
Apr 24, 2023

Conversation

Pandapip1
Copy link
Member

@Pandapip1 Pandapip1 commented Dec 20, 2022 •
edited by github-actions bot
Loading

No description provided.

@github-actions github-actions bot added c-status Changes a proposal's status t-erc labels Dec 20, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Dec 20, 2022 •
edited
Loading

✅ All reviewers have approved.

@poojaranjan poojaranjan mentioned this pull request Dec 27, 2022
Closed
12 tasks
SamWilsn
SamWilsn reviewed Dec 27, 2022
View reviewed changes
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.

Your security considerations is still "Needs discussion." That'll need to be filled out (even with just "none") before going into last call.

EIPS/eip-5615.md Outdated Show resolved Hide resolved
@Pandapip1
Copy link
Member Author

Done (this branch isn't updated)

SamWilsn
SamWilsn requested changes Feb 23, 2023
View reviewed changes
EIPS/eip-5615.md Outdated Show resolved Hide resolved
EIPS/eip-5615.md Outdated Show resolved Hide resolved
@Pandapip1
Copy link
Member Author

@SamWilsn fixed

@Pandapip1
Copy link
Member Author

@eth-bot rerun

@eth-bot eth-bot added the e-review Waiting on editor to review label Feb 24, 2023
@poojaranjan poojaranjan mentioned this pull request Mar 7, 2023
Closed
25 tasks
SamWilsn
SamWilsn previously requested changes Mar 8, 2023
View reviewed changes
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.

Whether the given token id exists, previously existed, or may exist

This is ambiguous. Should exists return true or false in each of these cases:

  • Token cannot exist (unambiguously false?)
  • Token exists (unambiguously true?)
  • Token could exist, but has never been minted.
  • Token has existed, but has been destroyed.

I'd reword and move some bits around:

Move to specification:

Implementations MAY support ERC-165 interface discovery with an id of 0x..., but consumers MUST NOT rely on it.

Move to rationale:

This EIP does not mandate ERC-165 support as this interface is simple enough that the extra complexity is unnecessary and would cause incompatibilities with pre-existing implementations.

@Pandapip1
Copy link
Member Author

Pandapip1 commented Mar 8, 2023 •
edited
Loading

This is ambiguous. Should exists return true or false in each of these cases:

Token cannot exist (unambiguously false?)
Token exists (unambiguously true?)
Token could exist, but has never been minted.
Token has existed, but has been destroyed.

False.
True.
True.
True.

It's not ambiguous. It's a binary OR of all three options. Nonetheless, I'm wondering if 3/4 should actually be true. I'll revise it. Ah yea, that's right. It should be checked with totalSupply.

@Pandapip1 Pandapip1 requested a review from SamWilsn March 8, 2023 22:29
xinbenlv
xinbenlv reviewed Apr 14, 2023
View reviewed changes
EIPS/eip-5615.md
@@ -4,7 +4,8 @@ title: EIP-1155 Supply Extension
description: A simple mechanism to fetch token supply data from EIP-1155 tokens
author: Pandapip1 (@Pandapip1)
discussions-to: https://ethereum-magicians.org/t/eip-5615-eip-1155-supply-extension/10732
status: Review
status: Last Call
last-call-deadline: 2023-03-08
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
last-call-deadline: 2023-03-08
last-call-deadline: 2023-04-27

xinbenlv
xinbenlv approved these changes Apr 14, 2023
View reviewed changes
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 read through it and believe there is no ambiguity about when to return zero.

Approved and please don't forget to update the deadline

@eth-bot eth-bot changed the title Update EIP-5615: Move to last call Update EIP-5615: Move to Last Call Apr 14, 2023
@eth-bot eth-bot enabled auto-merge (squash) April 14, 2023 17:25
eth-bot
eth-bot approved these changes Apr 14, 2023
View reviewed changes
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

eth-bot
eth-bot approved these changes Apr 14, 2023
View reviewed changes
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit 054a72e into master Apr 24, 2023
@eth-bot eth-bot deleted the eip-5615-last-call branch April 24, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eth-bot eth-bot eth-bot approved these changes

@xinbenlv xinbenlv xinbenlv approved these changes

@SamWilsn SamWilsn Awaiting requested review from SamWilsn

Assignees
No one assigned
Labels
c-status Changes a proposal's status e-review Waiting on editor to review t-erc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Pandapip1 @eth-bot @xinbenlv @SamWilsn