-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
EIP-2535: Diamonds #2535
Comments
This is great. Would be curious to see a uniswap build using the Diamond pattern. |
@androolloyd yes, me too! |
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? |
@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. |
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? |
To me Uniswap is great exactly because it's not upgradeable/centralized. |
@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
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. |
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.
That's a very good feature!
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.
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. |
The upgrade functionality can also be removed by the contract not being upgradeable in the first place.
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. |
@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
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. |
Some good discussion of EIP-2535 Diamonds here: https://ethereum-magicians.org/t/diamond-contract-standard/4038 |
Awesome work @mudgen!! I like this new version of the EIP, and have a few comments:
|
@spalladino Thank you very much. This is great feedback! I appreciate that you take the time to look at this and write this feedback.
I like the idea of simplifying the argument to
The argument to the
Can you explain this more? I don't understand the scenario you are describing here.
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 @spalladino What do you think now? |
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
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.
Maybe there is an opportunity to have this standard be automatically ERC165 compliant...? Haven't looked at it in-depth. |
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.
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. |
I recommend that this standard use ERC-165, just for the Because this contract is VERY general in purpose, the The event has arrays in it, so this limits the ability to search logs. The API is overly complex:
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 You can store more... but what? The selector in the target contract! So you can have a function selector 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. |
@fulldecent I appreciate your feedback on this.
Yes, I'll add this to EIP-2535 Diamonds.
I agree that
There is a section about function selector conflicts in the security section.
The argument to 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 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
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?
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. |
This change is complete: I renamed the |
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: |
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. |
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. |
@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.
@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
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. |
@androolloyd I love this use case:
I want everyone to have their own diamond. |
@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:
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 |
I also reached out to Tenderly. Their response:
Would love to get this support so everyone who wants this should also reach out to Tenderly! |
- 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
- 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
- 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
There has been no activity on this issue for six months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review. |
Just adding my agreement that diamondCut should have multiple init addresses and data. I know I can create my own function to do this, but one of the things I work on is a generic evm deployment system, and in that case I have to stick rigidly to the spec. |
@anders-torbjornsen Stick rigidly to what spec? |
As in my deployment tool can only call the functions defined in this spec, I have to assume that there isn't a version of diamondCut which accepts multiple init addresses and data |
Okay, understood. Out of curiosity is your deployment tool hardhat-deploy? Or what is it? |
It's Zem, it came out of my first few NFT projects before I knew hardhat-deploy was a thing, and so here we are hehe. Still, competition is probably not a bad thing :) |
I am glad to see Zem. I'd argue that Zem and all deployment tools should add the standard However Zem and any other deployment tool can also (in addition) add and use their own upgrade function in the diamonds they deploy that better suits the deployment tool. The trick will be how to still emit a standard DiamondCut event with a custom upgrade function. Emitting a standard DiamondCut event is important for tools that show the history of diamond upgrades. I do acknowledge that a Unfortunately the technical aspects of this standard are finalized and won't change. However it is possible and welcomed to propose a new standard about a new |
@anders-torbjornsen I think that you can still achieve what you want to achieve with the current Here is how: Make a single initializer function in its own contract. Make that initializer function take as arguments an array of addresses of the facets or contracts with the init functions you want to call. The second argument is an array of function calldata. So then your initializer function loops through the addresses and calldata and makes a delegatecall with each one. This achieves the same thing as having a diamondCut that takes multiple addresses and calldata. An example of a diamond int function that calls multiple init functions is here: https://github.com/mudgen/diamond-1-hardhat/blob/main/contracts/upgradeInitializers/DiamondMultiInit.sol |
Yes that's true that'd work, and I'd only need to deploy that contract once and then could use for all future deployments |
Yes |
This appears to be superseded by https://ethereum-magicians.org/t/discussion-for-eip2535-diamonds/10459 |
Further EIP2535 diamonds discussion has moved here: https://ethereum-magicians.org/t/discussion-for-eip2535-diamonds/10459 |
Recently a DEX using diamonds was launched. See here: https://www.bsc.news/post/croswap-dex-launches-with-strong-volume-in-first-days |
EIP-2535 Diamonds exists here: https://eips.ethereum.org/EIPS/eip-2535
Below is a feedback and discussion of the standard.
The text was updated successfully, but these errors were encountered: