Making contract Sellable

[Lcressot]

🧐 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 :

• Owner approves buyer: allow the owner to approve a buyer with given price
• Approved buyers buys: allow an approved buyer to buy the contract (the eth. will go to the owner and the ownership will transfer)
• Owner approves proxy: allow the owner to approve a trusted proxy to transfer the ownership of the contract (as it is done for tokens)
• Appproved Proxy trasnfers ownership: allow an approved proxy to transfer the ownership of the contract (proxy can be marketplace sale contracts)

The difficulties that have raised are the following:

• ERC: I would have liked to implement the ERC interface but then my contract could not be inherited by other contracts also inheriting from an ERC interface as I wish it could.
• Ownable inheritance: I would have liked to inherit from the Ownable module but as far as I understand it would then not be possible to transfer the contract ownership with other methods than the Ownable.transferOwnership method.
  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.

[Amxx]

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.

[Lcressot]

Hey @Amxx
Let's open this new ERC discussion then! Should I start it on ethereum-magicians.org forum ?
I agree with #2568 if allowing all inheriting class of Ownable to make ownership transfer is okay, which I am not sure right now.

[Lcressot]

@Amxx
Following the discussion at #2887, thanks for the explanations, indeed I wasn't using the correct terminology.

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 :)

[fulldecent]

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. 

[Lcressot]

@fulldecent
Indeed, a modification of Ownable with a two-step ownership transfer would probably be the simplest and most effective way to make all contracts potentially sellable on a marketplace !

[Amxx]

@fulldecent Maybe you focus too much on the original PR code.

IMO the discussion quickly moved to "only" add something like this.

[Amxx]

Sharing a cleaned version of the code here:

abstract contract OwnableApprovable is Ownable {
    address private _pendingOwner;
    
    function pendingOwner() public view returns (address) {
        return _pendingOwner;
    }
    function setPengingOwner(address newOwner) public onlyOwner {
        _setPengingOwner(newOwner);
    }
    function acceptOwnership(address newOwner) public {
        require(_msgSender() == pendingOwner());
        require(newOwner != address(0));
        _setOwner(newOwner);
        // Emit an event ?
    }
    function _setPengingOwner(address newOwner) internal virtual {
        _pendingOwner = newOwner;
        // Emit an event
    }
    function _transferOwnership(address newOwner) internal virtual {
        super._transferOwnership(newOwner);
        if (_approved != address(0) {
            _approveTransferOwnership(address(0);
        }
    }
}

[fulldecent]

Instead I would replace Ownable because the current behavior is dangerous.

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);
}

[Amxx]

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?

[Lcressot]

@fulldecent
I agree with you, it would be the best solution. Also it has always felt weird to me that someone can send contracts or ownership even if the receiver doesn't want them. In NFT world there is a lot of wallet spamming with this!

@Amxx Would it be possible to temporarily add the nominateOwnerand acceptOwnership functions to Ownable while keeping the current transferOwnershipuntill it gets deprecated ?

[Amxx]

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.

[fulldecent]

interface Ownable {} // Deprecated

interface OwnershipClaimable { // No ERC needed yet
    event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
    modifier onlyOwner();
    function nominateOwner(address nominee);
    function claimOwnership();
    function renounceOwnership();
    function owner() view returns (address);
}

[frangio]

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 Ownable contract can then set an instance of this contract as the owner.

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 relay of transferOwnership and renounceOwnership, although there may be other functions that should be rejected as well.

[Lcressot]

@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.
We can transfer A normally like any Ownable contract, or we can transfer/trade its ERC721Ownership counterpart, which will trigger the transfer of A. Under the hood, the ERC721Ownership instance always keeps contract A's address as owner, but it acts like if its owner was the owner of contract A.

I think this solution has all the requirements we were looking for, and I am looking forward for your thoughts on it :)

[fulldecent]

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.

[frangio]

@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 transferOwnership directly. I think this loses some interesting properties of fully coupling the ownership to the NFT in both directions.

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.

[Lcressot]

@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 😅
Anyway thanks for the reply