Implement batch transfer extension in BaseToken

[0x-r4bbit]

UPDATE: The description below is outdated. This issue has been update according to #41 (comment)

Status communities want to be able to batch transact tokens. Hence there's a request to move away from CollectibleV1 and instead have an ERC1155 implementation. Essentially it should be a drop-in replacement, meaning that, in addition to being an ERC1155 implementation, it should also have the custom function related to things like maxSupply that are currently implemented by CollectibleV1 and inherently BaseToken.

There is a chance we can't use BaseToken for this anymore, but this needs to analyzed.

To close this issue we need:

• Smart contract implementation
• Natspec docs where applicable
• Unit tests

[0x-r4bbit]

After further research, I've noticed some thing I'd like to bring up here:

• Noname and symbol support in ERC1155
  As per spec, ERC1155 tokens do not actually support name and symbol fields, as a single ERC155 implementation could represent multiple tokens (where name and symbol differ). As of today, Status Desktop/status-go makes heavy use of name and symbol of their tokens. This means name and symbol values would need to be handle via a token's metadata, which would be a breaking change for status-go

• No actual multitoken functionality needed
  ERC1155 is primarily a multitoken standard, meaning that it allows for representing multiple tokens in a single contract. So instead putting out an ERC20 (say CommunitERC20) and an ERC721 (say CollectibleV1), one could instead have just one CommunityERC1155 contract where the "ERC20" token have ID, say 0, and all other NFT token would get any of the other IDs.

  The main motivator for the introduction of such a contract at the time of writing this issue is the ability to perform batch transactions. This is cumbersome with plain ERC721 tokens as there has to be an approval for every token ID. ERC1155 allows for setting an operator to do transactions and then allowing for batch transactions.

Considering these two points, I wonder if it makes more sense to, instead of introducing a new CommunityERC1155 contract, have us backport some of the batching and operator functionality to the existing CollectibleV1. That way, we'd keep the name and symbol fields that are currently used, there's still the ability to send single tokens, but we'd also allow for batch transactions.

If for whatever reason this is not feasible, then maybe we can also consider going with ERC1155 from the get go and not have a dedicated ERC20 contract.

@gravityblast curious what you think about this
@3esmit please leave your thoughts as well if you have any! :)

[0x-r4bbit]

After more digging, I realized that open-zeppelin's ERC721 implementation (which we use for BaseToken and therefore inherently for OwnerToken and MasterToken) already supports setAppovalForAll() https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L123 and https://github.com/OpenZeppelin/openzeppelin-contracts/blob/fd81a96f01cc42ef1c9a5399364968d0e07e9e90/contracts/token/ERC721/ERC721.sol#L223

Given that this is the case, we can probably get away with simply adding the batch transfer to BaseToken

[0x-r4bbit]

As discussed offline with the team, we agreed that it's more feasible to extend BaseToken with batch transfer capabilities.
Updating the issue accordingly.

[0x-r4bbit]

Ran into another tiny challenge.

We're adding safeBatchTransfer() to BaseToken (as this is essentially what CollectibleV1 is.
Within safeBatchTransfer() we need to update an account's balance. The balances of the contract are stored inside _balances which lives in ERC721. BaseToken inherits from ERC721 so it operates on that property when doing things like safeTransferFrom().

The problem however is, that the property is private on ERC721, meaning there's no way access and update it from within BaseToken.

I'm exploring ways to go about this. Right now the following things come to mind:

1. Fork open zeppelin's contracts so we can maintain our own version of its ERC721 implementation that allows for _balances access (I'm not really fond of this as we'll have to make sure our fork stays up-to-date with upstream).
2. Not depending on OZ's ERC721 implementation and inlining most of its implementation into BaseToken. This is somewhat similar to maintaining our own fork just that we don't have a fork and a whole library to keep in sync.
3. Going back to giving ERC1155 a shot which comes with safeBatchTransfer() built-in.

@gravityblast any thoughts on this?

[0x-r4bbit]

As discussed offline, and suggested by @gravityblast , we could use _transfer() for this (which is what I ended up doing.