Revert on non-fungible token, when sending external call from second contract

Question

Thanks for your time.

I'm working with a Non-fungible token. I am trying to call the non-fungible token contract from a second contract called agContract.

I can create and track the owners of the tokens I create in agContract using createNFT(), but I cannot call transfer() within the nonfungibletoken contract from my agcontract. I get reverted during the _clearApprovalAndTransfer(_from, _to, _tokenId); line after using batchLeavesFacility() function to try to call my nonfungibletoken

Code is below:

pragma solidity ^0.4.18;
contract ERC721 {
    // Function
    function totalSupply() public returns (uint256 _totalSupply);
    function balanceOf(address _owner) public view returns (uint256 _balance);
    function ownerOf(uint _tokenId) public view returns (address _owner);
    function approve(address _to, uint _tokenId) public;
    function getApproved(uint _tokenId) public view returns (address _approved);
    function transferFrom(address _from, address _to, uint _tokenId) public;
    function transfer(address _to, uint _tokenId) public;
    function implementsERC721() public view returns (bool _implementsERC721);

    // Events
    event Transfer(address indexed _from, address indexed _to, uint256 _tokenId);
    event Approval(address indexed _owner, address indexed _approved, uint256 _tokenId);
}

/**
 * Interface for optional functionality in the ERC721 standard
 * for non-fungible tokens.
 *
 * Author: Nadav Hollander (nadav at dharma.io)
 */
contract DetailedERC721 is ERC721 {
    function name() public view returns (string _name);
    function symbol() public view returns (string _symbol);
    function tokenMetadata(uint _tokenId) public view returns (string _infoUrl);
    function tokenOfOwnerByIndex(address _owner, uint _index) public view returns (uint _tokenId);
}


/**
 * @title NonFungibleToken
 *
 * Generic implementation for both required and optional functionality in
 * the ERC721 standard for non-fungible tokens.
 *
 * Heavily inspired by Decentraland's generic implementation:
 * https://github.com/decentraland/land/blob/master/contracts/BasicNFT.sol
 *
 * Standard Author: dete
 * Implementation Author: Nadav Hollander <nadav at dharma.io>
 */
contract NonFungibleToken is DetailedERC721 {
    string public name;
    string public symbol;
    address public owner;
    address public agServiceContract;
    uint256 public numTokensTotal;

    mapping(uint => address) public tokenIdToOwner;
    mapping(uint => address) public tokenIdToApprovedAddress;
    mapping(uint => string) public tokenIdToMetadata;
    mapping(address => uint[])  public ownerToTokensOwned;
    mapping(uint => uint)public  tokenIdToOwnerArrayIndex;

    event Transfer(address indexed _from, address indexed _to, uint256 _tokenId
    );

    event Approval( address indexed _owner, address indexed _approved, uint256 _tokenId
    );

    modifier onlyExtantToken(uint _tokenId) {
        require(ownerOf(_tokenId) != address(0));
        _;
    }
    function NonFungibleToken(){
        owner = msg.sender;
    }

    function name()
        public
        view
        returns (string _name)
    {
        return name;
    }

    function symbol()
        public
        view
        returns (string _symbol)
    {
        return symbol;
    }

    function totalSupply()
        public

        returns (uint256 _totalSupply)
    {
        return numTokensTotal;
    }

    function balanceOf(address _owner)
        public
        view
        returns (uint _balance)
    {
        return ownerToTokensOwned[_owner].length;
    }
    function setAgContract(address addr) {
        require(msg.sender == owner);
        agServiceContract = addr;
    }

    function ownerOf(uint _tokenId)
        public
        view
        returns (address _owner)
    {
        return _ownerOf(_tokenId);
    }

    function tokenMetadata(uint _tokenId)
        public
        view
        returns (string _infoUrl)
    {
        return tokenIdToMetadata[_tokenId];
    }

    function approve(address _to, uint _tokenId)
        public
        onlyExtantToken(_tokenId)
    {
        require(msg.sender == ownerOf(_tokenId));
        require(msg.sender != _to);

        if (_getApproved(_tokenId) != address(0) ||
                _to != address(0)) {
            _approve(_to, _tokenId);
            Approval(msg.sender, _to, _tokenId);
        }
    }

    function transferFrom(address _from, address _to, uint _tokenId)
        public
        onlyExtantToken(_tokenId)
    {
        require(getApproved(_tokenId) == msg.sender);
        require(ownerOf(_tokenId) == _from);
        require(_to != address(0));

        _clearApprovalAndTransfer(_from, _to, _tokenId);

        Approval(_from, 0, _tokenId);
        Transfer(_from, _to, _tokenId);
    }

    function transfer(address _to, uint _tokenId)
        public
        onlyExtantToken(_tokenId)
    {
        /* used to be require(ownerOf(_tokenId) == msg.sender) but still reverted */
        require(ownerOf(_tokenId) == msg.sender);
        require(_to != address(0));
        _clearApprovalAndTransfer(msg.sender, _to, _tokenId);

        Approval(msg.sender, 0, _tokenId);
        Transfer(msg.sender, _to, _tokenId);
    }
    function transferAgContract(address _to, address _from, uint _tokenId)
        public
        onlyExtantToken(_tokenId)
    {
        /* used to be require(ownerOf(_tokenId) == msg.sender) but still reverted */
        require(ownerOf(_tokenId) == msg.sender || agServiceContract == msg.sender);
        require(_to != address(0));
        _clearApprovalAndTransfer(_from, _to, _tokenId);

        Approval(msg.sender, 0, _tokenId);
        Transfer(msg.sender, _to, _tokenId);
    }

    function tokenOfOwnerByIndex(address _owner, uint _index)
        public
        view
        returns (uint _tokenId)
    {
        return _getOwnerTokenByIndex(_owner, _index);
    }

    function getOwnerTokens(address _owner)
        public
        view
        returns (uint[] _tokenIds)
    {
        return _getOwnerTokens(_owner);
    }

    function implementsERC721()
        public
        view
        returns (bool _implementsERC721)
    {
        return true;
    }

    function getApproved(uint _tokenId)
        public
        view
        returns (address _approved)
    {
        return _getApproved(_tokenId);
    }

    function _clearApprovalAndTransfer(address _from, address _to, uint _tokenId)
        internal
    {
        _clearTokenApproval(_tokenId);
        _removeTokenFromOwnersList(_from, _tokenId);
        _setTokenOwner(_tokenId, _to);
        _addTokenToOwnersList(_to, _tokenId);
    }

    function _ownerOf(uint _tokenId)
        internal
        view
        returns (address _owner)
    {
        return tokenIdToOwner[_tokenId];
    }

    function _approve(address _to, uint _tokenId)
        internal
    {
        tokenIdToApprovedAddress[_tokenId] = _to;
    }

    function _getApproved(uint _tokenId)
        internal
        view
        returns (address _approved)
    {
        return tokenIdToApprovedAddress[_tokenId];
    }

    function _getOwnerTokens(address _owner)
        internal
        view
        returns (uint[] _tokens)
    {
        return ownerToTokensOwned[_owner];
    }

    function _getOwnerTokenByIndex(address _owner, uint _index)
        internal
        view
        returns (uint _tokens)
    {
        return ownerToTokensOwned[_owner][_index];
    }

    function _clearTokenApproval(uint _tokenId)
        internal
    {
        tokenIdToApprovedAddress[_tokenId] = address(0);
    }

    function _setTokenOwner(uint _tokenId, address _owner)
        internal
    {
        tokenIdToOwner[_tokenId] = _owner;
    }

    function _addTokenToOwnersList(address _owner, uint _tokenId)
        internal
    {
        ownerToTokensOwned[_owner].push(_tokenId);
        tokenIdToOwnerArrayIndex[_tokenId] =
            ownerToTokensOwned[_owner].length - 1;
    }

    function _removeTokenFromOwnersList(address _owner, uint _tokenId)
        internal
    {
        uint length = ownerToTokensOwned[_owner].length;
        uint index = tokenIdToOwnerArrayIndex[_tokenId];

        uint swapToken = ownerToTokensOwned[_owner][length - 1];




        ownerToTokensOwned[_owner][index] = swapToken;
        tokenIdToOwnerArrayIndex[swapToken] = index;

        delete ownerToTokensOwned[_owner][length - 1];
        ownerToTokensOwned[_owner].length--;
    }

    function _insertTokenMetadata(uint _tokenId, string _metadata)
        internal
    {
        tokenIdToMetadata[_tokenId] = _metadata;
    }
}

/**
 * @title SafeMath
 * @dev Math operations with safety checks that throw on error
 */
library SafeMath {

  /**
  * @dev Multiplies two numbers, throws on overflow.
  */
  function mul(uint256 a, uint256 b) internal pure returns (uint256) {
    if (a == 0) {
      return 0;
    }
    uint256 c = a * b;
    assert(c / a == b);
    return c;
  }

  /**
  * @dev Integer division of two numbers, truncating the quotient.
  */
  function div(uint256 a, uint256 b) internal pure returns (uint256) {
    // assert(b > 0); // Solidity automatically throws when dividing by 0
    uint256 c = a / b;
    // assert(a == b * c + a % b); // There is no case in which this doesn't hold
    return c;
  }

  /**
  * @dev Substracts two numbers, throws on overflow (i.e. if subtrahend is greater than minuend).
  */
  function sub(uint256 a, uint256 b) internal pure returns (uint256) {
    assert(b <= a);
    return a - b;
  }

  /**
  * @dev Adds two numbers, throws on overflow.
  */
  function add(uint256 a, uint256 b) internal pure returns (uint256) {
    uint256 c = a + b;
    assert(c >= a);
    return c;
  }
}
/**
 * @title MintableNonFungibleToken
 *
 * Superset of the ERC721 standard that allows for the minting
 * of non-fungible tokens.
 */
contract MintableNonFungibleToken is NonFungibleToken {
    using SafeMath for uint;

    event Mint(address indexed _to, uint256 indexed _tokenId);

    modifier onlyNonexistentToken(uint _tokenId) {
        require(tokenIdToOwner[_tokenId] == address(0));
        _;
    }

    function mint(address _owner, uint256 _tokenId, address _approvedAddress, string _metadata)
        public
        onlyNonexistentToken(_tokenId)
    {
        _setTokenOwner(_tokenId, _owner);
        _addTokenToOwnersList(_owner, _tokenId);
        _approve(_approvedAddress, _tokenId);
        _insertTokenMetadata(_tokenId, _metadata);

        numTokensTotal = numTokensTotal.add(1);

        Mint(_owner, _tokenId);
    }
}
contract
agContract {
 using SafeMath for uint;
    address public owner;
    MintableNonFungibleToken nft;
    modifier onlyOwner(){
        require(msg.sender == owner);
        _;
    }
   function agContract(address _nftContract){
        owner = msg.sender;
        nft = MintableNonFungibleToken(_nftContract);
    }
function createNFT(uint256 _weight, address _owner, string _name){
        nft.mint(_owner, readNFT(), _owner, _name); 
    }
 function batchLeavesFacility(uint _tokenId, address _to, uint256 _weight){

        nft.transferAgContract(_to, msg.sender, _tokenId);
    }
 function readNFT() internal returns(uint)  {
        return nft.totalSupply();
    }


}

So once I create the mintabletoken contract and the agcontract, I set the addresses of both. Then I createNFT() and transfer it to Account 1. Account 1 then tries to transfer token 0 using batchLeavesFacility(), to account 2, but gets reverted from nft.transfer().

Debug stops here, but I'm not sure why, it should have an updated array of tokenOwners when the token is minted. Can someone see what I'm missing here? Thanks!

   function _ownerOf(uint _tokenId)
        internal
        view
        returns (address _owner)
    {
        return tokenIdToOwner[_tokenId];
    }

Also here

  function transfer(address _to, uint _tokenId)
        public
        onlyExtantToken(_tokenId)
    {

        require((ownerOf(_tokenId) == msg.sender));
        require(_to != address(0));

        _clearApprovalAndTransfer(msg.sender, _to, _tokenId);

        Approval(msg.sender, 0, _tokenId);
        Transfer(msg.sender, _to, _tokenId);
    }

edit: So it looks like the function _removeTokenFromOwnersList() is the problem but I'm not sure whats going on. when calling ownerToTokensOwned[_owner][length -1] throws since the owernToTokensOwned[] is only 1 element long. Can someone explain further? I included a if/else statement to remove the length-1 but it still throws.

_removeTokenFromOwnersList(address _owner, uint _tokenId)
        internal
    {
        uint length = ownerToTokensOwned[_owner].length;
        uint index = tokenIdToOwnerArrayIndex[_tokenId];
        if(length >= 1){
    uint swapToken = ownerToTokensOwned[_owner][length - 1];
    }else{
       swapToken = ownerToTokensOwned[_owner][length];
    }


        ownerToTokensOwned[_owner][index] = swapToken;
        tokenIdToOwnerArrayIndex[swapToken] = index;

        delete ownerToTokensOwned[_owner][length - 1];
        ownerToTokensOwned[_owner].length--;
    }

Answer 1

When calling BatchLeavesFacility() the msg.sender was the AgContract, not the token owner. So I had to create agTransfer() to pass in the msg.sender and allow for the transfer to go through.

 function transferAgContract(address _to, address _from, uint _tokenId)
            public
            onlyExtantToken(_tokenId)
        {   require(agServiceContract == msg.sender);
            /* used to be require(ownerOf(_tokenId) == msg.sender) but still reverted */
            require(ownerOf(_tokenId) == _from);
            require(_to != address(0));
            _clearApprovalAndTransfer(_from, _to, _tokenId);

            Approval(msg.sender, 0, _tokenId);
            Transfer(msg.sender, _to, _tokenId);
        }