Proposing an access module to make contract sellable: Sellable.sol an…

[Lcressot]

Sellable: an extension of Ownable module to make contract ownership sellable. The module has a copy paste of Ownable inside it*, then it defines new methods to :

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

• I copy pasted Ownable instead of inheriting from it to be able to access its private method _setOwner, but the module will behave for users as if it had inherited from it.

Fixes #???? None

PR Checklist

• [v ] Tests
• [ v] Documentation
• [ ?] Changelog entry

[koolamusic]

I like this proposal.

[Amxx]

Hello @Lcressot.

Thank you for taking the time to propose contracts that offer new functionalities. We would however prefer if these features were discussed in an issue, so the exact requirements can be sorted out before any work is put into the code.

What you are proposing reminds me of a project I built a few years back. NFWallet are contracts (smart wallets) that are owned through an NFT. This created the ability to transfer & sell contract it using existing marketplaces (opensea).

The "selling" part of your proposal doesn't follow any standard. I would encourage you to pursue that first.

Also, I would argue that your code would deserve a lot of improvement before being ready:

• It would benefit from reusing the existing contract (ownable for example) rather than reimplementing it.
• Most functions do not emit events, making it difficult to follow the activity

IMO, for this feature to make it into OpenZeppelin contract, we would need a clear use-case, where this approach would be more effective than using NFTs AND a standardized interface (ERC) that would have been vetted by the community.

[Lcressot]

Hi @Amxx

Thanks for your short and full answer. Sorry I didn't open an issue about it first, it's my first time contributing. I will answer to your comments here and I also opened an issue as you said: #2889

• NFT: I am not sure I fully understand how it could work for contracts to be NFTs though it's very interesting. My idea was that any person making a contract could inherit the Sellable module to be able to sell the ownership later, when the contract gives the owner benefits like for instance with NFT royalties. If it were an NFT, it should be minted first by another contract, making it less independant right ?

• ERC: I did not see how I could implement the ERC interface because my module should be a parent contract of contracts implementing other ERC contracts, like Sellable ERC721 NFT contracts. There would be a conflict of function names. Maybe it will require to define a new standard interface for dealing with the ownership of the contract themselves.

• Ownable inheritance: I know it would have been preferable to reuse Ownable through inheritance, but then I would not be able to transfer the ownership with another way than the onlyOwner method Ownable.tranferOwnership, which I need to be able to do. That's why I decided to copy the Ownable code inside the Sellable contract, making it accessible to children contracts as if it were inheritated but also accessible to the Sellable methods that need to be able to make ownership transfers.

• As for using more events, this is not a problem.

[Amxx]

• NFT: I am not sure I fully understand how it could work for contracts to be NFTs though it's very interesting. My idea was that any person making a contract could inherit the Sellable module to be able to sell the ownership later, when the contract gives the owner benefits like for instance with NFT royalties. If it were an NFT, it should be minted first by another contract, making it less independant right ?

It true that you would need a trusted external NFT registry, but that would be easy to write, either for you application, or a generic one that any app could use.

For example:

Contract OwnableRegistry is ERC721("OwnableRegistry", "ownable")
{
    function mint(address owner)
    external
     {
        mint(owner, uint256(uint160(_msgSender())));
    }
    
    function _isApprovedOrOwner(address spender, uint256 tokenId)
    internal view virtual override returns (bool)
    {
        return super._isApprovedOrOwner(spender, tokenId)
            || uint256(uint160(spender)) == tokenId;
    }
}

abstract contract RegistryOwnable is Context {
    IERC721 public immutable ownershipRegistry;

    modifier onlyOwner() {
        require(owner() == _msgSender(), "RegistryOwnable: caller is not the owner");
        _;
    }
    
    constructor(IERC721 ownershipRegistry_) {
        ownershipRegistry_.mint(_msgSender());
        ownershipRegistry = ownershipRegistry_;
    }

    function owner() public view virtual returns (address) {
        return ownershipRegistry.ownerOf(uint256(uint160(address(this))));
    }

    function transferOwnership(address newOwner) public virtual onlyOwner {
        ownershipRegistry.transferFrom(owner(), newOwner, uint256(uint160(address(this))));
    }
}

• Ownable inheritance: I know it would have been preferable to reuse Ownable through inheritance, but then I would not be able to transfer the ownership with another way than the onlyOwner method Ownable.tranferOwnership, which I need to be able to do. That's why I decided to copy the Ownable code inside the Sellable contract, making it accessible to children contracts as if it were inheritated but also accessible to the Sellable methods that need to be able to make ownership transfers.

This is why I recently pushed for this commit. I think your extension of Ownable would have been a strong motivation to add this feature earlier ... but most people don't share their needs and fork code instead (that is a shame). Whenever you are limited by something like this, please share it so we can have the contract evolve in the right direction.

For the ERC part, my argument is to create a new ERC that documents the additional functions (on top of ERC173) and behaviors that would be define the ownable-sellable part

[Lcressot]

@Amxx
About your NFT idea, wouldn't there be a problem if the contract owner transfered ownership with Ownable.transferOwnership instead of tranfering the NFT ?
I still don't see how you could have the external registry sell the NFT ownership as well as the contract ownership itself, because this last ownership has to be transfered manually by the owner. That's why I think it would be preferable to have a Sellable module with a proxy approval interface to allow dealing with the ownership in an NFT like way.

I agree with your commit, it would solve a big part of my problem, allowing the Sellable to make ownership transfer. However it would mean that all inheriting class could make such a transfer, and I don't know if it would be good.

[Amxx]

About your NFT idea, wouldn't there be a problem if the contract owner transfered ownership with Ownable.transferOwnership instead of tranfering the NFT ?

Have a look at the code, the two are strictly identical. Using transferOwnership will transfer the NFT, and transferring the NFT will move ownership of the contract. Thus, if you sell the NFT on Opensea, you are selling ownership of the contract.

My argument with that being better, is that marketplaces and mechanisms to sell NFT are already widely available, which is not the case of your (non-standard) interface.

[Lcressot]

@Amxx
Ok I get it! With your registry trick you can have the contract ownership be dealt with as an ERC NFT while remaining able to have children of your contract also inheriting an ERC module. It's clever ! (However you also make a copy-past of Ownable here but it is the same issue as we discussed before with the need to replace private with internal in Ownable._setOwner).

I think it could be a great solution if we also added the methods I have proposed to make the owner able to make a direct sale to a buyer without using a proxy market place. These methods could be suject to a new standard, or not. I think they are important because for now there are no market places for contract and owners are more likely to be willing to make a direct safe sale.

However this solution would require to depend on an external trusted registry as you said and it could complicate things in practice.

[Amxx]

I had some thought about your proposal, and I now think the first points are a bad idea. Let me explain why.

ERC20 doesn't include any buy/sell logic. It doesn't include any built in AMM mechanism. What is great about ERC20 is that its simple and effective enough so that other contract (Crowdsale, Uniswap, ...) can operate on it, and implement complex logic. Also, you can come up with a new design for an AMM and still be compatible with old tokens.

ERC721 also doesn't include any sale/auction mechanism. it only deal with token transfer/approval ... and then third parties (like Opensea) do create contract that take benefit of the transfer mechanism to externally implement marketplaces.

IMO, the same logic should apply to selling Ownable contract.

If you code some mechanism in the contract, you are making an opinionated choice about who the sales should look like. I think this is bad, because you don't know in advance if your choices will still be relevant in 1, 3, 10 years. I believe it would be way simpler, cleaner, and more effective, to just improve the compatibility of Ownable with third-party transfers. This is what I do with the RegistryOwnable, but there are other ways of achieving that.

We can imagine

attract contract OwnableApprovable is Ownable {
    address private _approved;
    
    function approveTransferOwnership(address approved) public onlyOwner {
        _approved = approved;
        // Emit an event
    }
    function acceptTransferOwnership(address newOwner) public {
        require(newOwner != address(0));
        require(_msgSender() == _approved);
        _setOwner(newOwner);
        // Emit an event ?
    }
    function _transferOwnership(address newOwner) internal virtual {
        super._transferOwnership(newOwner);
        delete _approved;
    }
}

This mechanism is simple, light, versatile, and allow third party contract to handle sell

So I would go for ONLY including these 2 points

• allow the owner to approve a trusted proxy to transfer the ownership of the contract (as it is done for tokens)
• allow an approved proxy to transfer the ownership of the contract (proxy can be marketplace sale contracts)

[Amxx]

Here is an example of what a settling contract could look like.

contract OwnableSeller is Context {
    mapping(address => mapping(address => mapping(address => uint256))) public prices;

    function setPrice(address ownable, address buyer, uint256 price) public {
        prices[ownable][_msgSender()][buyer] = price;
    }

    function getPrice(address ownable, address seller, address buyer) public view returns (uint256) {
        uint256 price = prices[ownable][seller][buyer];
        if (price > 0) return price;
        return prices[ownable][seller][address(0)];
    }

    function buy(OwnableApprovable ownable, address newOwner) public payable {
        address owner = ownable.owner();
        uint256 price = getPrice(address(ownable), owner, _msgSender());
        require(price <= msg.value);
        require(price > 0);
        ownable.acceptTransferOwnership(newOwner);
        Address.sendValue(payable(owner), price);
        Address.sendValue(payable(_msgSender()), msg.value - price);
    }
}

It would be very easy to change it to accept payment using ERC20, or NFTs, which your approach will not support.

[koolamusic]

I agree with @Amxx on this, making this loosely coupled would enable anyone cater for use-cases that would be relevant beyond just 1 years, even in +10years to come. part of the implications of writing smart contracts is that once they are deployed on-chain, you're almost stuck with it.

Also looking at the OwnableSeller and OwnableApprovable example, I see how this can give the developer the ability to inject capabilities into the contract, for instance time-locking ownership transfers before pre- buy and post sell events.

[Lcressot]

@Amxx
I agree with you, if it is to become a standard, it needs to be usefull for a long time, and so be simple and versatile. I agree with your solution.

If I get you right we must wait that Ownable._setOwner gets updated to internal to be able to make this call:super._transferOwnership(newOwner);

Not important note:
I did not get why your setprice function is public, it seems to me anyone can chose their own price here, isn't it ? I thought the selling contract should ask the seller for his authorisation like it is done for nfts :

function setPrice(address ownable, address buyer, uint256 price) public {  
    require(_msgSender() == ownable.owner();  
    prices[ownable][_msgSender()][buyer] = price;    
}

[Amxx]

The seller (owner) set the price for a specific buyer, or for any buyer (address(0)).

[Lcressot]

@Amxx
Yes of course :) So what do we do next ? Should we open this discussion to other members, or wait for the ownable to change and have the private _setowner becomre internal before pursuing ?

[Amxx]

We would need the approveTransferOwnership, acceptTransferOwnership, and related events, to be standardized in an ERC that extends ERC173.

I believe @fulldecent is attached to this ownability transfer mechanism and might have an opinion to share.

[fulldecent]

I do not support this PR and think it should be closed. Here's why.

NFT are easy to review and compare. You can look at one CryptoKitty and look at another one. Looking at one helps you understand the other. Even non-technical people have an opinion on which ones they like. There are whole websites and wallets dedicated to helping you look at them and compare them. Pricing them and determining value is a whole study by itself.

There is a market to buy any and all Kitties. There are floor prices. People track the floor prices.

There is no such market for Ethereum contracts. Expert review of just one contract costs about USD 10,000–100,000. And you should review them before you buy them! If you don't read contracts before you buy them you will get scammed. Go and read the obfuscated Solidity contracts and compare to any popular NFT contract, can you tell which is backdoored?

Because this is not a liquid market, because nobody current does this, and because there is no standard, I think it is clear that this should be outside the scope of OpenZeppelin Contracts.

Now, if you want to create a marketplace for selling contracts, that's great. You should make your own custody smart contract, it can be compatible with Ownable contracts. You can have experts audit those contracts for sale. And you can go build a brand. There is no need to bloat every contract on the chain with the extra functions proposed in this PR.

[Amxx]

[...] because there is no standard, I think it is clear that this should be outside the scope of OpenZeppelin Contracts.

100% agree on that. I think I made it clear that the first step would be to have this standardized

Now, if you want to create a marketplace for selling contracts, that's great. You should make your own custody smart contract, it can be compatible with Ownable contracts. You can have experts audit those contracts for sale. And you can go build a brand. There is no need to bloat every contract on the chain with the extra functions proposed in this PR.

We would not bloat every contract, as this would be an opt-in extension to Ownable, that most people would possibly not use. It is true that it would mostly make sense for families or similar smart contracts (such as on-chain wallets?) for which the behaviour is known and understood.

I also think that having a custody wrapper would be the best option. However, I do have to admit that building such a custody system with the current transferOwnership system is difficult ... because the custody contract cannot take control of the ownable contract like it would an NFT (or some ERC20) using a transferFrom mechanism. This is why I think a transferFrom mechanism for (some) ownable contract could be interesting. I thought you were on the same page, since I thought I remember you praise Compound's timelock pendingAdmin / acceptAdmin. I'm sorry if I misunderstood.

[Lcressot]

@fulldecent
Hello, I understand your point. Initially my idea was not to extend to marketplaces but only to give the owner the possibility to make a safe paid transfer with a buyer instead of getting paid and transfering ownership in two separate steps as it is the only solution today.

Then I thought I could also allow a proxy to make the transfer and we discussed it with @Amxx and his view was more to abandon my first solution and make it compatible with ERC.

Given your argument, if the NFT/ERC option is out of the table, I think it would still be interesting to allow an owner to make a safe paid transfer, and maybe forget the proxy and ERC marketplace option.

However it is not impossible that in the future such marketplaces could emerge, taking a commission on the sale to get experts reviewing and auditing the code. Contracts will most probably never be traded like traditional NFTs but their could be a special trading procedure for them.

[Amxx]

[...] to abandon my first solution and make it compatible with ERC

@Lcressot You use the world ERC in a disturbing way.

For the record, and ERC is an Ethereum Request for Comment. This is the format used to standardized anything in the ethereum ecosystems. There are many of these, including ERC20 (that is the standard used by most fungible tokens) and ERC721 (that is the standard used by most non fungible tokens).

The current Ownable contract follows ERC173, which describes how ownable contract should behave (public functions, emitted events, ...).

If you were to build a marketplace for ownable contracts, you are obviously free to do it, but I really don't think it falls under the scope of OpenZeppelin. On the other hand, if the ERC173 standard includes some limitation, and if "Ownable" needs an extension to improve the feasibility of such a marketplace, then a NEW ERC173 (that extends 173) would possibly be welcome by the community and implemented by OpenZeppelin.

Hence, my proposition is that, if you feel like your usecase would need an extension of ERC173 (which supports some variation of transferOwnershipFrom / acceptOwnership), then you should propose to standard this extension (not here, on the EIP repo and on the eth magician forum). We would follow this proposal, and if it receives good feedback we would implement it, as an optional extension to Ownable.

[Amxx]

Discussion should continue at #2889