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

EIP-4883 Composable SVG NFT #4888

Merged
merged 19 commits into from
Aug 6, 2022
Merged

EIP-4883 Composable SVG NFT #4888

merged 19 commits into from
Aug 6, 2022

Conversation

abcoathup
Copy link
Contributor

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@eth-bot
Copy link
Collaborator

eth-bot commented Mar 9, 2022 •
edited
Loading

All tests passed; auto-merging...

(pass) eip-4883.md

classification
updateEIP
  • passed!

@abcoathup abcoathup changed the title Composable SVG EIP-4888 ERC721 Composable SVG Mar 9, 2022
@abcoathup abcoathup changed the title EIP-4888 ERC721 Composable SVG [DRAFT] EIP-4888 ERC721 Composable SVG Mar 9, 2022
@abcoathup
Copy link
Contributor Author

This EIP is currently a draft.

MicahZoltu
MicahZoltu previously requested changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

At a minimum, a specification is required for a Draft EIP. If you don't have a specification ready, I recommend closing this PR and waiting to open it until you do.

EIP number sniping and gaming is not allowed, and since this PR was open a mere 2 minutes after PR 4887 and doesn't contain any meaningful content, I'm overriding our normal numbering system in favor of a manual assignment of 4883 here.

EIPS/eip-4888.md Outdated Show resolved Hide resolved
EIPS/eip-4888.md Outdated Show resolved Hide resolved
EIPS/eip-4888.md Outdated Show resolved Hide resolved
EIPS/eip-4888.md Outdated Show resolved Hide resolved
@abcoathup
Copy link
Contributor Author

EIP number sniping and gaming is not allowed, and since this PR was open a mere 2 minutes after PR 4887 and doesn't contain any meaningful content, I'm overriding our normal numbering system in favor of a manual assignment of 4883 here.

I check the repo most days (as part of my job), saw that a meaningful number to me was next up and thought I had won the lottery so did a PR with a very early draft. Definitely not number gaming (unlike say 4844), more of a lunge, I'll update the draft next week.

@abcoathup abcoathup changed the title [DRAFT] EIP-4888 ERC721 Composable SVG [DRAFT] EIP-4883 ERC721 Composable SVG Mar 11, 2022
@SamWilsn
Copy link
Contributor

Let me know when you open your discussion thread. I've seen at least EIP-4841 by @Soohan-Park which is also proposing extensions for better SVG support. Might be a good idea to get an informal working group together!

@PradhumnaPancholi
Copy link

Just checking in, do we have a discussion thread going on for this?

@abcoathup abcoathup changed the title [DRAFT] EIP-4883 ERC721 Composable SVG [DRAFT] EIP-4883 Composable SVG NFT Mar 30, 2022
@AlexPartyPanda
Copy link

Let me know when you open your discussion thread

My discussion thread post to Ethereum-magicians is currently pending, waiting for their moderators to approve

@MicahZoltu
Copy link
Contributor

My discussion thread post to Ethereum-magicians is currently pending, waiting for their moderators to approve

@poojaranjan Hmm, is this something we can tune? What are the conditions that trigger needing approval?

@poojaranjan
Copy link
Contributor

@MicahZoltu It shouldn't be long waiting for a discussion link to be live. Checking with the FEM team to figure if it is an exceptional case.

@AlexPartyPanda Can you please share the link here or maybe with me at ECH Discord.

@poojaranjan
Copy link
Contributor

@MicahZoltu

What are the conditions that trigger needing approval?

As it appears, Discourse sometimes choose to hold a post, often it is a new users or even if they seem to be performing the task "too quickly".

Link is live now - https://ethereum-magicians.org/t/eip-4883-composable-svg-nft/8765
cc: @AlexPartyPanda @abcoathup

@PradhumnaPancholi
Copy link

Thanks, @poojaranjan, for getting it live.

@abcoathup
Copy link
Contributor Author

As it appears, Discourse sometimes choose to hold a post, often it is a new users or even if they seem to be performing the task "too quickly".

Thanks @poojaranjan.

@AlexPartyPanda had a new account on Eth magicians, so was limited to including two links in his post. We tried to work around this and when we did post, pasted the whole text of the discussion in one go, so was picked up by Discourse as potential spam.

Something to be aware of for EIP discussions that new users are limited to two links.

@MicahZoltu MicahZoltu dismissed their stale review July 28, 2022 05:59

addressed.

@abcoathup abcoathup requested review from Pandapip1 and SamWilsn and removed request for Pandapip1 and SamWilsn August 3, 2022 02:03
Pandapip1
Pandapip1 suggested changes Aug 3, 2022
View reviewed changes
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

LGTM except for this.

EIPS/eip-4883.md Outdated Show resolved Hide resolved
auto-merge was automatically disabled August 4, 2022 04:04

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) August 4, 2022 04:05
@abcoathup abcoathup requested a review from Pandapip1 August 4, 2022 04:05
@abcoathup
Copy link
Contributor Author

Auto-Merge Bot fails even though @Pandapip1 has reviewed?

File with name EIPS/eip-4883.md is new and new files must be reviewed
This PR requires review from one of [lightclient, axic, samwilsn, pandapip1]

@Pandapip1
Copy link
Member

Pandapip1 commented Aug 6, 2022 •
edited
Loading

Close and re-open the PR. There's a fix in #5400 but it has yet to be merged.

@MicahZoltu MicahZoltu closed this Aug 6, 2022
auto-merge was automatically disabled August 6, 2022 04:51

Pull request was closed

@MicahZoltu MicahZoltu reopened this Aug 6, 2022
@eth-bot eth-bot enabled auto-merge (squash) August 6, 2022 04:51
@eth-bot eth-bot merged commit 1b871be into ethereum:master Aug 6, 2022
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
@abcoathup @MicahZoltu
@Pandapip1 @nachomazzara
EIP-4883 Composable SVG NFT (ethereum#4888)
bbf6d94 
* create draft

* Create eip-4888.md

* Delete eip-erc721composablesvg.md

* Update eip-4888.md

* Apply suggestions from code review

Co-authored-by: Micah Zoltu <[email protected]>

* Create eip-4883.md

* Delete eip-4888.md

* eip-4883 add reference to svg specification

* add discussions link

* require rights

* add authors

* update authors

* make zindex optional

* rename render

Rename `render` function to ` renderTokenById` to match Loogie composable NFT projects

* address reviews and discussion feedback

* fixes for eipw validator

* fixes for eipw validator

* Apply suggestions from Padnapip1 review

Co-authored-by: Pandapip1 <[email protected]>

* remove url from code comment

Co-authored-by: Pandapip1 <[email protected]>

Co-authored-by: Micah Zoltu <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
@abcoathup abcoathup mentioned this pull request Feb 24, 2023
Closed
9 tasks
@abcoathup abcoathup mentioned this pull request Mar 10, 2023
Closed
@abcoathup abcoathup mentioned this pull request Mar 30, 2023
Merged
This was referenced May 2, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SamWilsn SamWilsn SamWilsn left review comments

@MicahZoltu MicahZoltu MicahZoltu left review comments

@Pandapip1 Pandapip1 Pandapip1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@abcoathup @eth-bot @SamWilsn @PradhumnaPancholi @AlexPartyPanda @MicahZoltu @poojaranjan @bitcoinbrisbane @Pandapip1