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 ERC-6956: Move to Review #43

Merged
merged 6 commits into from
Dec 11, 2023

Conversation

tbergmueller
Copy link
Contributor

Corresponds to ethereum/EIPs#7903

The EIP is implemented and used for several months now. As far as we know mainly for customers of ours though and projects we're involved in.

Nonetheless, the draft can be considered stable and has also been reviewed by members of https://www.swissdao.space/
In this review no major or critical technical issues or vulnerabilites have been raised, but it has been recommended to simplify the EIP.

In review stage we intend to discuss the possibility of making the anchor <> tokenId mapping optional, which would reduce complexity and also reduce gas-fees.

Besides Events, a backwards-compatible solution without this mapping should be possible enabling already existing contracts to still be supported. Not strictly in the ERC-165 sense, but uint256 (tokenId) and bytes32 (anchor) can technically be interchanged. This is possible by "just" using anchors as tokenIDs.

This merge-request:

Removes HTML-TODO comments
Removes links to stagnant- and draft ERCs (replaced one in related work with backticks, since I think it is still worth noting this draft exists). Notified authors in the respective discussion threads to ping me if their proposal is moving forward
Added ERC-5192 as recommendation for lockable()

@tbergmueller
Copy link
Contributor Author

tbergmueller commented Oct 27, 2023

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Oct 31, 2023

✅ All reviewers have approved.

@eip-review-bot eip-review-bot changed the title Prepare for and move ERC-6956 to Review Update ERC-6956: Move to Review Oct 31, 2023
Corresponds to ethereum/EIPs#7903

- Remove links to draft/stagnant EIPs
- Add recommendation for lockable() interface
- Move to Review
Copy link
Collaborator

@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.

You should move the "minimal typescript example to generate an ATTESTATION" into the reference implementation section.

The use cases in your rationale section seem to focus on justifying the EIP as a whole, and so should appear in your motivation section. The rationale section should explain technical choices made within the EIP while the motivation is the place to justify the whole proposal.

ERCS/erc-6956.md Outdated Show resolved Hide resolved
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

Copy link

The commit 300efa3 (as a parent of 8f88b63) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot removed the w-stale label Nov 30, 2023
To make HTMLProofer happy
@github-actions github-actions bot removed the w-ci label Dec 10, 2023
@eip-review-bot eip-review-bot enabled auto-merge (squash) December 11, 2023 21:48
Copy link
Collaborator

@eip-review-bot eip-review-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...

@eip-review-bot eip-review-bot merged commit 56251b5 into ethereum:master Dec 11, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants