Update EIP-6617: Move to Review #6775
Conversation
github-actions
bot
added
c-status
|
✅ All reviewers have approved. |
|
The commit 72583f2 (as a parent of b8b13e0) contains errors. |
EIPS/eip-6617.md
Outdated
| @title EIP-6617 Bit Based Permission | ||
| @dev See https://eips.ethereum.org/EIPS/eip-6617 | ||
| */ | ||
| library EIP6617 { |
There was a problem hiding this comment.
I think my last complaint before merging as draft is that this should be an interface.
There was a problem hiding this comment.
This part is a library that focused on bit manipulation that allow us to compose/discompose an uint256 permission. In my opinion, it shouldn't be an interface. We have no benefit by implement it as an interface.
There was a problem hiding this comment.
A library that implements bit manipulation doesn't need to be an EIP. It can live on GitHub, as an npm package, or in OpenZeppelin. You can have multiple different bit manipulation libraries that make different choices in implementation.
The EIP should only describe externally visible functionality, and in the case of Solidity that usually means just interfaces.
|
The provided Reference Implementation doesn't match the spec, I don't think at this version it satisfied the editorial requirement to merge for Review status. |
| /** | ||
| * @dev Implement the metadata of EIP-6617 | ||
| */ | ||
| contract EIP6617Meta is IEIP6617Meta { |
There was a problem hiding this comment.
This implementation doesn't implement what is being specified.
There was a problem hiding this comment.
Hi @xinbenlv, we will review then update the EIP to match the requirements. Could you be more specific so we can update all at once?.
EIPS/eip-6617.md
Outdated
| ## Specification | ||
|
|
||
| The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119 and RFC 8174. | ||
|
|
||
| *Note* The following specifications use syntax from Solidity `0.8.7` (or above) | ||
| _Note_ The following specifications use syntax from Solidity `0.8.7` (or above) | ||
|
|
||
| - Permission and role MUST be defined as an `uint256` |
There was a problem hiding this comment.
I appreciate author's effort to propose and authoring this EIP, folks. Hope this help explain it better about what kind of improvement I personally would like to see in this EIP before it can be merged as a Review
The specification of EIP at this current snapshot is far from editorially ready.
For example, this spec section begins with - Permission MUST be defined as a power of two
This is confusing. Is it a requirement for a complying contract to "have the methods of Permission" ? If so, it shall establish a requirement for certain methods.
For example
A compliant contract MUST have the following interface...
which contains methods about Permissions
And only then, it could begin to describe the requirement of each method, such as "Permission MUST be defined as a power of two." which is depending on a establishment of Permissions.
Also, next question: what does "Permission MUST be defined as power of two" means, exactly?
Maybe I guess you mean that Permission is bit-wise represented so each Permission is defined as one bit in a bytes32/uint256. etc. If so, the implementation shall refer to these requirements shows how a typical example of permission is implemented and how each methods will interact with it.
etc.
You could probably catch these editorial problem by asking a smart contract developer friend to read it and see if they could implement it by reading your ERC. An ERC/EIP's editorial readiness is all about "does it clearly talk about what a standard requires?" If you don't a good community to ask for this kind of help. Here is one resource: https://github.com/ercref/AllERCDevs/ a regular meetup for live tech peer feedback for ERC authors and devs. Feel free to join us. Next will be 5hours later today, and we do it bi-weekly.
Hey, @SamWilsn FYI, you wrote "I think my last complaint before merging as draft is that this should be an interface." in June, just in case you didn't see my complaints :)
There was a problem hiding this comment.
Thanks for the comments @xinbenlv,
I tried to apply all the requests everyone made, tell me if it's better like that 😄
|
I shared it here:
https://github.com/ethereum/EIPs/pull/6775/files#r1287470281
…On Mon, Aug 7, 2023 at 11:09 PM Chiro Hiro ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In assets/eip-6617/contracts/EIP6617Meta.sol
<#6775 (comment)>:
> @@ -0,0 +1,72 @@
+// SPDX-License-Identifier: CC0-1.0
+pragma solidity ^0.8.7;
+
+import "./interfaces/IEIP6617Meta.sol";
+
+/**
+ * @dev Implement the metadata of EIP-6617
+ */
+contract EIP6617Meta is IEIP6617Meta {
Hi @xinbenlv <https://github.com/xinbenlv>, we will review then update
the EIP to match the requirements. Could you be more specific so we can
update all at once?.
—
Reply to this email directly, view it on GitHub
<#6775 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4KRLJOZYEFHDCPYQPO23XUHJYDANCNFSM6AAAAAAWGUUA7Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
No description provided.