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-5380: EIP-721 Entitlement Extension #5380

Merged
merged 18 commits into from
Sep 3, 2022
Merged

Conversation

Pandapip1
Copy link
Member

@Pandapip1 Pandapip1 commented Jul 31, 2022

Like EIP-4907 but completely different.

@eth-bot eth-bot enabled auto-merge (squash) July 31, 2022 01:12
@Pandapip1 Pandapip1 changed the title EIP-4907 Alternative Design PR-5380: EIP-4907 Alternative Design Jul 31, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Jul 31, 2022

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

auto-merge was automatically disabled July 31, 2022 01:13

Head branch was pushed to by a user without write access

@github-actions
Copy link

The commit 9c8093f (as a parent of f335512) contains errors. Please inspect the Run Summary for details.

@eth-bot eth-bot enabled auto-merge (squash) July 31, 2022 01:14
@Pandapip1
Copy link
Member Author

file EIPS/eip-xxxx.md is not a valid filename, but this error has been ignored due to editor approvals

gulp I guess it's lucky there was an eipw error then...

EIPS/eip-5380.md Outdated Show resolved Hide resolved
EIPS/eip-5380.md Outdated Show resolved Hide resolved
EIPS/eip-5380.md Outdated Show resolved Hide resolved
auto-merge was automatically disabled July 31, 2022 12:12

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) July 31, 2022 12:12
auto-merge was automatically disabled July 31, 2022 12:14

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) July 31, 2022 12:15
auto-merge was automatically disabled July 31, 2022 12:17

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) July 31, 2022 12:18
@abcoathup
Copy link
Contributor

Does the title need to reference EIP4907? Could it just be: ERC-721 User and Expires extension

@TimDaub
Copy link
Contributor

TimDaub commented Aug 17, 2022

Hey sorry for taking suuuuper long to get to this. @Pandapip1 thanks so much for taking this on, it looks actually really interesting what you're doing here. I think it is in line with what has been outlined in the Decentralized Society paper, specifically this section:

Screenshot 2022-08-17 at 16 10 55

What do you think about listing a bunch of entitlement types and adding an enum-type structure that captures the entitlement that can be sent to a user?

References:

@Pandapip1
Copy link
Member Author

What do you think about listing a bunch of entitlement types and adding an enum-type structure that captures the entitlement that can be sent to a user?

Do you mean add a metadata extension? That does seem like a good idea.

EIPS/eip-5380.md Outdated Show resolved Hide resolved
EIPS/eip-5380.md Outdated Show resolved Hide resolved

## Backwards Compatibility

No backward compatibility issues were found.
Copy link
Contributor

@xinbenlv xinbenlv Aug 18, 2022

Choose a reason for hiding this comment

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

[Optional] If this EIP adds uint256 amount in the interfaces (amount be mandated to be 1 for EIP-721), it could have the benefit of supporting both EIP-721 and EIP-1155, potentially helpful for its adoption.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue here is supporting maxEntitlements. I initially was trying to do it in a cross-token manner, but it just didn't particularly work well. It's not clear who loses what when the balance changes without going to zero.

@Pandapip1 Pandapip1 changed the title PR-5380: EIP-721 Entitlement Extension EIP-5380: EIP-721 Entitlement Extension Aug 26, 2022
@TimDaub
Copy link
Contributor

TimDaub commented Aug 28, 2022

Do you mean add a metadata extension? That does seem like a good idea.

Yeah but preferably it's onchain and e.g. it allows someone to say: "I, address B, entitle address A to 'usus' Property p", where 'usus' is the entitlement and there can be many different ones all encoded in an enum.

@Pandapip1
Copy link
Member Author

Pandapip1 commented Aug 28, 2022

Hmm... what about being able to have entitlements extend from other entitlements (i.e. the default address for an entitlement extending from the usus entitlement is the person with the usus entitlement)?

@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels Aug 28, 2022
@Pandapip1 Pandapip1 changed the title EIP-5380: EIP-721 Entitlement Extension Add EIP-5380: EIP-721 Entitlement Extension Aug 28, 2022
EIPS/eip-5380.md Outdated Show resolved Hide resolved
EIPS/eip-5380.md Outdated Show resolved Hide resolved
}
```

`supportsInterface` MUST return true when called with `IERC5380`'s interface ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally I recommend including the interface id explicitly in the EIP. Helps to catch bugs when copying the interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally, I prefer keeping it this way while the EIP is in draft. When it moves to review I'd agree that it's probably best to explicitly state it.

}
```

`supportsInterface` MUST return true when called with `IERC5380Enumerable`'s interface ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: include the interface id.

Pandapip1 and others added 3 commits September 2, 2022 12:25
@TimDaub
Copy link
Contributor

TimDaub commented Sep 3, 2022

Hmm... what about being able to have entitlements extend from other entitlements (i.e. the default address for an entitlement extending from the usus entitlement is the person with the usus entitlement)?

I think this standards usefulnes can come from doing some research on the possible existing entitlements that exist, naming them appropriately and then recommending them as an enum:

  • usus
  • abusus
  • etc.

@eth-bot eth-bot enabled auto-merge (squash) September 3, 2022 20:08
@eth-bot eth-bot merged commit 0420779 into ethereum:master Sep 3, 2022
@Pandapip1 Pandapip1 deleted the patch-4 branch September 3, 2022 21:55
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* EIP-4907 Alternative Design

* Self-assign EIP-5380 and fix eipw errors

* I guess this is one way to do it

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

* Update EIPS/eip-5380.md

* Update EIPS/eip-5380.md

* Use title of previous EIP

* *poof* Becomes a different EIP

* Apply suggestions from code review

* Entitle -> EntitlementChanged

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

* Define TCG and add reason parameter

* Renter <-> owner

Co-authored-by: Sam Wilson <[email protected]>

* Grammatical fix

Co-authored-by: Sam Wilson <[email protected]>

* Remove reason

Co-authored-by: Micah Zoltu <[email protected]>
Co-authored-by: xinbenlv <[email protected]>
Co-authored-by: Sam Wilson <[email protected]>
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.

7 participants