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

Diamonds #2535

Open
mudgen opened this issue Feb 25, 2020 · 166 comments
Open

Diamonds #2535

mudgen opened this issue Feb 25, 2020 · 166 comments

Comments

@mudgen
Copy link
Contributor

@mudgen mudgen commented Feb 25, 2020

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

Below is a feedback and discussion of the standard.

@androolloyd
Copy link

@androolloyd androolloyd commented Feb 26, 2020

This is great.

Would be curious to see a uniswap build using the Diamond pattern.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 26, 2020

@androolloyd yes, me too!

@pi0neerpat
Copy link
Contributor

@pi0neerpat pi0neerpat commented Feb 26, 2020

I think you need to add the proper table to the top of this file. Wait, this is an issue not a PR. I'm confused are you trying to make a new EIP?

(Putting discussion here since I don't see an external link)

This looks interesting, albeit very ahead of it's time. You've clearly put a lot of thought into it, and it is very well written. I'm curious to see how this intersects with governance. My jerk reaction is that this is only useful in a specific application that includes permission-based "cut" control. But even that would introduce lots of systemic risk. For now I think we are all stuck with upgrades being controlled by a single source of control, like Maker and rDAI, which publish documentation to record the history of changes. I look forward to the day where this fine-grained control is reality, and small changes can be made on-the-fly without introducing additional risk!

Am I missing another use-case for this?

Edit: (follow-up) how can we isolate storage on the same permissioned system that we use to allow cuts?

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 26, 2020

@pi0neerpat thanks for your kind comments! The discussion is here. An official EIP for this standard is coming soon. It will be EIP 2535.

Companies are already using this kind of architecture (ERC1538) for their contracts, such as Caesar's Triumph, Enjin and others. It is real and in use today.

Enjin's NFT standard EIP 1155 recommends using an architecture like ERC1538 for upgrades.

I don't know why you think a diamond can't be controlled by a single source. The reference implementation of the standard is implemented that way -- only the single owner of the diamond can make changes. Maybe I am understanding you wrong? Authentication is not part of the standard, it can be fine grained or not.

A big use case is contracts with designs that exceed the max size of contracts, since diamonds don't have a max size. It is also very nice that while diamonds can be large their functionality can still be compartmented by facets. Particularly NFT contracts tend to exceed the max size limit, such as implementations of ERC721 and ERC998 and others that implement an NFT but also need to implement custom functionality for the application.

Permissions and authentication for cuts and access to storage variables can be handled in the same way as any other contract. Yes, if you wanted to, you could add various different permissions/authentications for different changes and handling different storage variables. Or you could keep it simple and not do that.

@mudgen mudgen changed the title Diamond Contract Standard Diamond Standard Feb 26, 2020
@leonardoalt
Copy link
Member

@leonardoalt leonardoalt commented Feb 26, 2020

The owners(s) of an upgradeable contract have the ability to alter, add or remove data from the contract's data storage. Owner(s) of a contract can also execute any arbitrary code in the contract on behalf of any address. Owners(s) can do these things by adding a function to the contract that they call to execute arbitrary code. This is an issue for upgradeable contracts in general and is not specific to diamonds.

Well, exactly. How is this standard any different from the centralized owned upgradeable smart contracts out there? Why not a standard that abstracts upgrades being opt-in only by default?

@leonardoalt
Copy link
Member

@leonardoalt leonardoalt commented Feb 26, 2020

This is great.

Would be curious to see a uniswap build using the Diamond pattern.

To me Uniswap is great exactly because it's not upgradeable/centralized.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 26, 2020

@leonardoalt It is different in a lot of ways, but it is not different in regards to ownership/authentication. Ownership/authentication is not part of this standard. The ownership/authentication can be implemented in a diamond any way anybody wants.

I realize now that the way EIP-2535 Diamonds is currently written it misleads people into thinking that EIP-2535 Diamonds specifies how authentication/ownership is implemented or should work. It doesn't. So I think I will change the text you quoted because it is misleading. It certainly does not have to be that way, it just could be that way, depending on how ownership/authentication is implemented.

But note that the standard does suggest a different way to do ownership or authentication. See the "Decentralized Authority" section.

Also, the upgrade functionality can be removed in a diamond making a diamond immutable. This could be done by removing the cut function. And it possibly could be added back depending on certain cases dictated by the implementation of the diamond -- there are so many possibilities to what could be implemented and EIP-2535 Diamonds does not limit what can be done.

Why not a standard that abstracts upgrades being opt-in only by default?

Standards or tutorials or implementations that build on top of EIP-2535 Diamonds to provide different ownership/authentication/upgrade schemes are very much wanted!

EIP-2535 Diamonds provides basic architecture and structure and points of interoperability with user interfaces and software, and it leaves up to the implementer what the diamond does and how it works.

@pi0neerpat
Copy link
Contributor

@pi0neerpat pi0neerpat commented Feb 26, 2020

@mudgen

I don't know why you think a diamond can't be controlled by a single source.

The only reason I say this is because the single source would just publish documentation whenever they made a change. There's no real reason to emitt changes as events or capture this on-chain. Maker and other protocols already have governance processes to document this already.

A big use case is contracts with designs that exceed the max size of contracts, since diamonds don't have a max size.

That's a very good feature!

Companies are already using this kind of architecture (ERC1538) for their contracts, such as Caesar's Triumph, Enjin and others. It is real and in use today

I'll have to look more closely at these. This could be really useful for @austintgriffith DAOG game where the rules of the game are updated on-the-fly. Right now the rules are limited in scope, but this could allow more open-ended rules to be added or removed.

Permissions and authentication for cuts and access to storage variables can be handled in the same way as any other contract.

I'm glad you think this is possible because I really think this is the best potential use-case for this system. For instance, in the DAOG game, you could allow players to add/remove rules, without letting them interfere with the core game logic (i.e. the logic and storage deciding who wins and can withdraw the prize pot). Or with a Moloch, you could set certain thresholds for high/low impact changes to the dao contract itself. Dao members could pass smaller rules more easily, to run segregated and quick experiments, without risking the dao's funds.

@leonardoalt
Copy link
Member

@leonardoalt leonardoalt commented Feb 26, 2020

the upgrade functionality can be removed in a diamond making a diamond immutable.

The upgrade functionality can also be removed by the contract not being upgradeable in the first place.

there are so many possibilities to what could be implemented and the diamond standard does not limit what can be done

Exactly my point. It is basically a really complicated way to make contracts as mutable as simply delegating everything to a dynamic address, which is actually a lot more transparent and simpler to read and check what it's doing. Complicated upgrade standards enable backdoors by obfuscation, especially when you can literally change everything.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 26, 2020

@leonardoalt Its is really not very complicated. It is new and the standard does provide a lot of information to help use and implement the standard.

EIP-2535 Diamonds is this

  1. One function for adding/replacing/removing functions.
  2. One event for logging changes.
  3. Some functions for showing what functions the contract has.

A big part of the standard is transparency (logging changes), which removes obfuscation. I plan to make a user interface that shows and visualizes all changes to a diamond.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 26, 2020

Some good discussion of EIP-2535 Diamonds here: https://ethereum-magicians.org/t/diamond-contract-standard/4038

@spalladino
Copy link
Contributor

@spalladino spalladino commented Feb 27, 2020

Awesome work @mudgen!! I like this new version of the EIP, and have a few comments:

  • I really like the fact that cut can modify several functions atomically, and you don't need multiple calls, potentially leaving your contract in an inconsistent state. That said, I'd look into simplifying it a bit: instead of having the concept of add/remove/replace action, why not just have cut accept an array of pairs selector,implementation, directly setting the implementation for each selector? Add and replace would be similar, and removal can be done by setting implementation to zero. Function clashes can be managed via appropriate tooling here.

  • In line with the previous one, I'd also simplify the event. Having a big event with a bunch of actions is harder to consume. I'd just set up events with the selector and implementation arguments, and fire as many as needed. You could even make selector indexed, so clients can monitor for changes to a specific function (I'd look into gas tradeoffs here though).

  • There is another potential clash that is event clashing. The DiamondCut event could have the same selector as an event from a facet, potentially fooling a client who is listening to these events. That said, a client could just query the contract to verify the facet address if needed.

  • I'd also simplify the loupe interface. I'd remove any enumerable features, and just keep a getFacet(bytes4 selector) method. I'm not sure in which situations another contract would need to iterate over the facets of a diamond, and an offchain client could look into past events. I think this should also simplify the storage structures needed to support a diamond, making it overall more gas efficient.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 27, 2020

@spalladino Thank you very much. This is great feedback! I appreciate that you take the time to look at this and write this feedback.

Awesome work @mudgen!! I like this new version of the EIP, and have a few comments:

  • I really like the fact that cut can modify several functions atomically, and you don't need multiple calls, potentially leaving your contract in an inconsistent state. That said, I'd look into simplifying it a bit: instead of having the concept of add/remove/replace action, why not just have cut accept an array of pairs selector,implementation, directly setting the implementation for each selector? Add and replace would be similar, and removal can be done by setting implementation to zero. Function clashes can be managed via appropriate tooling here.

I like the idea of simplifying the argument to cut. Your suggestion omits the string message part of the argument that describes a set of changes. It is a commit message. Removing it would simplify the argument to cut and reduce some gas. Do you think its disadvantages outweigh its benefits? The argument is as complicated as it currently is because it gives the ability to have different change messages for different sets of changes in one transaction. This helps document the changes, but maybe it isn't worth the added complexity of the data structure.

  • In line with the previous one, I'd also simplify the event. Having a big event with a bunch of actions is harder to consume. I'd just set up events with the selector and implementation arguments, and fire as many as needed. You could even make selector indexed, so clients can monitor for changes to a specific function (I'd look into gas tradeoffs here though).

The argument to the cut function and the argument to the DiamondCuts event is exactly the same. The cut function literally passes its argument directly to the DiamondCuts event without any manipulation. This is a nice simplicity. You are right, it would be nicer if it was a simpler data structure. It associates the change messages with their changes. Again, if we remove the string message then this data structure could be simplified. So I guess the important question is how important is it to have descriptive text describing changes to a diamond emitted in events? I suppose the datastructure could also be simplified some by only allowing one change message for all changes in one transaction. With the way it is now you can have multiple change messages for different sets of changes in one transaction. That is why the data structure is as complicated as it is.

  • There is another potential clash that is event clashing. The DiamondCut event could have the same selector as an event from a facet, potentially fooling a client who is listening to these events. That said, a client could just query the contract to verify the facet address if needed.

Can you explain this more? I don't understand the scenario you are describing here.

  • I'd also simplify the loupe interface. I'd remove any enumerable features, and just keep a getFacet(bytes4 selector) method. I'm not sure in which situations another contract would need to iterate over the facets of a diamond, and an offchain client could look into past events. I think this should also simplify the storage structures needed to support a diamond, making it overall more gas efficient.

Wow, that is an interesting idea. I see what you are saying, the loupe isn't needed if software can simply look at the events to determine which functions exist. I'm not sure why getFacet(bytes4 selector) would be needed by the standard. The main purpose of the loupe is to provide interoperability with user interface software and tooling, but if events already give that then they could be used. People can always add in their own functions like getFacet(bytes4 selector) for their own purposes. I am considering removing the loupe completely from the standard.

@spalladino What do you think now?

@spalladino
Copy link
Contributor

@spalladino spalladino commented Feb 27, 2020

The commit message is definitely interesting, but shouldn't it be handled off chain? Perhaps the contract should just have a small identifier that points to an actual commit or release off-chain, for traceability from the source to the deployment?

Still, one option (following the address,selector pairs approach) would be for the cut event to publish the array of address,selector pairs, and the commit message, similar to what you originally proposed - though easier to parse for the client. You lose indexability of selectors, but it may not be too critical.

Can you explain this more? I don't understand the scenario you are describing here.

Sorry, forget about this one. I mixed up event topics and function selectors. The first topic for an event, which is derived from a hash of its name and args and identifies the type of event, is 32 bytes long -not 4 bytes like a function selector. So clashes between event names are not possible.

People can always add in their own functions like getFacet(bytes4 selector) for their own purposes. I am considering removing the loupe completely from the standard.

Maybe there is an opportunity to have this standard be automatically ERC165 compliant...? Haven't looked at it in-depth.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 27, 2020

The commit message is definitely interesting, but shouldn't it be handled off chain? Perhaps the contract should just have a small identifier that points to an actual commit or release off-chain, for traceability from the source to the deployment?

Have the identifier where? The commit messages are not stored in the contracts, they are just emitted with the event that shows the changes. I'd rather just emit the commit message in the event than emit a hash of a commit stored somewhere because I don't think this would be useful for user interfaces that show people all the changes to a diamond, but the commit messages describing the changes could be useful. The idea of the user interface is that it would pull all the verified source code from somewhere like etherscan so people could easily see all the source code of all the facets used by a diamond, and in addition people could see the verified source code of how a diamond was in the past if it was cut. And people would see the commit messages describing the upgrades, why they were done etc.

Maybe there is an opportunity to have this standard be automatically ERC165 compliant...? Haven't looked at it in-depth.

I don't see how it could be automatically compliant. I do like ERC165 and I think it is good for people to use it.

@fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Feb 27, 2020

I recommend that this standard use ERC-165, just for the cut function. This will help tools, such as Etherscan, recognize compliant contracts.


Because this contract is VERY general in purpose, the cut function should be renamed to a more qualified name such as diamondCut. Also I would like to see a discussion of the proposed name and any potential function selector conflicts in the rational section.


The event has arrays in it, so this limits the ability to search logs.


The API is overly complex:

enum CutAction {Add, Replace, Remove}
struct FacetCut {
  address facet;
  CutAction action;
  bytes4[] functionSelectors;
}
struct DiamondCut {
  FacetCut[] facetCuts;
  string message;
}
interface Diamond {
  function cut(DiamondCut[] calldata _diamondCuts) external;
  event DiamondCuts(DiamondCut[] _diamondCuts);    
}

It can be:

struct DiamondBatchCuts {
  bytes4[] functionSelectors;
  address[] implementations;
}
interface Diamond {
  function diamondCut(DiamondBatchCuts calldata diamondCuts) external;
  event DiamondCut(bytes4 functionSelector, address oldImplementation, address newImplementation);
}

It is unnecessary to categorize adding, changing and removing. Simply, a zero address corresponds to no implementation and a non-zero address is an implementation.


Use extra bytes.

Nobody asked for this feature but I'll suggest it any way.

I assume the standard contract is implemented like:

contract DiamondImplementation {
  mapping (bytes4 => address) implementations;
  function diamondCut(DiamondBatchCuts calldata diamondCuts) external {
    for (uint i = 1; i < diamondCuts.functionSelectors.length, I++) {
      address old = diamondCuts.functionSelectors[I];
      implementations[diamondCuts.functionSelectors[i]] = diamondCuts.implementations[i];
      emit DiamondCut(diamondCuts.functionSelectors[I], old, diamondCuts.implementations[I]);
    }
  }
}

So this means the storage is mapping (bytes4 => address) implementations;. That is wasteful because you are only putting 160 bits in a 256 bit register.

You can store more... but what? The selector in the target contract!

So you can have a function selector a on your Diamond contract be implemented by function b on the implementation contract. Why would you want to do that? Because of code reuse.

OR you can ignore this suggestion entirely if all the implementation contracts are implemented using the fallback function, which is better.

E.g. specify that all implementations are:

interface DiamondFunctionImplementation {
    fallback () external {
      // code goes here
    }
}

^^ this will be more efficient.

I didn't actually read the EIP some maybe you already specified this.


Documentation on storage mutability is insufficient. This is a major design consideration. And as somebody that audits contracts I'll hate auditing this kind of contract :-~~~


Mutability is bad. As stated before in my prior related review. I'm still not a fan of using this EIP or Zeppelin OS for upgradeable contracts. If you want to upgrade your contract then best practice is to deploy a new contract and spend your marketing budget to inform everybody of the new version.

This is what I did, multiple times, when working on ERC-721. Since I published the first "ERC-721 compliant contract" (it's Su Squares, check it out) that means I needed to redeploy it every time there was a new ERC-721 draft. That's okay, and all of the wallet providers know me because I kept having to bother them to update the contract address in MetaMask, MyEtherWallet, etc. And that's a good thing.

An exception is zero-knowledge proof contracts. These require an enormous amount of code. And these require a limited caveat to my note above. It might be reasonable for a ZKP contract to be deployed in multiple stages. But the contract should not be open to the public until deployment is completed (dependent contracts are loaded) and no further changes should be possible after deployment. Even still, the functionality of Diamond contracts should not be necessary for this deployment strategy.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 27, 2020

@fulldecent I appreciate your feedback on this.

I recommend that this standard use ERC-165, just for the cut function. This will help tools, such as Etherscan, recognize compliant contracts.

Yes, I'll add this to EIP-2535 Diamonds.

Because this contract is VERY general in purpose, the cut function should be renamed to a more qualified name such as diamondCut.

I agree that cut should be renamed to a more qualified name. I'll rename it to diamondCut.

Also I would like to see a discussion of the proposed name and any potential function selector conflicts in the rational section.

There is a section about function selector conflicts in the security section.

The API is overly complex:

The argument to cut is complex because it supports descriptive messages describing changes, like commit messages. And different messages can be associated with different sets of changes. The argument can be simplified if we throw out the descriptive messages of the changes, but I think it is worth it to keep them. I understand that documentation messages are not sufficient for a security audit but I think they can help people understand diamonds and why/what changes are made.

An important part of EIP-2535 Diamonds is creating user interfaces that pull all the verified source code that is used and displaying it in such a way that a person can see and understand all the code that is currently used by a diamond and also look at past code that was used before it was cut.

It is unnecessary to categorize adding, changing and removing. Simply, a zero address corresponds to no implementation and a non-zero address is an implementation.

It is necessary to prevent function selector conflicts. That's why it is there. The alternative is to let the user or off-chain software first verify that they aren't making any function selector clashes before calling cut but a diamond can't enforce they do this and it may be easier for them to specify add,replace,remove and the diamond can enforce they do this.

contract DiamondImplementation {

I understand your DiamondImplementation and I understand that an address is 160 bits and that additional data can be stored in the 256 slot. But after that I am lost. But I am interested. Can you explain more?

As stated before in my prior related review.

I think I kinda remember your prior review but for some reason I can't find it. Do you happen to know where it is? Because I'd like to review it if we can find it. Nevermind I found it! It was an email to me and I found it.

I remember you working and handling Su Squares and I thought you did a good job with it and the way you did things with it was good.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 28, 2020

This change is complete: I renamed the cut function to diamondCut in the standard.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 28, 2020

Wow, that is an interesting idea. I see what you are saying, the loupe isn't needed if software can simply look at the events to determine which functions exist. I'm not sure why getFacet(bytes4 selector) would be needed by the standard. The main purpose of the loupe is to provide interoperability with user interface software and tooling, but if events already give that then they could be used. People can always add in their own functions like getFacet(bytes4 selector) for their own purposes. I am considering removing the loupe completely from the standard.

Okay, here's the truth, I just don't totally trust 100 percent that events will be available all the time, forever, and with good performance all the time, forever. Maybe that is dead wrong and hope it is wrong and I'd love someone to prove it to me that it is wrong so I am totally convinced. I want to be overly safe until then -- after all we are dealing with diamonds!. Having the loupe functions implemented is a very good guarantee that you will be able to inspect your diamonds for facets and functions. If it is implemented right and it doesn't work then that means ethereum contracts don't work anymore and we have bigger problems.

So I'm keeping the loupe functions in the standard. Actually I removed two of them: functionSelectorByIndex and totalFunctions, because I don't see the need for them with the other ones. Also, by having events and loupe functions it provides two different ways to get the functions and facets of a diamond. If one way breaks for some impossible reason, the other way is available.

@GNSPS
Copy link

@GNSPS GNSPS commented Feb 28, 2020

I am going to support @leonardoalt fully here. Rare are the occasions where upgradeability on-chain cannot be replaced with off-chain mechanisms to achieve the same end result.

As an added problem, the more you complicate on-chain upgradeability mechanisms, the more obfuscated and less auditable these become. This means that clients' trust on the system is greatly reduced.

If everything is mutable why not just delegate execution?

@androolloyd
Copy link

@androolloyd androolloyd commented Feb 28, 2020

I am going to support @leonardoalt fully here. Rare are the occasions where upgradeability on-chain cannot be replaced with off-chain mechanisms to achieve the same end result.

As an added problem, the more you complicate on-chain upgradeability mechanisms, the more obfuscated and less auditable these become. This means that clients' trust on the system is greatly reduced.

If everything is mutable why not just delegate execution?

I feel like this approach takes the stance that we're never going to improve the way we do things today.

The audit log itself ensures that no central party has to prove what the state is, as it's self managed.

As a user if you want any real trust with proxies, you want to have your own, anytime an app needs a proxy they deploy one for you, as a user, you could have A proxy that you trust, that you cut with any features that you need, without having to extend that trust to anyone else to verify what the state of your proxy is.

Governance maintained Diamonds with the ability to cut in new features seems like a huge boon in terms of how we manage and maintain upgradeability.

No question there are new security challenges to deal with, but these types of patterns work well in other application development to date.

@leonardoalt
Copy link
Member

@leonardoalt leonardoalt commented Feb 28, 2020

As a user you should be asked to opt-in an upgrade, not be forced to trust obfuscated code.

@androolloyd
Copy link

@androolloyd androolloyd commented Feb 28, 2020

As a user you should be asked to opt-in an upgrade, not be forced to trust obfuscated code.

No disagreements there, which is why its great for user owned contracts.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 28, 2020

@leonardoalt @androolloyd @GNSPS One thing to keep in mind, which @fulldecent pointed out, is that this standard is very general, which also means that it is extremely flexible. It is easy to make the error of pegging this standard to a particular use case or to make assumptions about it.

As a user you should be asked to opt-in an upgrade, not be forced to trust obfuscated code.

@leonardoalt This standard is probably flexible enough to accommodate that. I'm interested in more details about how that could work.

I myself am very guilty of pegging this standard to a particular use case: "upgradeable contracts". This standard is useful for creating very useful immutable contracts that can't be upgraded. How so? Well there might be many ways (being so general and all) but I think of two really good use cases. But before I tell you the use cases let me tell you how to make useful immutable contracts with this standard.

The standard has been carefully edited to say that a diamond "uses" the diamondCut function. It does not say that the diamondCut function has to be added to the diamond as one of its functions. So this means that you can delegatecall to the diamondCut function in the constructor of the diamond to add all the functions needed by the diamond. The example in the standard shows how to call diamondCut in the constructor of a diamond. Since the diamondCut function is never actually added to the diamond the diamond is immutable with the same immutability and trust guarantees as a vanilla contract. My two use cases illustrate why this is useful:

  1. If you have a contract that hits the max contract size limit and there is too much code or dependency upon storage variables to separate it out into regular contracts, then you can make it an immutable diamond (which has no max size). Cool thing about making it a diamond is that you still break your big contract into smaller contracts, modularizing your code to a degree. A good description of a diamond is this: A group of contracts that share the same storage variables and address.

  2. You can start with an upgradeable diamond in your development and testing and upgrade it to your heart's delight. Reap the advantages of easy upgrading and a stable address as you work out new features, bugs and kinks. Release the upgradeable contract on a test network with your application for beta testing and upgrade it when needed. This is iterative development. When it is all solid then make it an immutable diamond and launch it on the main network.

Obfuscation exists when there are no tools to make something transparent and clear. This standard standardizes diamonds so that tools can be written for them so they are transparent and clear.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 28, 2020

@androolloyd I love this use case:

As a user if you want any real trust with proxies, you want to have your own, anytime an app needs a proxy they deploy one for you, as a user, you could have A proxy that you trust, that you cut with any features that you need, without having to extend that trust to anyone else to verify what the state of your proxy is.

I want everyone to have their own diamond.

@fergarrui
Copy link

@fergarrui fergarrui commented Feb 24, 2022

@FantasyGameFoundation

Does this mean that I can hardly inherit from any existing standard contract, to implement my own facet

No, it means to inherit from standard contracts that support diamond storage like from SolidState-Solidity or your own contracts, or contracts that don't read/write state variables.

Well it is actually a real concern. Only libraries that support diamond can be used (which is not the case of most common libraries like Openzeppelin)

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Feb 24, 2022

It is true that it is a problem that OpenZeppelin and some other contract libraries with contracts that are inherited and that read or write state variables don't work with diamonds. This barrier can be overcome by modifying existing contracts to work with diamonds, by writing contracts that work with diamonds and by using or contributing to smart contract libraries like SolidState-Solidity that support diamonds.

Also, keep in mind that all the Solidity libraries like Counters.sol, SafeERC20.sol, Strings.sol and all the rest of them from OpenZeppelin and other places all work with EIP2535 Diamonds. It is only the contracts that are inherited and read or write state variables that don't work in diamonds unless they are modified.

SolidState-Solidity has implementations for common things people need that work with diamonds.

@mainjoke
Copy link

@mainjoke mainjoke commented Feb 27, 2022

Excuse me, how should I use chainlink vrf in facet?

@nataouze
Copy link

@nataouze nataouze commented Mar 2, 2022

It is true that it is a problem that OpenZeppelin and some other contract libraries with contracts that are inherited and that read or write state variables don't work with diamonds. This barrier can be overcome by modifying existing contracts to work with diamonds, by writing contracts that work with diamonds and by using or contributing to smart contract libraries like SolidState-Solidity that support diamonds.

Hoping in on this point as I'm working on a base lib with a similar concept: core features managed on top of diamond-like storages, usable by inheritance (for proxy and non-proxy setting), or deployable as facets. Each facet having its own initialization logic, I realized that diamondCut could beneficiate from being able to call several initialization functions.

struct Initialization {
    address initContract;
    bytes initData;
}

function diamondCut(
    FacetCut[] calldata _diamondCut,
    Initialization[] calldata _initializations
) external;

For example, we would be able to add several facets and call their respective init sequences in one go.
Currently, we have 2 options:

  • call diamondCut several times, which partly defeats the logic of being able to manipulate several facets at the same time,
  • deploy an initialisation contract which takes care of doing the several necessary initialisation calls, which somehow defeats the logic of facets modularity.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Mar 2, 2022

@nataouze Yes, I see what you mean. I see that this function could be better for tooling and automated systems. Thank you for this valuable feedback.

I want to point out that adding and using the version of the diamondCut function you showed now is perfectly valid for diamonds and does not violate the standard.

The reason is because the standard says that people can create their own versions of the diamondCut function that take and use different parameters.

From the standard:

You can implement your own custom functions that add or replace or remove functions. You can also implement your own non-standard versions of diamondCut that have different parameters.

However the standard says that the DiamondCut event must be emitted for all functions added/replaced/removed. The only things that are actually enforced or standardized are the DiamondCut event, the loupe interface, and the Implementation Points. This is to give a lot of flexibility to implementors and creators while standardizing some things for integration and interoperability with tools and other software.

Would or could your new library use diamonds?

@nataouze
Copy link

@nataouze nataouze commented Mar 18, 2022

@mudgen Thank you for your comment.

One point about the DiamondCut event is that it would not be possible to emit all the initializations data in a single event. Emitting multiple events does not seem like an elegant solution, so I would rather use by default _init=address(0) and _calldata="". From what I can see, this is still compatible with the standard, but that would be good to get your insight on this.

I am thinking to create an extension ERC for this additional cut function, would this make sense?

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Mar 18, 2022

@nataouze I appreciate your attention and work on this. Perhaps tell me more so I can understand better.

The address _init, bytes _calldata parameters of the DiamondCut event are to specify what initialization function call, if any, was done in an upgrade. More info about this is in the standard.

@nataouze
Copy link

@nataouze nataouze commented Mar 18, 2022

@mudgen In this case, there would be several initializations called within a single diamondCut. But the DiamondCut event is designed for a single initialization. So there could be two possible scenarios:

1/ Emit a DiamondCut for each initialization. The first event will have the _diamondCut field as provided in the function argument, while _init and _calldata correspond to the first initialization call. The subsequent events will use an empty array for the _diamondCut field.

2/ Emit a single DiamondCut event, effectively dropping the _init and _calldata fields.

I am inclined to go for 2/ as I am actually not really clear of what use the _init and _calldata fields can be for an event listener: they do not represent a state change and do not allow to make a conclusion as to what consequence the initialization call can have.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Mar 18, 2022

The _init and _calldata parameters in the DiamondCut contain important information in an upgrade. They tell someone what function call was made with what arguments on what contract during an upgrade.

A person who wants to know all changes made in an upgrade would look at the DiamondCut event to see what functions were added/replaced/removed with what facets. And the person would look at the _init and _calldata arguments of DiamondCut event to see what function call was made with what arguments on what contract. Then the person could look at the verified source code of the contract the function was called on to see what the function did.

Only emitting one DiamondCut event is needed to get full information about an upgrade. This assumes that full verified source code is available for all changes made.

The DiamondCut event enables anyone to see all changes made in an upgrade. This is done by looking at the following things for any upgrade:

  1. Look at the verified source code for the function that emitted the DiamondCut event.
  2. Look at the functions added/replaced/removed in the upgrade. This information is contained in the DiamondCut event, which includes the function selectors and the contract addresses of the functions added/replaced/removed. A person can look at the verified source code for the functions added, replaced and removed.
  3. The DiamondCut event contains function call information in the _init and _calldata parameters. Those parameters can be used to find the verified source code of a function and contract that were called during an upgrade, if any.

Any number of initiation function calls can be done in single upgrade by the function call specified in the _init and _calldata arguments making other function calls. Information about these calls can be found by looking at the verified source code of the function making these function calls.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Mar 18, 2022

@mudgen Thank you for your comment.

One point about the DiamondCut event is that it would not be possible to emit all the initializations data in a single event. Emitting multiple events does not seem like an elegant solution, so I would rather use by default _init=address(0) and _calldata="". From what I can see, this is still compatible with the standard, but that would be good to get your insight on this.

I am thinking to create an extension ERC for this additional cut function, would this make sense?

I am sorry, I don't really understand what is the problem you are trying to solve or how you are trying to solve it. What is initializations data to you and why can't DiamondCut provide it or link to the function call that has it?

@nataouze
Copy link

@nataouze nataouze commented Mar 18, 2022

I am sorry, I don't really understand what is the problem you are trying to solve or how you are trying to solve it. What is initializations data to you and why can't DiamondCut provide it or link to the function call that has it?

struct Initialization {
    address initContract;
    bytes initData;
}

function diamondCut(
    FacetCut[] calldata _diamondCut,
    Initialization[] calldata _initializations
) external;

This is about doing multiple initialization calls using a single function call for an upgrade.

@nataouze
Copy link

@nataouze nataouze commented Mar 18, 2022

The _init and _calldata parameters in the DiamondCut contain important information in an upgrade. They tell someone what function call was made with what arguments on what contract during an upgrade.

A person who wants to know all changes made in an upgrade would look at the DiamondCut event to see what functions were added/replaced/removed with what facets. And the person would look at the _init and _calldata arguments of DiamondCut event to see what function call was made with what arguments on what contract. Then the person could look at the verified source code of the contract the function was called on to see what the function did.

Only emitting one DiamondCut event is needed to get full information about an upgrade. This assumes that full verified source code is available for all changes made.

The DiamondCut event enables anyone to see all changes made in an upgrade. This is done by looking at the following things for any upgrade:

  1. Look at the verified source code for the function that emitted the DiamondCut event.
  2. Look at the functions added/replaced/removed in the upgrade. This information is contained in the DiamondCut event, which includes the function selectors and the contract addresses of the functions added/replaced/removed. A person can look at the verified source code for the functions added, replaced and removed.
  3. The DiamondCut event contains function call information in the _init and _calldata parameters. Those parameters can be used to find the verified source code of a function and contract that were called during an upgrade, if any.

Any number of initiation function calls can be done in single upgrade by the function call specified in the _init and _calldata arguments making other function calls. Information about these calls can be found by looking at the verified source code of the function making these function calls.

I would argue that the information provided by the call arguments is not definitive:

1/ The source code may not be verified
2/ The call could be made on a code that has since changed when the event is processed (for example if the target is a proxy)
3/ The logic of the initialization function may depend on a current state which could be hard to track down

Concretely, if some information is absolutely relevant during an initialization call, it would seem more legitimate that the _init contract would be responsible for emitting a tailored event about it.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Mar 18, 2022

would argue that the information provided by the call arguments is not definitive:

1/ The source code may not be verified
2/ The call could be made on a code that has since changed when the event is processed (for example if the target is a proxy)
3/ The logic of the initialization function may depend on a current state which could be hard to track down

Concretely, if some information is absolutely relevant during an initialization call, it would seem more legitimate that the _init contract would be responsible for emitting a tailored event about it.

@nataouze Excellent points. I agree with you.

@nataouze
Copy link

@nataouze nataouze commented Mar 18, 2022

Maybe a snippet will help to put things in perspective. This is how I would implement the function mentioned above:

function diamondCut(FacetCut[] memory diamondCut_, Initialization[] memory initializations_) external {
    cutFacets(diamondCut_); 
    emit DiamondCut(diamondCut_, address(0), "");
    for (uint256 i = 0; i < initializations_.length; i++) {
        initializationCall(initializations_[i].initContract, initializations_[i].initData);
    }
}

When I read the standard, I can see that the constraints on emitting a DiamondCut event are related to the add/replace/remove actions. Except for this sentence:

The _diamondCut, _init, and _calldata arguments are passed directly to the DiamondCut event.

But cutting a facet could be operated outside of a call to diamondCut, so this sentence would not be relevant in every case.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Mar 18, 2022

But cutting a facet could be operated outside of a call to diamondCut, so this sentence would not be relevant in every case.

Yes, you are correct.

It makes sense to me how you implemented the function you showed, thanks for showing that.

@nataouze
Copy link

@nataouze nataouze commented Mar 18, 2022

It makes sense to me how you implemented the function you showed, thanks for showing that.

Thanks for bearing with me @mudgen 😊
Glad to hear from you that managing the event in this way is acceptable.

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Mar 18, 2022

Yes, thanks for bearing with me too. Yes, it is acceptable. I am thinking it is probably a good idea to change the diamondCut parameters in the standard to what you are using, but I'm not sure how much disruption that might cause amongst projects and tools that have already adopted the current version of diamondCut. But anyway, it is perfectly alright for people to make their own versions of diamondCut or other upgrade functions, as long as they emit the DiamondCut event to show functions added/replaced/removed.

@dotc-dev
Copy link

@dotc-dev dotc-dev commented Mar 24, 2022

The diamond contract has failed to verify in the blockchain browser. I have been failing to verify on the bsc testnet. How can I verify it successfully. Thank you.

Verifying DOTCFactoryDiamond@0xDaD59054b627e75C1cd1f233dA7DCE85447d03aC
Fail - Unable to verify
Failed to verify 1 contract(s): DOTCFactoryDiamond@0xDaD59054b627e75C1cd1f233dA7DCE85447d03aC

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented Mar 24, 2022

@dotc-dev Hi there. I suggest verifying each facet separately. And I suggest verifying the diamond proxy contract separate from its facets. Verifying the diamond proxy contract or facets is done the same way any other contract is verified.

I did recently talk to someone who had trouble verifying his contracts for his diamond implementation. He was able to fix it. This is what he said:

As far as I can tell, something was wrong with my hardhat repo. After trying various methods I tried to verify an empty contract and it also failed. I created a clean repo and it worked immediately. So I'm not sure what the issue was exactly. Some thing I tried before was linking the libraries which wasn't necessary, I also cleaned artifacts and cache, and tried some code changes to get it to work. It may have been a bug with hardhat.

Once you do get everything verified check that it shows up in louper.dev, which is like etherscan for diamonds.

@rhlsthrm
Copy link

@rhlsthrm rhlsthrm commented May 24, 2022

Once you do get everything verified check that it shows up in louper.dev, which is like etherscan for diamonds.

This is a great resource thank you. However is there any chance of getting Etherscan support for diamonds?

@mudgen
Copy link
Contributor Author

@mudgen mudgen commented May 24, 2022

Once you do get everything verified check that it shows up in louper.dev, which is like etherscan for diamonds.

This is a great resource thank you. However is there any chance of getting Etherscan support for diamonds?

Yes, if someone will contact the Etherscan team and ask or convince them to add support for diamonds, or if enough people ask them to do this.

@rhlsthrm
Copy link

@rhlsthrm rhlsthrm commented May 24, 2022

Once you do get everything verified check that it shows up in louper.dev, which is like etherscan for diamonds.

This is a great resource thank you. However is there any chance of getting Etherscan support for diamonds?

Yes, if someone will contact the Etherscan team and ask or convince them to add support for diamonds, or if enough people ask them to do this.

I just contacted asking: https://etherscan.io/contactus

@rhlsthrm
Copy link

@rhlsthrm rhlsthrm commented May 24, 2022

I also reached out to Tenderly. Their response:

Unfortunately, we are not supporting Diamond contracts. We have this in plan but this is the lowest priority and there are chances we won't implement this in foreseeable future. Sorry for bringing you the bad news 😕

Would love to get this support so everyone who wants this should also reach out to Tenderly!

florianleger added a commit to Gallion-labs/gallion-contracts that referenced this issue Jun 12, 2022
- Implementation of the [EIP-2535 Diamonds](ethereum/EIPs#2535) standard by using the [diamond-1-hardhat](https://github.com/mudgen/diamond-1-hardhat) boilerplate
- Migrated to Typescript
- Added empty Facets and some declarations in the AppStorage
- Updated dependencies
- Added typechain-types for a better contracts integration in TypeScript
florianleger added a commit to Gallion-labs/gallion-contracts that referenced this issue Jun 12, 2022
- Implementation of the [EIP-2535 Diamonds](ethereum/EIPs#2535) standard by using the [diamond-1-hardhat](https://github.com/mudgen/diamond-1-hardhat) boilerplate
- Migrated to Typescript
- Added empty Facets and some declarations in the AppStorage
- Updated dependencies
- Added typechain-types for a better contracts integration in TypeScript
florianleger added a commit to Gallion-labs/gallion-contracts that referenced this issue Jun 12, 2022
- Implementation of the [EIP-2535 Diamonds](ethereum/EIPs#2535) standard by using the [diamond-1-hardhat](https://github.com/mudgen/diamond-1-hardhat) boilerplate
- Migrated to Typescript
- Added empty Facets and some declarations in the AppStorage
- Updated dependencies
- Added typechain-types for a better contracts integration in TypeScript
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests