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

ERC875 for non fungible tokens and simple atomic swaps #875

Open
James-Sangalli opened this issue Feb 8, 2018 · 44 comments
Open

ERC875 for non fungible tokens and simple atomic swaps #875

James-Sangalli opened this issue Feb 8, 2018 · 44 comments

Comments

@James-Sangalli
Copy link
Contributor

@James-Sangalli James-Sangalli commented Feb 8, 2018

Summary

A simple non fungible token standard that allows batching tokens into lots and settling p2p atomic transfers in one transaction. You can test out an example implementation on rinkeby here: https://rinkeby.etherscan.io/address/0xffab5ce7c012bc942f5ca0cd42c3c2e1ae5f0005 and view the repo here: https://github.com/alpha-wallet/ERC-Example

Purpose

While other standards allow the user to transfer a non-fungible token, they require one transaction per token, this is heavy on gas and partially responsible for clogging the ethereum network. There are also few definitions for how to do a simple atomic swap.

Rinkeby example

This standard has been implemented in an example contract on rinkeby: https://rinkeby.etherscan.io/address/0xffab5ce7c012bc942f5ca0cd42c3c2e1ae5f0005

Specification

function name() constant returns (string name)

returns the name of the contract e.g. CarLotContract

function symbol() constant returns (string symbol)

Returns a short string of the symbol of the in-fungible token, this should be short and generic as each token is non-fungible.

function balanceOf(address _owner) public view returns (uint256[] balance)

Returns an array of the users balance.

function transfer(address _to, uint256[] _tokens) public;

Transfer your unique tokens to an address by adding an array of the token indices. This compares favourable to ERC721 as you can transfer a bulk of tokens in one go rather than one at a time. This has a big gas saving as well as being more convenient.

function transferFrom(address _from, address _to, uint256[] _tokens) public;

Transfer a variable amount of tokens from one user to another. This can be done from an authorised party with a specified key e.g. contract owner.

Optional functions

function totalSupply() constant returns (uint256 totalSupply);

Returns the total amount of tokens in the given contract, this should be optional as assets might be allocated and issued on the fly. This means that supply is not always fixed.

function ownerOf(uint256 _tokenId) public view returns (address _owner);

Returns the owner of a particular token, I think this should be optional as not every token contract will need to track the owner of a unique token and it costs gas to loop and map the token id owners each time the balances change.

function trade(uint256 expiryTimeStamp, uint256[] tokenIndices, uint8 v, bytes32 r, bytes32 s) public payable

A function which allows a user to sell a batch of non-fungible tokens without paying for the gas fee (only the buyer has to) in a p2p atomic swap. This is achieved by signing an attestation containing the amount of tokens to sell, the contract address, an expiry timestamp, the price and a prefix containing the ERC spec name and chain id. A buyer can then pay for the deal in one transaction by attaching the appropriate ether to satisfy the deal.

This design is also more efficient as it allows orders to be done offline until settlement as opposed to creating orders in a smart contract and updating them. The expiry timestamp protects the seller against people using old orders.

This opens up the gates for a p2p atomic swap but should be optional to this standard as some may not have use for it.

Some protections need to be added to the message such as encoding the chain id, contract address and the ERC spec name to prevent replays and spoofing people into signing message that allow a trade.

@James-Sangalli James-Sangalli changed the title ERC draft for non fungible tokens and simple atomic swaps ERC875 draft for non fungible tokens and simple atomic swaps Feb 9, 2018
@James-Sangalli James-Sangalli changed the title ERC875 draft for non fungible tokens and simple atomic swaps ERC875 for non fungible tokens and simple atomic swaps Feb 9, 2018
@naterush
Copy link

@naterush naterush commented Feb 12, 2018

There are parts of the example implementation that diverge from the specification or are added functionality (e.g. the endContract, getContractAddress, myBalance functions). I think it might be good to keep the example contract as close to the spec as possible.

Also, it's possible to implement atomic swaps for tokens in a single transaction with contracts that exist above current ERC token so that we don't have to add this functionality to every token contract that gets deployed. Adding this is general purpose contract seems like a better approach than implementing it in every token contract (maintainability and cost reasons). Otherwise, we can keep changing the token spec to add arbitrary functionality.

@cylon56
Copy link

@cylon56 cylon56 commented Feb 12, 2018

Does this standard allow someone to store different ERC721 tokens in the same ERC875 token? I.e. Could I create an ERC875 token that holds a basket of assets such as stocks, bonds, real estate, all of which have are different NFAs?

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Feb 13, 2018

@naterush Hi Nate, thanks for your comment! I added those functions in as boilerplate, they are not part of the ERC Spec by default I just wanted them in there in case I wanted to remove the contract later etc.

The getContractAddress() is needed in this spec as a protection against double spending (i.e. I could get you to sign a message for my contract and rebroadcast it on another contract, by adding this data in as well as a message like ERC875 etc this is not possible)

Yes, open to here the views of what people would like to change in this draft!

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Feb 13, 2018

@cylon56 the token itself is a 256 bit unsigned int, you could represent multiple assets in the one variable via encoding but it depends how complete your info is. From my understanding this would not be the case for cryptokitties as each cat requires the full 256 bit number to be represented, however if you asset info is simple enough, you could represent multiple assets in one token index.

@Nanolucas
Copy link

@Nanolucas Nanolucas commented Feb 13, 2018

Why does this need to be a standard that competes with the proposed #721, #841 or #821? This looks more like a version of those same standards which ignores most of the improvements that have been made in each of them (based on community discussions) simply for the sake of adding additional transfer functionality.

This seems more like something that should be suggested as an optional extension interface for the existing standards proposals, rather than attempting to be its own entirely new proposal.

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Feb 13, 2018

Hi @Nanolucas , Thanks for the feedback! I am open to have it be an extension or it's own standard or a standard that has proposed changes which are implemented. This is only meant as an initial draft and I am open to hearing suggestions to change it.

@Nanolucas
Copy link

@Nanolucas Nanolucas commented Feb 13, 2018

In #841 you can see the optional extensions ERC721Metadata and ERC721Enumerable, I think providing your proposal in that format as some sort of ERC721Trade would be much more logical.

That way, the base functionality of the NFT/deed/asset registry standard isn't something you have to worry about when your focus is on developing a standardised way to perform swaps or multiple transfers.

Edit: Accidentally referenced incorrect EIP

@colourful-land
Copy link

@colourful-land colourful-land commented Feb 21, 2018

@Nanolucas, I guess you refer to ERC721Enumerable in #841.

A few years ago I was involved in a project by Sydney entrepreneur Nick Addison (now of Consensys Sydney) where we wanted to tokenise subway advertisement screens in batches of 5-minute. For example, someone can buy the 5-minutes ad of Sydney Townhall station LCD ad in a rush hour because it's his birthday, or because he believes it was undervalued. It failed for regulatory reasons but what I observed is that investors tend to buy and sell hours or days of advertisement together, moving 2000 tokens a time - each covers 5 minutes of one screen in one subway station. The technical learning is that transaction fee only increase a little (less than 10%) if you transact 200 tokens instead of 1 in one transaction because most of the fee goes to the update of state tree. So there definitely is an incentive to transfer them in one call, instead of calling an Enumerator and transfer each asset piecemeal by the identifiers returned from Enumerator. Furthermore, being able to specify a range is helpful for reducing blockchain size, since such batch movements are usually done in lots. If someone moves 2000 tokens, there is a good chance all of them are from the same subway station or same hour of all subway stations.

It turns out that the Sydney subway experiment was not one of a kind - there are in fact more business cases of batch NFT operations than there are for individual NFTs (collectables), as I learned by working on a blockchain architect role in Australia's largest bank. For example, RESIMAC regularly transfers thousands of loan titles to Westpac in one go and requires them to move in an atomic fashion. The thriving of the kittens did give an impression that NFT is usually traded one by one - all because real businesses haven't moved in.

That was just the argument why Enumerator is not the answer for a batch transaction. trade() was for a different reason though. I see reasons for ERC721Trade extension to be there in #841 as well as for having a version of trade() function to move assets in batch.

@Nanolucas
Copy link

@Nanolucas Nanolucas commented Feb 21, 2018

@colourful-land My reference to ERC721Enumerable was only as an example of an optional extension for the ERC721 base standard. I was suggesting that the non-base functionality of this proposal should be in the form of an ERC721Trade extension to ERC721 so that the base functionality doesn't have to be reproduced in a separate standard.

I was certainly not meaning to imply there was an existing useful method for batch transactions, since there isn't (as far as I'm aware).

This was referenced Feb 21, 2018
@MoMannn
Copy link

@MoMannn MoMannn commented Feb 22, 2018

With the delegation added to the current 721 standard shouldn't atomic swap be possible with the use of a proxy(the same way 0x project is doing it)?

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Feb 25, 2018

@MoMannn yes, however this is an alternative implementation.

The proxy could either hold a key or hold a batch of signed orders and release them according to expiry, neither is fully trust-less but the attack vector is limited.

@HackFisher
Copy link

@HackFisher HackFisher commented Feb 26, 2018

I think it is important that there should be a compatible method as following with-in ERC20 & ERC721:

function transfer( address to, uint valueOrTokenId) public returns (bool ok);

Otherwise, some existing contracts using "transfer" will not be able support ERC875, e.g.

function withdraw(address _token, uint _balanceOrTokenId) public {
        if (_token == 0x0) {
            target.transfer(this.balance);
            return;
        }

        ERC20ERC721 token = ERC20ERC721(_token);
        token.transfer(target, _balanceOrTokenId);
    }
@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Mar 1, 2018

@HackFisher are you suggesting containing two mapping variables with different balances types for compatibility with other ERC's like ERC20 and 721?

@HackFisher
Copy link

@HackFisher HackFisher commented Mar 12, 2018

@James-Sangalli I was suggesting supporting the transfer cases of only one tokenId, this was proposed when I'm trying to design a contract can support future assets standards, but found that This ERC875 have different API with ERC721. I resolve that buy using more general methods, but by compatible with existing API, could gather more support from existing standards infrastructure.

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Mar 22, 2018

@HackFisher I see what you mean however I don't think it makes much sense to have that even in erc721 when it is causing an inefficiency for no good reason (cannot transfer by bulk). Most of the gas cost usually comes from the transaction itself so I am not sure why it was decided that you could only do it one by one.

I am open to being corrected on my view however...

@fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Mar 22, 2018

You can do bulk transfer in 721. But that would be an implementation-specific design.

I would be happy to standardize after we see some use in the wild.

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Mar 22, 2018

@fulldecent yes but it should probably be included in the standard. We will be doing a ticketing project soon which requires such functionality and I am sure many others will as well.

@fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Mar 22, 2018

Personally I think the time to discuss a standard is after your ticketing application is live and on mainnet and wallet wants to interface with it.

Right now your best bet is to launch with 721 include extension for the parts you want. 721 is extendable and two extensions are already provided.

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented May 30, 2018

Here is an example link that uses the standard: https://app.awallet.io/AJiWgGhh6nmmaj8IBoF06PAFESqLLHpQeoIjNQZTftbzkBzkhVw8h7dsVi3lK30zDQQHGJyJKGJavooHfyxh9ZNvcIAk4mzAO9fjjGmt8xyylxsnWdVfZr48rG1PHA==

Our protocol allows you to transfer and sell links via URL's like this. The parameters for the trade function can be parsed from this link and plugged into the trade function.

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Jun 13, 2018

You can now easily create your own ERC875 token here: https://alpha-wallet.github.io/ERC875-token-factory/index

@bianning
Copy link

@bianning bianning commented Jul 20, 2018

How the seller cancels a transaction?

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Jul 20, 2018

@bianning I am not sure what you mean? Once a transaction has been executed you cannot stop it.

An order (constructed offline) will have an expiry timestamp

@bianning
Copy link

@bianning bianning commented Jul 20, 2018

I have signed for the trade and disclosed the information, but I change my mind before someone transfer the trade. How can I withdraw it?

@duncanwang
Copy link

@duncanwang duncanwang commented Jul 30, 2018

I have 3 question, please help me:
(1) trade(uint256 expiry,
uint256[] tokenIndices,
uint8 v,
bytes32 r,
bytes32 s)
A sample is given as 0, [3, 4], 27, "0x2C011885E2D8FF02F813A4CB83EC51E1BFD5A7848B3B3400AE746FB08ADCFBFB", "0x21E80BAD65535DA1D692B4CEE3E740CD3282CCDC0174D4CF1E2F70483A6F4EB2"
QUESTION: what the means of v, r, s ?

(2)What's the means of the code below in function trade() ?
inventory[msg.sender].push(inventory[seller][index]);

(3) How can I trigger "trade" function to get a success process in REMIX & MetaMask environment with Ropsten test net?

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Jul 30, 2018

Hi @duncanwang

  1. v r and s are simply the cryptographic signature split up into 3 parts
  2. As the balance of non fungibles is stored as an array, the sellers elements are added to the senders array on success of the trade
  3. I recommend you go here: https://github.com/alpha-wallet/ERC875-Example/blob/master/TradeImplementationExample.java and generate a signature, at the moment the contract is on rinkeby: https://rinkeby.etherscan.io/address/0xffab5ce7c012bc942f5ca0cd42c3c2e1ae5f0005 for which you can copy and paste the address into remix with the contract and call the trade function. Do you require it to be done on ropsten?
@zhangzhongnan928
Copy link

@zhangzhongnan928 zhangzhongnan928 commented Jul 31, 2018

Bianning
I have signed for the trade and disclosed the information, but I change my mind before someone transfer the trade. How can I withdraw it?

You can “transfer”/purchase the token by yourselves before anyone else does.

@duncanwang
Copy link

@duncanwang duncanwang commented Jul 31, 2018

How can I get some free ETH on rinkeby test net? Can you send 2 ETH to the address "0xB51Fa936B744CFEbAeD8DbB79d2060903e689F89" ?

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Jul 31, 2018

@duncanwang sent you some test ether

@duncanwang
Copy link

@duncanwang duncanwang commented Aug 1, 2018

I've received 2 ETH。Thank you very much, james.
As your proposal,I run trade function after my MetaMask connected to rinkeby network
image.
How can I buy this ticket in chrome browser with MetaMASK, then?

@duncanwang
Copy link

@duncanwang duncanwang commented Aug 17, 2018

Can ERC875 be used for shopping scenario ? For example, a custom buy many NFTs one times paying with ERC20 tokens. Or exists any other solution ?

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Aug 17, 2018

@duncanwang You mean purchasing ERC875 non fungibles with ERC20 tokens rather than ether?

@duncanwang
Copy link

@duncanwang duncanwang commented Aug 18, 2018

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Aug 20, 2018

@duncanwang this feature is not implemented as is and would be a bit tricky to do, but it is probably possible.

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Nov 11, 2018

Closing as this has been merged. Further discussion can take place here: #1549 and in future PR's/issues

@axic
Copy link
Member

@axic axic commented Jul 4, 2019

Closing as this has been merged. Further discussion can take place here: #1549 and in future PR's/issues

Pull requests cannot be nominated as a discussion URL.

@axic axic reopened this Jul 4, 2019
@lyhistory
Copy link

@lyhistory lyhistory commented Jul 5, 2019

👍 I think EIP875 is ready to be finalized, If someone has some changes they would like to suggest, we should discuss it now.

@bobjiang
Copy link

@bobjiang bobjiang commented Jul 5, 2019

I believe ERC875 (this EIP) should be finalized, because there is not only commercial example (2018 world cup ticket trail), but also have ethereum developer's support, let's say HiBlock had run several workshop to promote ERC875.

If any further change, there should be an additional/extra patch for it.

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Jul 6, 2019

I just added approve functionality to the spec as this is necessary for all token standards. Other than that I think it is ready to go.

@fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Jul 13, 2019

I will review this at some point if it is still in last review by 2019-07-29.

But right now, this draft does not follow the required format for EIPs. See

https://github.com/ethereum/EIPs/blob/master/eip-X.md

Currently the largest weaknesses are

  • Lack of implementations
  • It keeps changing
  • "Gas savings" is mentioned but there is no gas analysis
  • It is very similar to another EIP but no attempt is made to address compatibility
@fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Jul 29, 2019

Following is my review of ERC-875. This includes technical problems which warrant cancelling/extending this Last Call.

This EIP puts forth a standard but provided implementations are not provided

There should be a deployed implementation as a case for standardizing the interface. At current, no such implementation is presented where Ether has been spent and the product has been developed. It fails to demonstrate that the lead author has skin in the game.

It keeps changing

The standard has changed but the Last Call has not been cancelled.

"Gas savings" is mentioned but there is no gas analysis

A primary motivation for this EIP is to reduce gas costs

this is heavy on gas

but no analysis is provided to show that this proposed standard solves that problem.

It is very similar to another EIP but no attempt is made to address compatibility

This could clearly be implemented as an extension to ERC-721 or ERC-1155 but no such notes on backwards compatibility are addressed.

@James-Sangalli
Copy link
Contributor Author

@James-Sangalli James-Sangalli commented Jul 30, 2019

@fulldecent

Following is my review of ERC-875. This includes technical problems which warrant cancelling/extending this Last Call.

This EIP puts forth a standard but provided implementations are not provided

Implementation has (and has been for a long time) been provided here: https://github.com/AlphaWallet/ERC875-Example-Implementation

There should be a deployed implementation as a case for standardizing the interface. At current, no such implementation is presented where Ether has been spent and the product has been developed. It fails to demonstrate that the lead author has skin in the game.

We have many such tokens running in our own wallet and we supported atomic swap magic links natively in AlphaWallet. An example is our FIFA ticket experiment(where FIFA tickets were tokenised and held significant value (>1000USD), xDAI coin drop links (giving free xDAI to participants) and various tickets for events we have held.

Example magic link which supports ERC875: https://xdai.aw.app/AQAAAABc0rRfuU_5wI_lgMWiln2fYuVc3hb41s4HcpCXNwNvIMdK3fj8odeAhUIWgBJtmZnEvNaZlrOYSsB0esHHVaoIOFFyHFJXfmrM-1vzL6qLCYIAEhXUlWbozhw=

Our wallet also supports redeeming, selling and transferring such tokens and we even had a payment server to cover the transaction cost for cryptoless users and to prevent front running.

It keeps changing

The standard has changed but the Last Call has not been cancelled.

Our only major changes were from bytes32[] to uint256[] and adding the approve spec.

"Gas savings" is mentioned but there is no gas analysis

A primary motivation for this EIP is to reduce gas costs

this is heavy on gas

This conclusion was reached when ERC721 had just come out, back then it only supported transfers of one token at a time. This was much more gas heavy when a user wanted to transfer multiple tokens. It was and still is a pain to use erc721 in wallets because it is impossible to get the users full balance without a fully fledged service like OpenSea which indexes events.

but no analysis is provided to show that this proposed standard solves that problem.

It is very similar to another EIP but no attempt is made to address compatibility

This could clearly be implemented as an extension to ERC-721 or ERC-1155 but no such notes on backwards compatibility are addressed.

Extensions would be a good idea, agreed. For now we will mostly be using it as a showcase for some tokens but our wallet is agnostic on this front and we are building a powerful framework to add extra functionality to existing tokens (regardless of the spec). See TokenScript if you are interesting in taking a glimpse of what can be done with tokens :)

Thank you for your feedback!

@fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Jul 30, 2019

Implementations

Glad to see this implementation. Please update the EIP draft to use the heading "Implementation" per https://github.com/ethereum/EIPs/blob/master/eip-template.md then add that implementation you have cited.

Changes to EIP specification

Fair enough. This item is resolved.

Gas savings

Regarding gas savings for batch. This is a technical decision. Please cite it in the rationale section including references to any prior discussion that you believe are relevant. ERC-1155 also provides batch transfers. The text of the new EIP should explain its technical decisions in the rationale section and make reference to ERC-721 and ERC-1155 if the intention is to present itself as superior in some respect. Doing the analysis to present an actual gas comparison to with multiple techniques (this draft EIP vs. 1155 vs. 721 multiple transactions vs 721 using a batch transfer agent) would be very helpful for your intended audience.

You also mentioned token balance under that heading, which I address in the next point.

Backwards compatibility

The decision to not provide backwards compatibility with ERC-721 or ERC-1155 is your decision to make. These points should be raised inside the EIP text under the backwards compatibility section and also the rationale section as required in https://github.com/ethereum/EIPs/blob/master/eip-template.md.

Your note regarding balance is not clear to me because ERC721Enumerable does provide the total supply onchain without the need of an offchain wallet.

Headings

Please adopt the other required headings as per https://github.com/ethereum/EIPs/blob/master/eip-template.md

  • Summary -> simple summary
  • Purpose -> Abstract
  • Rinkby Example -> put somewhere else
  • Motivation missing
  • Optional functions -> should be part of specification, not its own heading
  • Interface -> should be part of specification
  • Rational missing
  • Backwards Compatibility missing
  • Test cases missing
  • Example implementation -> Implementation
@fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Aug 7, 2019

Can this last review be ended please and return to draft. It seems this does not have enough momentum to get to final directly at this time.

@fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Dec 1, 2019

Requesting to please mark this as back to DRAFT status or ABANDONED.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet