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

Add EIP-5773: Multi-Resource Token standard #5773

Merged
merged 21 commits into from Nov 25, 2022

Conversation

ThunderDeliverer
Copy link
Contributor

This EIP draft proposal includes RMRK team's work on developing an NFT standard where a single NFT can be tied to multiple resources.

RMRK team has developed a next step in NFTs where one NFT can be tied to
multiple resources.
@eth-bot
Copy link
Collaborator

eth-bot commented Oct 10, 2022

All tests passed; auto-merging...

(pass) eip-5773.md

classification
updateEIP
  • passed!

(pass) assets/eip-5773/contracts/IMultiAsset.sol

classification
ambiguous
  • file assets/eip-5773/contracts/IMultiAsset.sol is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

(pass) assets/eip-5773/contracts/MultiAssetToken.sol

classification
ambiguous
  • file assets/eip-5773/contracts/MultiAssetToken.sol is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

(pass) assets/eip-5773/contracts/library/MultiAssetLib.sol

classification
ambiguous
  • file assets/eip-5773/contracts/library/MultiAssetLib.sol is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

(pass) assets/eip-5773/contracts/mocks/ERC721ReceiverMock.sol

classification
ambiguous
  • file assets/eip-5773/contracts/mocks/ERC721ReceiverMock.sol is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

(pass) assets/eip-5773/contracts/mocks/MultiAssetTokenMock.sol

classification
ambiguous
  • file assets/eip-5773/contracts/mocks/MultiAssetTokenMock.sol is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

(pass) assets/eip-5773/contracts/mocks/NonReceiverMock.sol

classification
ambiguous
  • file assets/eip-5773/contracts/mocks/NonReceiverMock.sol is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

(pass) assets/eip-5773/contracts/utils/MultiAssetRenderUtils.sol

classification
ambiguous
  • file assets/eip-5773/contracts/utils/MultiAssetRenderUtils.sol is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

(pass) assets/eip-5773/hardhat.config.ts

classification
ambiguous
  • file assets/eip-5773/hardhat.config.ts is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

(pass) assets/eip-5773/package.json

classification
ambiguous
  • file assets/eip-5773/package.json is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

(pass) assets/eip-5773/test/multiasset.ts

classification
ambiguous
  • file assets/eip-5773/test/multiasset.ts is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

(pass) assets/eip-5773/test/renderUtils.ts

classification
ambiguous
  • file assets/eip-5773/test/renderUtils.ts is associated with EIP 5773; because there are also changes being made to EIPS/eip-5773.md all changes to corresponding assets are also allowed

@ThunderDeliverer
Copy link
Contributor Author

ThunderDeliverer commented Oct 10, 2022

I intend to add discussion link at the same time as I will update the EIP number, to reduce the number of pushes to the initial PR.

@github-actions github-actions bot added c-new Creates a brand new proposal e-number Waiting on EIP Number assignment s-draft This EIP is a Draft t-erc labels Oct 10, 2022
title: Multi-Resource Token
description: An interface for Multi-Resource tokens.
author: Bruno Škvorc (@Swader), Cicada (@CicadaNCR), Steven Pineda (@steven2308), Stevan Bogosavljevic (@stevyhacker), Jan Turk (@ThunderDeliverer)
discussions-to: <URL>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't forget to create a discussion thread :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intended to add it at the same time as the EIP number was assigned (wanted to include the EIP number in the discussion title). I'll open a discussion now and add it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✅

Copy link
Contributor

Choose a reason for hiding this comment

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

I intended to add it at the same time as the EIP number was assigned (wanted to include the EIP number in the discussion title). I'll open a discussion now and add it :)

I thought the bot was going to assign the number and create the thread automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intended to add it at the same time as the EIP number was assigned (wanted to include the EIP number in the discussion title). I'll open a discussion now and add it :)

I thought the bot was going to assign the number and create the thread automatically.

It only reports the missing EIP number and that an editor should assign it. Should I just use the PR number? So 5773?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll use it as this EIP is already being referred to as EIP-5773:
https://weekinethereumnews.com/week-in-ethereum-news-october-15-2022
https://twitter.com/WeekInEthNews/status/1582456945750904832?s=20&t=0H712tazBBqHtRQgD227OA

The number can be changed at any time, if you decide to assign a different one 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I thought the bot was going to assign the number and create the thread automatically.

Not yet. We're waiting on #5508.

@github-actions github-actions bot removed the e-number Waiting on EIP Number assignment label Oct 18, 2022
@Pandapip1 Pandapip1 changed the title Propose Multi-Resource Token standard Add EIP-5773: Multi-Resource Token standard Oct 23, 2022
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.

Here's my review so far.

EIPS/eip-5773.md Outdated

## Abstract

The Multi-Resource NFT standard allows for the construction of a new primitive: context-dependent output of multimedia information per single NFT.
Copy link
Member

Choose a reason for hiding this comment

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

WDYM by primitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were referring to a simple, clearly defined, structure

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The Multi-Resource NFT standard allows for the construction of a new primitive: context-dependent output of multimedia information per single NFT.
The Multi-Resource NFT standard allows for the construction of a new simple, clearly defined structure for context-dependent output of multimedia information per single NFT.

"Primitive" is very often associated with language features. I suggest this wording.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense. Thank you 😄

EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
@ThunderDeliverer
Copy link
Contributor Author

Here's my review so far.

Thank you for the comments, I applied the changes based on them and pushed them

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
EIPS/eip-5773.md Outdated
@@ -0,0 +1,362 @@
---
eip: 5773
title: Multi-Resource Token
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a bit too vague. Try to expand a tiny bit about what resources are, or what you can do with these tokens while staying under the character limit.

EIPS/eip-5773.md Outdated
---
eip: 5773
title: Multi-Resource Token
description: An interface for Multi-Resource tokens.
Copy link
Contributor

Choose a reason for hiding this comment

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

Try not to make this just a rewording of the title. It should expand a little beyond what's in the title.

EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated
Comment on lines 259 to 270

RMRK MultiResource lego block and documentation

- Compatible with the original version of the standard

Neon Crisis, by CicadaNCR

- A NFT game utilizing RMRK MultiResource lego block

Snake Soldiers, by Steven Pineda

- A NFT game utilizing RMRK MultiResource lego block
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include the reference implementation in the assets directory if you have a compatible license (free to redistribute and doesn't place additional restrictions on downstream projects like Apache, MIT, or preferably CC0.)

If you cannot include them, just remove these projects.

EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Show resolved Hide resolved
Applied the changes based on feedback received on the EIP Editing Office
Hour Meeting ethereum#5.

This commit includes changes to the proposal as well as adds an exaple
implementation to assets/ directory.
@ThunderDeliverer ThunderDeliverer requested review from Pandapip1 and removed request for xinbenlv November 15, 2022 13:37
@ThunderDeliverer ThunderDeliverer requested review from SamWilsn and Pandapip1 and removed request for Pandapip1 and SamWilsn November 15, 2022 13:37
EIPS/eip-5773.md Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
assets/eip-5773/hardhat.config.ts Outdated Show resolved Hide resolved
assets/eip-5773/contracts/MultiResourceToken.sol Outdated Show resolved Hide resolved
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
The proposal was renamed to Context-Dependent Multi-Asset Tokens to
better illustrate its function.

Another example was added to represent the possible IoT usecase and the
explanation on the naming decision was added to the rationale.

The examples were relicensed to CC0, to conform to the requirements of
EIP repository.
assets/eip-5773/contracts/MultiAssetToken.sol Outdated Show resolved Hide resolved
assets/eip-5773/contracts/IMultiAsset.sol Outdated Show resolved Hide resolved
assets/eip-5773/contracts/library/MultiAssetLib.sol Outdated Show resolved Hide resolved
assets/eip-5773/contracts/mocks/ERC721ReceiverMock.sol Outdated Show resolved Hide resolved
assets/eip-5773/contracts/mocks/MultiAssetTokenMock.sol Outdated Show resolved Hide resolved
assets/eip-5773/contracts/mocks/NonReceiverMock.sol Outdated Show resolved Hide resolved
EIPS/eip-5773.md Outdated Show resolved Hide resolved
assets/eip-5773/contracts/utils/MultiAssetRenderUtils.sol Outdated Show resolved Hide resolved
@eth-bot eth-bot enabled auto-merge (squash) November 25, 2022 20:57
@SamWilsn SamWilsn dismissed Pandapip1’s stale review November 25, 2022 21:02

a privitive is generally a building block, and isn't necessarily limited to programming languages

@eth-bot eth-bot merged commit a3bcb50 into ethereum:master Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants