Adding fees for other functions, added more test coverage when fees are enabled

Author: johnhforrest

README.md

@@ -2,17 +2,14 @@
 [![Build Status](https://travis-ci.org/codex-protocol/contract.codex-registry.svg?branch=master)](https://travis-ci.org/codex-protocol/contract.codex-registry)
 
 ## To do
-### Testing
-- Extensive testing on CodexTitle
-
-### Features
-- Documentation hashes array for CodexTitle (i.e., in addition to images)
+- Extensive testing on ERC900
+- Token Ejection (similar to BiddableEscrow)
+- 100% code coverage
+- Gas optimization
+- TCR for providers
 - Add a version field to the tokens themselves so we can track if they've been upgraded or not. This is useful for cases where storage state in the new implementation needs to be migrated over.
-- Consider adding the burn functionality back in, but maybe restricting it to onlyOwner for now.
 
 ## Notes
-- Everything under the zeppelin-solidity directory was copied over from node_modules/zeppelin-solidity to give visibility into the contract code and so they can be pinned to a specific compiler version. No extensive changes have been made
-- This contract is not intended to be used directly on it's own. Users should instead communicate to the contract through instances of the Biddable Widget hosted by our consortium members
 - If you accidentally send ERC-20 tokens to the address where this is deployed, please email contact@codexprotocol.com to discuss retrieval
 
 ## Thanks to

contracts/CodexTitleAccess.sol

@@ -21,7 +21,7 @@ contract CodexTitleAccess is CodexTitleCore {
     string _providerMetadataId) // TODO: convert to bytes32
     public
     whenNotPaused
-    canPayFees
+    canPayFees(creationFee)
   {
     return super.mint(
       _to,
@@ -42,6 +42,7 @@ contract CodexTitleAccess is CodexTitleCore {
     uint256 _tokenId)
     public
     whenNotPaused
+    canPayFees(transferFee)
   {
     return super.transferFrom(_from, _to, _tokenId);
   }
@@ -55,6 +56,7 @@ contract CodexTitleAccess is CodexTitleCore {
     uint256 _tokenId)
     public
     whenNotPaused
+    canPayFees(transferFee)
   {
     return super.safeTransferFrom(_from, _to, _tokenId);
   }
@@ -69,6 +71,7 @@ contract CodexTitleAccess is CodexTitleCore {
     bytes _data)
     public
     whenNotPaused
+    canPayFees(transferFee)
   {
     return super.safeTransferFrom(
       _from,
@@ -87,9 +90,10 @@ contract CodexTitleAccess is CodexTitleCore {
     bytes32 _newDescriptionHash,
     bytes32[] _newImageHashes,
     string _providerId, // TODO: convert to bytes32?
-    string _providerMetadataId // TODO: convert to bytes32?
-  )
-    public whenNotPaused
+    string _providerMetadataId) // TODO: convert to bytes32?
+    public
+    whenNotPaused
+    canPayFees(modificationFee)
   {
     return super.modifyMetadataHashes(
       _tokenId,

contracts/CodexTitleFees.sol

@@ -26,11 +26,17 @@ contract CodexTitleFees is Pausable {
   // Fee to create new tokens. 10^18 = 1 token
   uint256 public creationFee = 0;
 
-  modifier canPayFees() {
+  // Fee to transfer tokens. 10^18 = 1 token
+  uint256 public transferFee = 0;
+
+  // Fee to modify tokens. 10^18 = 1 token
+  uint256 public modificationFee = 0;
+
+  modifier canPayFees(uint256 baseFee) {
     if (feeRecipient != address(0)) {
       // TODO: Update the discount to be based on weight as opposed to just
       //  a binary on/off value
-      uint256 calculatedFee = creationFee;
+      uint256 calculatedFee = baseFee;
       if (codexStakeContainer != address(0) &&
         codexStakeContainer.totalStakedFor(msg.sender) >= 0) {
 
@@ -47,25 +53,29 @@ contract CodexTitleFees is Pausable {
 
   /**
    * @dev Sets the address of the ERC20 token used for fees in the contract.
+   *  Fees are in the smallest denomination, e.g., 10^18 is 1 token.
    * @param _codexToken The address of the ERC20 Codex Protocol Token
    * @param _feeRecipient The address where the fees are sent
-   * @param _creationFee The new creation fee. 10^18 is 1 token.
+   * @param _creationFee The new creation fee.
+   * @param _transferFee The new transfer fee.
+   * @param _modificationFee The new modification fee.
    */
-  function setFees(ERC20 _codexToken, address _feeRecipient, uint256 _creationFee) external onlyOwner {
+  function setFees(
+    ERC20 _codexToken,
+    address _feeRecipient,
+    uint256 _creationFee,
+    uint256 _transferFee,
+    uint256 _modificationFee)
+    external onlyOwner
+  {
     codexToken = _codexToken;
     feeRecipient = _feeRecipient;
     creationFee = _creationFee;
+    transferFee = _transferFee;
+    modificationFee = _modificationFee;
   }
 
   function setStakeContainer(ERC900 _codexStakeContainer) external onlyOwner {
     codexStakeContainer = _codexStakeContainer;
   }
-
-  /**
-   * @dev Sets the creation fee in CODX
-   * @param _creationFee The new creation fee. 10^18 is 1 token.
-   */
-  function setCreationFee(uint256 _creationFee) external onlyOwner {
-    creationFee = _creationFee;
-  }
 }

contracts/ERC900/ERC900.sol

@@ -17,7 +17,7 @@ contract ERC900 {
   function token() public view returns (address);
   function supportsHistory() public pure returns (bool);
 
-  // optional
+  // NOTE: Not implementing the optional functions
   // function lastStakedFor(address addr) public view returns (uint256);
   // function totalStakedForAt(address addr, uint256 blockNumber) public view returns (uint256);
   // function totalStakedAt(uint256 blockNumber) public view returns (uint256);

contracts/ERC900/ERC900BasicStakeContainer.sol

@@ -37,17 +37,29 @@ contract ERC900BasicStakeContainer is ERC900 {
     Stake personalStake;
   }
 
-  constructor(ERC20 _stakingToken) public {
-    stakingToken = _stakingToken;
+  modifier noExistingStake(address _address) {
+    require(
+      !addresses[_address].personalStake.exists,
+      "Stake already exists");
+    _;
   }
 
-  function stake(uint256 _amount, bytes _data) public {
-    require(!addresses[msg.sender].personalStake.exists, "Stake already exists");
-
+  modifier canStake(address _address, uint256 _amount) {
     require(
-      stakingToken.transferFrom(msg.sender, this, _amount),
+      stakingToken.transferFrom(_address, this, _amount),
       "Stake required");
+    _;
+  }
 
+  constructor(ERC20 _stakingToken) public {
+    stakingToken = _stakingToken;
+  }
+
+  function stake(uint256 _amount, bytes _data)
+    public
+    noExistingStake(msg.sender)
+    canStake(msg.sender, _amount)
+  {
     addresses[msg.sender].personalStake = Stake(block.number, _amount, true);
     addresses[msg.sender].amountStakedFor.add(_amount);
 
@@ -58,13 +70,11 @@ contract ERC900BasicStakeContainer is ERC900 {
       _data);
   }
 
-  function stakeFor(address _user, uint256 _amount, bytes _data) public {
-    require(!addresses[msg.sender].personalStake.exists, "Stake already exists");
-
-    require(
-      stakingToken.transferFrom(msg.sender, this, _amount),
-      "Stake required");
-
+  function stakeFor(address _user, uint256 _amount, bytes _data)
+    public
+    noExistingStake(msg.sender)
+    canStake(msg.sender, _amount)
+  {
     addresses[msg.sender].personalStake = Stake(block.number, _amount, true);
 
     // Notice here that we are increasing the staked amount for _user
@@ -115,4 +125,8 @@ contract ERC900BasicStakeContainer is ERC900 {
   function token() public view returns (address) {
     return stakingToken;
   }
+
+  function supportsHistory() public pure returns (bool) {
+    return false;
+  }
 }

contracts/ERC900/ERC900StakeContainer.sol

@@ -23,15 +23,21 @@ module.exports = async (deployer, network, accounts) => {
 
         case 'rinkeby':
           erc20TokenAddress = '0xb05e292f89c6a82f5ed1be694dc7b6444866b364'
-          initialFees = 10
+          initialFees = 10 ** 18 // 1 token
           break
 
         default:
           throw new Error('No erc20TokenAddress & initialFees defined for this network')
       }
 
       console.log(`Setting the fees to ${initialFees} at ERC-20 token address: ${erc20TokenAddress}`)
-      await proxiedCodexTitle.setFees(erc20TokenAddress, accounts[0], initialFees)
+      await proxiedCodexTitle.setFees(
+        erc20TokenAddress,
+        accounts[0],
+        initialFees, // creationFee
+        initialFees, // transferFee
+        initialFees, // modificationFee
+      )
     })
     .then(async () => {
 

migrations/6_initialize_721token.js

@@ -11,6 +11,7 @@ export default async function modifyMetadataHashes({
   expectedImageHashes = newImageHashes,
   expectedDescriptionHash = newDescriptionHash,
 
+  feesEnabled = false,
 }) {
 
   if (
@@ -22,6 +23,10 @@ export default async function modifyMetadataHashes({
     return
   }
 
+  // If fees are enabled, a Transfer event is fired in addition to the Modified event
+  const expectedLogsLength = feesEnabled ? 2 : 1
+  const logIndex = feesEnabled ? 1 : 0
+
   const { logs } = await this.token.modifyMetadataHashes(
     this.tokenId,
     newNameHash,
@@ -39,19 +44,19 @@ export default async function modifyMetadataHashes({
 
   // no Modified event is emitted when no provider details are specified
   if (!providerId && !providerMetadataId) {
-    logs.length.should.be.equal(0)
+    logs.length.should.be.equal(expectedLogsLength - 1)
     return
   }
 
-  logs.length.should.be.equal(1)
+  logs.length.should.be.equal(expectedLogsLength)
 
-  logs[0].event.should.be.eq('Modified')
-  logs[0].args._from.should.be.equal(this.creator)
-  logs[0].args._tokenId.should.be.bignumber.equal(this.tokenId)
-  logs[0].args._newNameHash.should.be.equal(tokenData[0])
-  logs[0].args._newDescriptionHash.should.be.equal(tokenData[1])
-  logs[0].args._newImageHashes.should.deep.equal(tokenData[2])
-  logs[0].args._providerId.should.be.equal(providerId)
-  logs[0].args._providerMetadataId.should.be.equal(providerMetadataId)
+  logs[logIndex].event.should.be.eq('Modified')
+  logs[logIndex].args._from.should.be.equal(this.creator)
+  logs[logIndex].args._tokenId.should.be.bignumber.equal(this.tokenId)
+  logs[logIndex].args._newNameHash.should.be.equal(tokenData[0])
+  logs[logIndex].args._newDescriptionHash.should.be.equal(tokenData[1])
+  logs[logIndex].args._newImageHashes.should.deep.equal(tokenData[2])
+  logs[logIndex].args._providerId.should.be.equal(providerId)
+  logs[logIndex].args._providerMetadataId.should.be.equal(providerMetadataId)
 
 }

test/helpers/modifyMetadataHashes.js

@@ -1,4 +1,6 @@
+import shouldBehaveLikeERC165 from './behaviors/ERC165.behavior'
 import shouldBehaveLikeCodexTitle from './behaviors/CodexTitle.behavior'
+import shouldBehaveLikeCodexTitleWithFees from './behaviors/CodexTitleFees.behavior'
 
 const { BigNumber } = web3
 const CodexTitle = artifacts.require('CodexTitle.sol')
@@ -9,10 +11,28 @@ require('chai')
   .should()
 
 contract('CodexTitle', function (accounts) {
+  const metadata = {
+    hashedMetadata: {
+      name: web3.sha3('First token'),
+      description: web3.sha3('This is the first token'),
+      images: ['asdf'].map((image) => {
+        return web3.sha3(image)
+      }),
+    },
+    providerId: '1',
+    providerMetadataId: '10',
+  }
+
   beforeEach(async function () {
     this.token = await CodexTitle.new()
     await this.token.initializeOwnable(accounts[0])
   })
 
-  shouldBehaveLikeCodexTitle(accounts)
+  shouldBehaveLikeERC165()
+
+  // Base behavior, no fees
+  shouldBehaveLikeCodexTitle(accounts, metadata)
+
+  // Extended functionality & base behavior with fees enabled
+  shouldBehaveLikeCodexTitleWithFees(accounts, metadata)
 })

test/token/CodexTitle.test.js

@@ -1,6 +1,6 @@
 import assertRevert from '../helpers/assertRevert'
-import shouldBehaveLikeERC165 from './behaviors/ERC165.behavior'
 import shouldBehaveLikeCodexTitle from './behaviors/CodexTitle.behavior'
+import shouldBehaveLikeCodexTitleWithFees from './behaviors/CodexTitleFees.behavior'
 
 const { BigNumber } = web3
 const ERC721Token = artifacts.require('ERC721TokenMock.sol')
@@ -214,6 +214,18 @@ contract('CodexTitleProxy', async function (accounts) {
   })
 
   describe('proxying CodexTitle', function () {
+    const metadata = {
+      hashedMetadata: {
+        name: web3.sha3('First token'),
+        description: web3.sha3('This is the first token'),
+        images: ['asdf'].map((image) => {
+          return web3.sha3(image)
+        }),
+      },
+      providerId: '1',
+      providerMetadataId: '10',
+    }
+
     beforeEach(async function () {
       const token = await CodexTitle.new()
       this.proxy = await CodexTitleProxy.new(token.address)
@@ -223,8 +235,8 @@ contract('CodexTitleProxy', async function (accounts) {
     })
 
     describe('should behave', function () {
-      shouldBehaveLikeERC165()
-      shouldBehaveLikeCodexTitle(accounts)
+      shouldBehaveLikeCodexTitle(accounts, metadata)
+      shouldBehaveLikeCodexTitleWithFees(accounts, metadata)
     })
   })
 })