Solidity Tutorial – Best Way to Iterate Through Mapping in Solidity

nftsmart-contract-walletssoliditytruffletruffle-contract

I have recently started working on solidity. I want to create a smart contract where users can mint and own the LAND (NFT) by clicking Buy Now button on UI and send LAND Coordinates(x and y) to the contract. I have created a separate smart contract for that. Now I want to revert transaction if some user tries to buy/mint an NFT that is already minted/owned by some other address using same coordinates, so what is best way to do this?
Please check buyLand() function or go through my code and suggest me better practice to do so.
Thank You!

// SPDX-License-Identifier: MIT
pragma solidity >=0.4.22 <0.9.0;

interface ILandContract {
    function mint(address to, string memory tokenURI) external;
    function getTokenCount() external view returns(uint256);
}

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/access/Ownable.sol";

contract MintingContract is ReentrancyGuard, Ownable {

    IERC20 private Token;
    address private _landContractAddress;
    uint256 private _landPrice;
    uint256 public numberOfSold;

    struct ParcelsSold{
        uint x;
        uint y;
    }

    mapping (uint256 => ParcelsSold) coords;
    string[] private _landCoordinates;

    event LandSold(
        uint256 timestamp,
        address buyer,
        uint256 amount
    );

    constructor(address landContractAddress, IERC20 tokenAddress, uint256 initialLandPrice){
        _landContractAddress = landContractAddress;
        Token = tokenAddress;
        _landPrice = initialLandPrice;
        numberOfSold = 0;
    }

    function buyLand(string memory tokenURI, uint x, uint y) public nonReentrant {

        for(uint i=0; i< numberOfSold; i++){
            require(!(coords[i].x == x && coords[i].y == y), "Minting : Parcel Sold Already");
        }
        Token.transferFrom(msg.sender, owner(), _landPrice);
        ILandContract land = ILandContract(_landContractAddress);
        land.mint(msg.sender, tokenURI);
        numberOfSold++;

        uint256 tokenId = land.getTokenCount();

        ParcelsSold memory sold = ParcelsSold(x, y);
        coords[tokenId] = sold;

        emit LandSold(block.timestamp, msg.sender, _landPrice);
    }

}

Best Answer

This for(uint i=0; i< numberOfSold is an unbounded loop and is a big no-no. Eventually, the cost of 1 transaction will be over the block gas limit, and it will revert. At this point buyLand would be bricked.

Try:

//       x,                  y
mapping (uint256 => mapping (uint256 => ParcelInfo)) parcels;

You cannot enumerate through a mapping. And you don't want to, again, because that would be an unbounded loop. However, you may want to keep track of the plots sold, for instance with a:

// Token ids perhaps?
uint256[] allSold;

But never have your Solidity code do a for on it. This increases the complexity of your contract. So think hard about what is needed in your contract, and what can be outsourced to Web2.0 servers.

To improve: you do land.mint and then tokenId = land.getTokenCount(). That does not sound right. Right now you are saved by the fact that Ethereum is "single-thread". Instead, the mint should return the new tokenId, something like:

uint256 tokenId = land.mint(msg.sender, tokenURI);
Related Topic