mudgen commented 4 years ago

EIP-2535 Diamonds exists here: https://eips.ethereum.org/EIPS/eip-2535

Below is a feedback and discussion of the standard.

JulesGoddard commented 4 years ago

@mudgen Yes, I drew that diagram. You can just make out the datona logo in the background.

It is based on many similar diagrams I have drawn in the past for embedded system design.

But I was actually prompted by your diagram in https://dev.to/mudgen/understanding-diamonds-on-ethereum-1fb

That is very similar, it's just missing the diamond data (DataR in my diagram) and overall common data (DataX).

mudgen commented 4 years ago

@JulesGoddard Very cool.

sambacha commented 4 years ago

👏 @JulesGoddard great diagram thanks 📖

mudgen commented 4 years ago

I want to add new functions to the Diamond Standard reference implementation. If you want to contribute to an interesting project, here's your chance. See this issue: https://github.com/mudgen/Diamond/issues/7

JulesGoddard commented 4 years ago

I think that the Diamond Standard EIP should identify the function of Diamond Storage much earlier in the description. In my opinion, this should be a fundamental part of Diamond Standard and should be introduced in the simple summary, abstract and motivation. A group of facets (a side of a diamond?) can use data that is common to them but not available to the master diamond contract or to other facets.

mudgen commented 4 years ago

@JulesGoddard You are right. Thanks for reminding me about this. The reason Diamond Storage isn't as much a focus in the Diamond Standard is because the Diamond Standard was written before Diamond Storage existed, but that's no reason for the Diamond Standard not to be updated.

I plan to update the Diamond Standard EIP to give Diamond Storage a more central focus.

JulesGoddard commented 4 years ago

@mudgen I realised that. However, my concern is that without the common data/Diamond Storage feature, all contracts are only able to share the same data, and that is less likely to be a useful pattern. I think that the more generic, and probably much more useable, pattern is where groups of facets may use common data/Diamond Storage. That is why I was suggesting you reorganise your EIP (if you are permitted to) and accentuate the common data/Diamond Storage feature.

JulesGoddard commented 4 years ago

@mudgen Please consider this:

Simple Summary

A diamond is a set of contracts that can access the same storage variables and share the same Ethereum address. cf A diamond is a set of contracts that share the same Ethereum address and can access common storage variables as well as their own.

Abstract

... 6.
Facets can use their own data, as well as data common to multiple facets or to the whole diamond.

Why Make a Diamond?

... 6.
You develop features independently, which may need to communicate with other features. Use storage which is common between the contracts.

Etcetera.

mudgen commented 4 years ago

@JulesGoddard Thanks. I'm working on it slowly. I just added this description to the Abstract:

A way to create a set of contracts (facets) under a single address that are independent from each other but can share functionality and contract storage.

I'd like to figure out more ways for people to learn/understand diamonds and what they can do and achieve with them.

JulesGoddard commented 4 years ago

@mudgen Fair enough. I think that I know what you are intending to achieve.

The question is, how can we communicate the maximum amount of information with the minimum quantity of words? Very difficult. Especially when some words are particularly ambiguous or domain-specific.

In terms of Diamond Standard, it's reasonable to assume a fair knowledge of Ethereum: contracts have their own storage, libraries don't have storage.

Where you want to deviate from that norm, that's where you need to explain, in my humble opinion.

How can contracts 'share functionality?' Suggest worth explaining.

What about 'sharing contract storage?' Suggest introduce Diamond Storage (or common data) here.

mudgen commented 4 years ago

The question is, how can we communicate the maximum amount of information with the minimum quantity of words? Very difficult. Especially when some words are particularly ambiguous or domain-specific.

In terms of Diamond Standard, it's reasonable to assume a fair knowledge of Ethereum: contracts have their own storage, libraries don't have storage.

Where you want to deviate from that norm, that's where you need to explain, in my humble opinion.

How can contracts 'share functionality?' Suggest worth explaining.

What about 'sharing contract storage?' Suggest introduce Diamond Storage (or common data) here.

Exactly! A challenge. Even if I rewrite the Diamond Standard in a good way it won't be enough.

This is why I need your help, to explain these things in other ways, to break things down further, to give relatable analogies like memory used in embedded systems, more diagrams, examples of diamonds and their explanation, ways diamonds can be used, tutorials, videos that explain and show diamonds. I can add a supplementary section at/near the end of the standard to link to articles and material that give information about diamonds.

mudgen commented 4 years ago

There is something missing in the diamondCut function, which has been mentioned by @wighawag and @dOrgJelli. diamondCut has no way to execute arbitrary code in order to initialized contract storage for functions added/replaced/removed.

So I am thinking of changing diamondCut to give it another parameter: bytes memory _initialization which contains 0 bytes if there is no initialization and otherwise contains a contract address and additional bytes. The initialize(bytes calldata _data) function will be called with delegatecall on the contract address and the additional bytes will be passed in the _data argument.

Thoughts?

mudgen commented 4 years ago

@androolloyd @pi0neerpat @leonardoalt @spalladino @fulldecent @GNSPS @Droopy78 @wjmelements @nfurfaro @hiddentao @sambacha

I want to thank you guys for your comments and feedback about the Diamond Standard.

The Diamond Standard has changed significantly since you first read it. The standard is different and much of the standard has been rewritten.

I'd really appreciate it if you would read the NEW Diamond Standard with fresh eyes here: https://eips.ethereum.org/EIPS/eip-2535 As always, your feedback is helpful and appreciated.

I have tried to make diamonds more useful and easier to understand. Let me know if I succeeded.

I wrote a blog post that describes changes to the Diamond Standard here: https://dev.to/mudgen/update-what-s-new-in-the-diamond-standard-eip-fjk

Droopy78 commented 4 years ago

@mudgen Fresh eyes here. I really like the rewrite of the documentation. After first pass review here are some comments:

Upgrades and Immutability

I would separate out all three life-cycle cases for clarity. Suggested wording:

"A diamond can be immutable from inception by not adding any external functions that can add/replace/remove functions. The primary use case would be unlimited contract size.

A diamond that is mutable can be made immutable by removing such functions.

A diamond can be upgraded if has a diamondCut external function or other function(s) that can add/replace/remove functions."

Other comments

I like how you only bring up the facet and diamond terms in the first half of the write up, then wait until later to introduce loupe. Previously, I found it to be more confusing when all the terms were introduced at the onset.

In the "Facets and State Variables" section, it would be more clear to use the same naming in the example code as is used in the diagram in the following section showing DiamondStorage1, DiamondStorage2, etc, so that the reader can connect the code example with the diagram more easily (or maybe I didn't understand it on first pass).

General question: Are there specific scenarios where someone would want to create their own interface to add/remove/upgrade functions instead of using the DiamondCut function defined in the interface?

If there isn't a compelling case, I think it would make the standard and documentation more clear throughout to simply make DiamondCut mandatory instead of trying to be overly flexible. The add/remove/upgrade Event is already mandatory. It would make the Life Cycle descriptions I mentioned above more succinct as one example.

Also, as a counterpoint to the argument that DiamondCut is simply “used” and not mandatory in the immutable case: The constructor always needs DiamondCut even in the immutable diamond case. It is needed for the initial cut the diamond. Following that, yes it wouldn’t be added as a facet, but it was still a necessary function for the Diamond standard to work.

Thanks, and again great job on this!

MicahZoltu commented 4 years ago

Motivation and Abstract sections are missing, and there are a bunch of sections between the Simple Summary and the Specification that probably should be rolled up into one of the template sections.

I would really like to see a strong argument why this needs to be a standard rather than a best-practice or good-idea. In general, standards are useful when there is a many-to-many relationship between clients and providers. In this case, I do not see a clear many-to-many relationship.

As an example, object oriented programming, functional programming, and dependency injection are all design patterns/good ideas but none of them benefit from being standardized. On the other hand, browser plugins are standardized because there is a many-to-many relationship between browsers and extension developers.

MicahZoltu commented 4 years ago

I think I mentioned this somewhere else but I don't see it here, and figured it would be good to have a record of it. I recommend using a more intuitive naming convention rather than the clever "diamond, facet, loupe, etc." naming convention. New jargon should be introduced only when absolutely necessary as in raises the barrier to entry for new developers.

mudgen commented 4 years ago

@Droopy78

Thanks for the helpful feedback! Yea, your descriptions of upgrades/immutability are good. I added them to the standard.

Good idea about making the example code consistent with the diagrams. It's hard for me to do that for various reasons but I updated the standard with some code changes to make it more consistent. I didn't think of that, thank you.

The standard diamondCut function is general and designed for maximum flexibility so it can be used by all tools in lots of ways. People may want to have versions that are more specific. The diamond reference implementation uses a custom version of diamondCut in the constructor of the diamond to add functions. It is an internal function without the address _init, bytes calldata _calldata parameters.

@Droopy78 Thanks for your acknowledgement and help. I look forward to reading about your diamonds.

mudgen commented 4 years ago

@MicahZoltu It is true that the Diamond Standard encapsulates some good ideas and a pattern for developing flexible smart contract-based applications. And the EIP does discuss the pattern a lot and how it works. I'm glad you brought up reasons for standardization because I think it deserves more attention. And it is an area that people and developers can really help.

Standardization is needed to make the diamond pattern more useful and fulfill its design.

Specifically standardization is needed for these two things:

Transparency
Interoperability

Transparency

A key feature of the Diamond Standard is transparent upgrades on blockchains. The Diamond Standard used to be the Transparent Contract Standard. There is an interesting article about it that covers transparency.

A diamond provides at least two kinds of transparency:

Realtime transparency, using the diamond loupe.
Historical transparency using the DiamondCut event.

The diamond loupe enables people to see what functions and facets a diamond currently has. This is standardized so that tools can be made to show people what functions and facets a diamond has. There is a many to many relationship here. There can be many diamonds. There can be many (or multiple) client software programs that show diamonds. Standardizing the loupe functions makes this many to many relationship possible. Standarizing the loup functions encourages software to be developed that makes diamonds more transparent.

The DiamondCut event is emitted any time any function is added/replaced/removed on a diamond. This is standardized so that multiple software programs can be developed that can show all upgrades to all diamonds on a network, thus making diamond upgrades transparent.

Etherscan has expressed some interest in supporting diamonds and I think there will be more interest in creating diamond transparency tools and user interfaces as more projects use the Diamond Standard. More projects are using the Diamond Standard. I know of several projects in development that are using diamonds.

Interoperability

The diamondCut function adds/replaces/removes any number of functions on a diamond in a single transaction. This function is standardized so that multiple tools can be made to create diamonds and upgrade upgradeable diamonds. This enables someone with diamonds to migrate or use different tools, or have more choice of tools, for upgrading or creating diamonds.

A couple tools have been made or added support for creating, deploying and upgrading diamonds:

I expect more tools to be created.

wjmelements commented 4 years ago

What is good about this standard:

Works around contract size limit
Comparable overhead to other SLOAD+DELEGATECALL upgradeable proxies.
Standardization helps universal dapp services like etherscan expose the current ABI.

What is bad about it:

Naming: diamonds are hard, proxies are soft
Method lookup overhead in the top-level proxy

My preferred proxy pattern is the CREATE2 reincarnation, which has no proxy overhead, but doesn't handle the contract size limit.

I also suggest moving all diamond interface functions, including DiamondLoupe, to an initial facet, so that upgradeability can be upgraded or removed.

I also suggest finding a use for the top 12 bytes of the selectorToFacet mapping. It could be used to distinguish delegatecall from possible call or callcode facets, but there might be something even better. Otherwise it's fine to leave it unused because such bloat also impacts gas.

mudgen commented 4 years ago

@wjmelements Thank you for this great feedback!

My preferred proxy pattern is the CREATE2 reincarnation, which has no proxy overhead, but doesn't handle the contract size limit.

This is interesting, thank you. I'd like to implement diamonds by storing the function selectors and facet addresses in the bytecode of the diamond using CREATE2 and then finding which facet to use in a fallback function, perhaps using a binary search on the function selectors. All to avoid the SLOAD but keep the diamond pattern. This is something I plan to work on.

I also suggest moving all diamond interface functions, including DiamondLoupe, to an initial facet, so that upgradeability can be upgraded or removed.

Yes, this is done in the diamond reference implementation.

I also suggest finding a use for the top 12 bytes of the selectorToFacet mapping. It could be used to distinguish delegatecall from possible call or callcode facets, but there might be something even better. Otherwise it's fine to leave it unused because such bloat also impacts gas.

Yes, the 12 bytes in the diamond reference implementation are used to efficiently store information needed to keep track of function selectors that are returned by the loupe functions.

Interesting idea to let the user choose which call opcode to use for an external function. I'll chew on that.

Checkout the diamond reference implementation: https://github.com/mudgen/Diamond

Thanks!

JulesGoddard commented 4 years ago

Glad to see that the Diamond Standard is getting engagement from interested parties.

All this will help to improve it.

Regards,

Jules Goddard

On 27 Aug 2020, at 04:27, Nick Mudge notifications@github.com wrote:

@wjmelements https://github.com/wjmelements Thank you for this great feedback!

My preferred proxy pattern is the CREATE2 reincarnation, which has no proxy overhead, but doesn't handle the contract size limit.

This is interesting, thank you. I'd like to implement diamonds by storing the function selectors and facet addresses in the bytecode of the diamond using CREATE2 and then finding which facet to use in a fallback function, perhaps using a binary search on the function selectors. All to avoid the SLOAD but keep the diamond pattern. This is something I plan to work on.

I also suggest moving all diamond interface functions, including DiamondLoupe, to an initial facet, so that upgradeability can be upgraded or removed.

Yes, this is done in the diamond reference implementation.

I also suggest finding a use for the top 12 bytes of the selectorToFacet mapping. It could be used to distinguish delegatecall from possible call or callcode facets, but there might be something even better. Otherwise it's fine to leave it unused because such bloat also impacts gas.

Yes, the 12 bytes in the diamond reference implementation are used to efficiently store information needed to keep track of function selectors that are returned by the loupe functions.

Interesting idea to let the user choose which call opcode to use for an external function. I'll chew on that.

Checkout the diamond reference implementation: https://github.com/mudgen/Diamond https://github.com/mudgen/Diamond Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ethereum/EIPs/issues/2535#issuecomment-681321810, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOPZGXXQQUVCCZV7RDHQVF3SCXHDDANCNFSM4K3SHYQQ.

dguido commented 4 years ago

Hello all! I'm here from @trailofbits to let everyone know that we conducted a paid security review of this EIP for a client. Results are forthcoming, however, we felt the need to immediately advise others that they should avoid using this code.

mudgen commented 4 years ago

Hello all! I'm here from @trailofbits to let everyone know that we conducted a paid security review of this EIP for a client. Results are forthcoming, however, we felt the need to immediately advise others that they should avoid using this code.

This is unsurprising since Trail of Bits strongly advises against using upgradeable contract patterns, and particularly proxy contract patterns as can be found in their article about upgradeable contracts.

Trail of Bits also advises others not to use OpenZeppelin's proxy contracts.

mudgen commented 4 years ago

Right now when using a diamond from EIP-2535 Diamond Standard, and using the diamondCut function to remove functions from a diamond, should trying to remove a function that already does not exist cause the transaction to revert? Or should it be a noop and the transaction succeeds?

When trying to replace a function with the same function using the same facet, should the transaction succeed or should it be a noop and the transaction succeeds?

Upgrades should be done in a careful, intentional way, so it makes sense to me to revert in these cases. But if it does not revert then the diamondCut function is a little more flexible. What do you think?

Here's how it is currently written in EIP-2535 Diamond Standard:

The diamondCut function does nothing if a supplied function selector is already associated with the supplied facet address. The diamondCut function does nothing if a supplied function selector can’t be removed because it already does not exist.

I am thinking of changing this in the standard so that these cases revert. Please give me your feedback and reasoning.

wjmelements commented 4 years ago

I am thinking of changing this in the standard so that these cases revert. Please give me your feedback and reasoning.

Opposed because the suggested change would require an additional SLOAD on the prior value. If gas costs never changed you could use GAS to implement the behavior. Another reason to oppose is that the diamondCut function loses idempotence.

fulldecent commented 4 years ago

Gas and idempotence are out of scope

The intended use case is not specified (missing in Simple Summary, and EIP is woefully failing to meet correct EIP format). But I understand that the use case is a human administrator manually performing diamond upgrades.

In this case one-time SLOAD gas costs and idempotence are not a concern for administrators occasionally upgrading contracts.

Idempotence is bad

See also the ERC-721 discussion that led to the requirement that all token transfers will also specify the current token owner. Theoretically, this information is redundant (you could query it with ownerOf(tokenID). And even in this case, with token transferring happening frequently, and being initiated by computers in batch rather than humans occasionally, our consensus still found that the safety afforded by requiring this extra comparison outweighed the cost.

mudgen commented 4 years ago

Thanks @wjmelements !

The SLOAD on the prior value is done anyway. You are correct that with this change diamondCut loses its idempotence.

mudgen commented 4 years ago

@fulldecent Thank you for your input! Fully noted. Safety is really important. And I'd rather have security auditors pointing out less things.

mudgen commented 4 years ago

I'm also considering another change to the standard.

Currently the function signature for diamondCut is this:

    struct Facet {
        address facetAddress;
        bytes4[] functionSelectors;
    }
    /// @notice Add/replace/remove any number of functions and optionally execute
    ///         a function with delegatecall
    /// @param _diamondCut Contains the facet addresses and function selectors
    /// @param _init The address of the contract or facet to execute _calldata
    /// @param _calldata A function call, including function selector and arguments
    ///                  _calldata is executed with delegatecall on _init
    function diamondCut(
        Facet[] calldata _diamondCut,
        address _init,
        bytes calldata _calldata
    ) external;

I am thinking of changing it to this:

interface IDiamondCut {

    enum FacetCutAction {Add, Replace, Remove}

    struct FacetCut {
        address facetAddress;
        FacetCutAction action; 
        bytes4[] functionSelectors;
    }

    /// @notice Add/replace/remove any number of functions and optionally execute
    ///         a function with delegatecall
    /// @param _diamondCut Contains the facet addresses and function selectors
    /// @param _init The address of the contract or facet to execute _calldata
    /// @param _calldata A function call, including function selector and arguments
    ///                  _calldata is executed with delegatecall on _init
    function diamondCut(
        FacetCut[] calldata _diamondCut,
        address _init,
        bytes calldata _calldata
    ) external;

    event DiamondCut(FacetCut[] _diamondCut, address _init, bytes _calldata);
}

The change is adding replace to the struct.

I have two reasons for possibly making this change:

It makes it more explicit what you are doing. I think that contract upgrades should be explicit and intentional. Saying exactly what you are replacing and what you are adding makes it more explicit and intentional.
This prevents selector clash. Without this there is a very small possibility of trying to add a function whose signature hashes to the same 4 bytes as an existing function in a diamond, causing an unintentional replacement. With this change, trying to add a selector that already exists would revert.

The downside to making this change is that it slightly complicates the diamondCut function by adding something to it.

I appreciate any feedback on this change, for or against.

wjmelements commented 4 years ago

The SLOAD on the prior value is done anyway.

Why?

mudgen commented 4 years ago

@wjmelements The SLOAD on the prior value is done as a safety check. To make sure that a new function being added does not exist already.

JulesGoddard commented 4 years ago

if a diamondCut tries to remove a function that does not exist

That sounds like an error, although benign, to me.

if a diamondCut tries to tries to upgrade a function with a facet it is already using

That sounds benign, to me.

Jules

On 23 Sep 2020, at 23:17, Nick Mudge notifications@github.com wrote:

@everyone https://github.com/everyone I have a question: Right now if a diamondCut tries to remove a function that already does not exist or tries to upgrade a function with a facet it is already using it is noop and nothing happens and the transaction succeeds.

Should it continue to work like this or revert?

Upgrades should be done in a careful, intentional way, so it makes sense to me to revert in this case. But if it does not revert then the diamondCut function is a little more flexible. What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ethereum/EIPs/issues/2535#issuecomment-698000865, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOPZGXTKW46XRGFVYVRFLM3SHJXX3ANCNFSM4K3SHYQQ.

JulesGoddard commented 4 years ago

Would it make more sense to use different functions for the different operations, add, reface, remove?

Jules

On 24 Sep 2020, at 01:17, Nick Mudge notifications@github.com wrote:

I'm also considering another change to the standard.

Currently the function signature for diamondCut is this:
struct Facet {
    address facetAddress;
    bytes4[] functionSelectors;
}
/// @notice Add/replace/remove any number of functions and optionally execute
///         a function with delegatecall
/// @param _diamondCut Contains the facet addresses and function selectors
/// @param _init The address of the contract or facet to execute _calldata
/// @param _calldata A function call, including function selector and arguments
///                  _calldata is executed with delegatecall on _init
function diamondCut(
    Facet[] calldata _diamondCut,
    address _init,
    bytes calldata _calldata
) external;
I am thinking of changing it to this:
struct FacetCut {
    address facetAddress;
    bool replace;  // If true then replace selectors, otherwise add selectors
    bytes4[] functionSelectors;
}

/// @notice Add/replace/remove any number of functions and optionally execute
///         a function with delegatecall
/// @param _diamondCut Contains the facet addresses and function selectors
/// @param _init The address of the contract or facet to execute _calldata
/// @param _calldata A function call, including function selector and arguments
///                  _calldata is executed with delegatecall on _init
function diamondCut(
    FacetCut[] calldata _diamondCut,
    address _init,
    bytes calldata _calldata
) external;
The change is adding replace to the struct.

I have two reasons for possibly making this change:

It makes it more explicit what you are doing. I think that contract upgrades should be explicit and intentional. Saying exactly what you are replacing and what you are adding makes it more explicit and intentional.

This prevents selector clash. Without this there is a very small possibility of trying to add a function whose signature hashes to the same 4 bytes as an existing function in a diamond. With this change, this selector clash is not possible.

The downside to making this change is that it slightly complicates the diamondCut function by adding something to it.

I appreciate any feedback on this change, for or against.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ethereum/EIPs/issues/2535#issuecomment-698037838, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOPZGXVQ5TMJZDGGOYBPOBDSHKFYDANCNFSM4K3SHYQQ.

mudgen commented 4 years ago

if a diamondCut tries to remove a function that does not exist

That sounds like an error, although benign, to me.

if a diamondCut tries to tries to upgrade a function with a facet it is already using

That sounds benign, to me. Jules

@JulesGoddard You are correct, they are benign.

mudgen commented 4 years ago

Would it make more sense to use different functions for the different operations, add, reface, remove? Jules

No. Upgrades need to be atomic or else storage data corruption is possible. For example if new functionality depended on adding 1 function and replacing 1 function then those two changes need to occur in the same transaction. Otherwise it is possible that a transaction is sent inbetween one transaction to add and one transaction to replace.

JulesGoddard commented 4 years ago

That is a good point.

Would the replace flag you propose apply to all the selectors being replaced?

Is that the same as having a separate function to replace selectors?

Jules

On 29 Sep 2020, at 15:35, Nick Mudge notifications@github.com wrote:

Would it make more sense to use different functions for the different operations, add, reface, remove? Jules

No. Upgrades need to be atomic or else storage data corruption is possible. For example if new functionality depended on adding 1 function and replacing 1 function then those two changes need to occur in the same transaction. Otherwise it is possible that a transaction is sent inbetween one transaction to add and one transaction to replace.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ethereum/EIPs/issues/2535#issuecomment-700748516, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOPZGXWKB4YHV6MR3T75TMTSIHWEBANCNFSM4K3SHYQQ.

fulldecent commented 4 years ago

Embrace zero. No need to make things unnecessarily complicated.

interface IDiamondCut {

    struct FacetCut {
        bytes4 functionSelector;
        address oldProxyTarget; // can be zero for "no proxy"
        address newProxyTarget; // can be zero for "no proxy"
    }

    /// @notice Add/replace/remove any number of functions and optionally execute
    ///         a function with delegatecall
    /// @param _facetCuts Details every change being made
    /// @param _initializationContract The address of the contract or facet to execute _initializationCalldata
    /// @param _initializationCalldata A function call, including function selector and arguments
    ///                  _calldata is executed with delegatecall on _init
    /// @apiSpec Function throws if a specified oldProxyTarget does not match the actual current proxy target
    function diamondCut(
        FacetCut[] calldata _facetCuts,
        address _initializationContract,
        bytes calldata _initializationCalldata
    ) external;

    event DiamondCut(FacetCut[] _facetCuts, address _initializationContract, bytes _initializationCalldata);
}

mudgen commented 4 years ago

Would the replace flag you propose apply to all the selectors being replaced?

Is that the same as having a separate function to replace selectors?

Yes.

The diamondCut function takes an array of FacetCut structs.

I updated EIP-Diamond Standard with the following:

To add new functions create a FacetCut struct with facetAddress set to the facet that has the new functions and functionSelectors set with the function selectors to add. Set the action enum to Add.

To replace functions create a FacetCut struct with facetAddress set to the facet that has the replacement functions and functionSelectors set with the function selectors to replace. Set the action enum to Replace.

To remove functions create a FacetCut struct with facetAddress set to address(0) and functionSelectors set with the function selectors to remove. Set the action enum to Remove.

JulesGoddard commented 4 years ago

That looks great.

On 29 Sep 2020, at 16:21, Nick Mudge notifications@github.com wrote:

Would the replace flag you propose apply to all the selectors being replaced?

Is that the same as having a separate function to replace selectors?

Yes.

The diamondCut function takes an array of FacetCut structs.

I updated EIP-Diamond Standard with the following:

To add new functions create a FacetCut struct with facetAddress set to the facet that has the new functions and functionSelectors set with the function selectors to add. Set the action enum to Add.

To replace functions create a FacetCut struct with facetAddress set to the facet that has the replacement functions and functionSelectors set with the function selectors to replace. Set the action enum to Replace.

To remove functions create a FacetCut struct with facetAddress set to address(0) and functionSelectors set with the function selectors to remove. Set the action enum to Remove.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ethereum/EIPs/issues/2535#issuecomment-700779258, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOPZGXSUUU5RVLRKJ67HNGTSIH3PJANCNFSM4K3SHYQQ.

mudgen commented 4 years ago

That looks great.

Great!

@fulldecent Thank you for your proposed design of FacetCut. Your design is simple and I like it. I appreciate it. But I'm not changing the current design because I think it has similar complexity to the design you proposed and some people have already started using the current design.

elenadimitrova commented 4 years ago

Leaving a note on this claiming to solve the issue with maximum contract size. Really any contract that uses an underlying proxy that looks up function signatures for respective implementations is already solving that problem. I spoke about this at the first TruffleCon in 2018 but it is an existing idea used by many and note really owned by anyone in particular.

mudgen commented 4 years ago

@elenadimitrova EIP-2535 does not claim to own the idea, it provides a standardized way to use it. I'm sorry if it gives that kind of impression and I am open to changing the text to make it better. It does reference older implementations of the idea in the Inspiration & Development section.

Diamond storage is new and the modularity made possible by it is new, since it wasn't possible until 10 March 2020.

That's great that you spoke about the technique to look up function signatures for respective implementations in 2018. I would like to share that kind of material if possible.

mudgen commented 4 years ago

Should diamonds have loupe functions?

I wrote an article about it here: https://dev.to/mudgen/why-loupe-functions-for-diamonds-1kc3

adklempner commented 3 years ago

Hello all! I'm here from @trailofbits to let everyone know that we conducted a paid security review of this EIP for a client. Results are forthcoming, however, we felt the need to immediately advise others that they should avoid using this code.

@dguido I'm very interested in this review (and I'm sure others are as well), when can we expect to read about this?

fulldecent commented 3 years ago

Currently, the words "storage" and "data" are used interchangeably across:

EIP documentation text
Variable names in documentation and reference implementation
Graphics in EIP documentation

For clarity, I request that only one of the terms is used.

On a separate note, I request that there be a base contract class for a "Storage" (or "Data") contract. Benefits include:

Separation of concerns—the storage may not change after upgrades, so this is beneficial to isolate the storage that will be accessed across old and new facets
Expressivity—the implementation can better document why storage is using a struct and a assembly (!). As current, an implementation should always prefix every assembly usage with:

This library, although very large, is an implementation of the "Storage" concept in the Diamond Contract standard. You will need to read the entire Diamond specification before reading this large library file in order to properly understand the context of all the functions and data layout here.

fulldecent commented 3 years ago

Use clear language

Currently, every contract every deployed to Ethereum is a valid EIP-2535: Diamond Standard contract.

This is because the specification does not actually have any requirements.

Please study RFC 2119 and see how this is used in ERC-721 to make a specification that actually specifies things.

fulldecent commented 3 years ago

Add motivation

Please see EIP-1.

Diamond Standard is complicated. And it is not a valid specification. At the moment this is a fatal flaw. Further, nobody can make sense of it because there is no project scope (i.e. "Motivation").

Please add a motivation section as per EIP-1, so that people can understanding what you are thinking so that contributions can be accepted to fix the things that are broken.

fulldecent commented 3 years ago

Factor out loupe requirement

The loupe interface may or may not actually be required (Diamond Standard actually does not have any requirements, see above).

But all of the reference implementations support loupe.

The loupe specification (assuming I understand what you mean, not what is written) requires a whole lot of overhead in all diamond contracts which provide no benefit towards the stated goal... I am GUESSING the goal of Diamond Standard is to make it easy to upgrade contracts. (I could make a better argument here if the goal of the project was stated, see MOTIVATION notes above.)

(Weakly) therefore, loupe should be removed or factored out from the main specification because it is complicated and does not support the goal. Also, the reference implementations should include an example ("THE" example) where a loupe and the extra complexity this requires is not in it.

fulldecent commented 3 years ago

`diamontCut` not specified

This function is not specified. Multiple implementations are possible which follow the specification (I'm using that word loosely, see above). And the results would be catastrophic.

mudgen commented 3 years ago

@fulldecent Thanks for these points. I agree that EIP-2535 Diamond Standard need some informational editing and specifically the motivation section.

The Specification section is meant to be the specification. The diamondCut function is specified in the specification section. In a couple places in the standard the standard says that a diamond must implement the Specification section.

Can you be more specific about what would be catastrophic?

The loupe functions are required. The new Motivation section provides information about why they exists and their use. Also, there is this article about the loupe functions: https://dev.to/mudgen/why-loupe-functions-for-diamonds-1kc3

ethereum / EIPs

EIP-2535: Diamonds #2535

Transparency

Interoperability

Gas and idempotence are out of scope

Idempotence is bad

Use clear language

Add motivation

Factor out loupe requirement

`diamontCut` not specified

ethereum / EIPs

EIP-2535: Diamonds #2535

Transparency

Interoperability

Gas and idempotence are out of scope

Idempotence is bad

Use clear language

Add motivation

Factor out loupe requirement

diamontCut not specified

`diamontCut` not specified