-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Making contract Sellable #2889
Comments
Hello @Lcressot AFAIK this approach, of having an ownable-sellable contract is not widely used nor is it clearly documented. As I already said in #2887, I think this should be the topic of a new ERC. I would love to include that in our codebase at some point, but before that, I think this disserves to be discussed in the ethereum-magicians.org forum and submitted to the EIP repository. For your point about ownable, I believe #2568 addresses it. |
@Amxx To answer you, I do not intend to build a contract marketplace myself, I just happened to witness people wanting to buy contracts and I had this idea. I am pretty confident that some day soon a lot of contracts in the industry will be sold. However it will maybe be done in real money with real contracts stipulating who the smart contracts should be transfered to. But there will probably also still be more intermediate size companies or independant willing to have a safe blockchain way to make a sale. And who knows maybe contracts could someday get traded on the stock market ^^ So my goal was really just to contribute but not for a short term usecase of my own. So I will keep in mind your idea of suggesting it to EIP repo and ethereum magicians, maybe we can do that together as I am still very new to these things :) |
I think the main feature that's missing and that is needed is to make transfer safe. 0xcert's Framework has a solution for this: Ownable should be two-step. First approve, then take. That's what I like in Compound. But against still, buying and selling a contract is a VERY involved process. And I do support a marketplace here. The marketplace will be a lot better if it welcomes any contract, not just ones that adopt this proposed interface. I am biased a few ways here because I'd like to buy contracts (with no less than 1,000 transactions) and I make money auditing contracts. |
@fulldecent |
@fulldecent Maybe you focus too much on the original PR code. IMO the discussion quickly moved to "only" add something like this. |
Sharing a cleaned version of the code here:
|
Instead I would replace interface Ownable {
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
modifier onlyOwner();
function nominateOwner(address nominee);
function acceptOwnership();
function renounceOwnership();
function owner() view returns (address);
} |
The current implementation is standardized (ERC173) and is expected by users. Please understand that for backward compatibility reason we cannot remove it and replace it with something new, potentially breaking a lot of user code as a consequence. What we can do is extending it (either in the Ownable contract, or as an extension) and maybe at some point in the future deprecate the current version. Is the interface you shared standardized / published? |
@fulldecent @Amxx Would it be possible to temporarily add the |
We don't do "temporary additions". The good thing is, you can build and ship an extension to Ownable now that the fundamental functions are available internally. It should be easy, and it will be compatible with our library so our users can choose to depend on both your code and ours. And you can do that today without us. But if we provide it, we would be committing to maintaining it for the foreseeable future (so that code that uses doesn't break with an update). Also, we would potentially be splitting the community if what we ship is not a standard, and something else emerges. We don't think our role is to initiate the creation of standard (even though we sometimes do) but rather to offer quality implementation of standards that the community, as a whole, supports. I'll discuss that with other members of the team next week, but I believe they will back me on this. |
|
There seem to be two independent things being discussed here: one is the ability to sell a contract, the other is a Claimable variant of Ownable. I don't understand the relation between those two things. For the ability to sell a contract, which is the focus of this issue, I agree with prior discussion that it should be based around an existing standard for reuse of existing marketplaces, and it seems that ERC721 is fit for this purpose, so it would be a matter of figuring out how to make ownership of a contract into an ERC721 token. But I think this is already reasonably achievable with what we have available: // SPDX-License-Identifier: MIT
pragma solidity 0.8.9;
import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/utils/Address.sol";
contract Ownership is ERC721 {
uint constant NFT_ID = 1;
constructor(address initialOwner) ERC721("Ownership", "OWN") {
_mint(initialOwner, NFT_ID);
}
function relay(address target, bytes memory data) public {
require(msg.sender == ownerOf(NFT_ID));
Address.functionCall(target, data);
}
} An There is one problem with the above code: a current owner can frontrun a purchase to transfer or renounce ownership of a contract. A possible way to fix this issue is to reject |
@Amxx @fulldecent @frangio Hi guys, I was inspired by @frangio's last answer and I imagined a solution to make contract's ownerships be tradable as ERC721. I made a code here: NFTOwnable repo. I tested it and it seems to work, I would be glad to contribute to openzeppelin, tell me what you think :) More precisely, to make a contract A be tradable, my idea is to make it inherit from NFTOwable. It will then have a private instance of ERC721Ownership which is an ERC721 contract containing one only NFT, that of the contract A's ownership. I think this solution has all the requirements we were looking for, and I am looking forward for your thoughts on it :) |
Probably "a thing I think is cool, but nobody is using in production, not even me" does not meet the threshold of what is included in this repo. |
@Lcressot In your design, it's like there are parallel ownership mechanisms. I wouldn't say that the NFT actually represents the ownership, because you can transfer ownership independently by calling I think these are very interesting examples and showcases of how the OpenZeppelin Contracts building blocks can be used, and I would encourage any of you to write some content about the idea, and possibly share it on our forum. But this use case is too niche at the moment for us to be interested in merging the functionality and maintaining the code. |
@frangio Got it, I did it mainly for learning. Also I just recall that there is still a problem in my implementation because the owner cannot call approve himself to approve a marketplace... My bad 😅 |
🧐 Motivation
Hi, I just submitted my first pull request ever here on openzeppelin (PR #2887) and I have been told to discuss my idea here with an issue :)
My idea is to make contracts sellable. For now, Ownable contracts can only be transfered for free to another owner by the owner himself/herself via Ownalble.transferOwnership.
Owning a contract gives a power on this contract and can also be profitable (selling NFTs for instance, or with built-in royalties). Some owners could be intersted in selling it to another party and doing it the safest way possible, which I want to implement. The contract should of course always stay transferable for free as well.
📝 Details
My idea for this module are :
The difficulties that have raised are the following:
This latter method is onlyOwner so I can't make a buyerApprove/buy nor proxyApprove/take set of methods to transfer the ownership. Also, the _setOwner is private so cannot be inheritated (as it shouldn't).
What I am proposing is thus to copy/paste the Ownable code inside the Sellable contract, which make Sellable able to use _setOwner, and make child classes able to use Ownable like if it was inherited.
The text was updated successfully, but these errors were encountered: