Update EIP-6224: Move to Review #6911
Conversation
github-actions
bot
added
c-status
File
|
|
The commit e8098b1 (as a parent of 881e052) contains errors. |
Arvolear
force-pushed
the
master
branch
2 times, most recently
from
5c49bd2
to
4f48e03
Compare
| * @param name the name of the contract | ||
| * @param contractAddress the address of the added contract | ||
| */ | ||
| event ContractAdded(string name, address contractAddress); |
There was a problem hiding this comment.
I think these should be indexed.
There was a problem hiding this comment.
This makes sense, however there might be a catch with long strings that do not fit into 32 bytes index and will be hashed.
|
@Pandapip1 @xinbenlv hey, could you please take a look at the PR? |
There was a problem hiding this comment.
Couple things I can't comment directly on:
- The abstract should contain a terse technical summary of how the proposal achieves its goal. Importantly, your abstract doesn't mention anything about dependencies, which is in your title.
- "In the ever growing Ethereum" is a bit weirdly worded, maybe: "In the ever growing Ethereum ecosystem".
- Abbreviations need to be expanded the first time they are used (i.e.
UUPS.) - Please remove the Reference Implementation section, or include an implementation inline or in the assets directory.
| @@ -204,7 +221,7 @@ There are a few design decisions that have to be specified explicitly: | |||
|
|
|||
| #### Usage | |||
|
|
|||
| The extensions of this EIP SHOULD add proper access control checks to the described non-view methods. | |||
| The extensions of this EIP SHOULD add proper access control checks to the described non-view methods. | |||
|
|
|||
| The `getContract` and `getImplementation` methods MUST revert if the nonexistent contracts are queried. | |||
There was a problem hiding this comment.
This paragraph, and the following one, introduce new requirements. The rationale section should only explain choices made within the proposal, while any requirements have to be defined in the specification section. In other words, SHOULD, MAY, MUST, etc. can't appear in the rationale.
| @@ -234,15 +251,15 @@ The injector address MAY be stored in the dedicated slot `0x3d1f25f1ac447e55e7fe | |||
|
|
|||
| ## Reference Implementation | |||
|
|
|||
| *0xdistributedlab-solidity-library dev-modules* provides a reference implementation. | |||
| _0xdistributedlab-solidity-library dev-modules_ provides a reference implementation. | |||
There was a problem hiding this comment.
Can you share where this is provided?
| @@ -4,7 +4,7 @@ title: Contracts Dependencies Registry | |||
| description: An interface for managing smart contracts with their dependencies. | |||
| author: Artem Chystiakov (@arvolear) | |||
| discussions-to: https://ethereum-magicians.org/t/eip-6224-contracts-dependencies-registry/12316 | |||
| status: Draft | |||
| status: Review | |||
| type: Standards Track | |||
| category: ERC | |||
| created: 2022-12-27 | |||
There was a problem hiding this comment.
This comment goes to next line:
requires: 1967, 5750
Maybe I am missing something here.
It seems I couldn't find the mentioning of 1967 in this EIP. This EIP also seemed lack mentioning the requirement of a proxy. If a proxy is a ERC-1967 proxy, it doesn't need any specific function to retrieve the impl address because you can get it from ERC-1967.
|
This ERC draft seems not editorial clear enough to merge as a Review status. |
|
I've rebased this, but you'll still need to address the issues raised by myself and xinbenlv. |
|
I am closing this pull request because we are in the process of separating EIPs and ERCs into distinct repositories. Unfortunately, as far as we are aware, GitHub does not provide any tools to ease this migration, so every pull request will need to be re-opened manually. As this is a PR to create / modify an ERC, I will kindly ask you to redirect this to the new repository at ethereum/ERCs. We have prepared a guide to help with the process. If there is relevant history here, please link to this PR from the new pull request. On behalf of the EIP Editors, I apologize for this inconvenience. |
Adding several modifications (with linting fixes) to EIP as proposed by fellow magicians. Also moving the EIP to Review as no breaking changes are foreseen.