What is the right way to track a uint256 to struct mapping?

Question

the smart contract below is a market place where users can sell items. The struct Offer, holds all the info of a specific item. To track the offers, I used a mapping with a uint256 key and created a counter variable that increments by one every time an offer is created. My question is that is this the correct way to do this? For example, if by some chance two offers are created at the same time, would they have the same id? Here is the code sample:

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

contract MarketPlace {
    uint256 public offerCount = 0;

    struct Offer {
        uint256 id;
        uint256 priceInEther;
        uint256 createdAt;
        address seller;
        address buyer;
        string itemName;
        bool isActive;
    }

    mapping(uint256 => Offer) public offers;

    event OfferCreated(uint256 id, uint256 price, uint256 createdAt);

    event OfferBought(uint256 id, address buyer, uint256 boughtAt);

    function createOffer(uint256 _priceInEther, string memory _name) external {
        require(_priceInEther > 0, "Amount must be positive");

        uint256 id = offerCount++;

        offers[id].id = id;
        offers[id].priceInEther = _priceInEther;
        offers[id].createdAt = block.timestamp;
        offers[id].seller = msg.sender;
        offers[id].itemName = _name;
        offers[id].isActive = true;

        emit OfferCreated(id, _priceInEther, block.timestamp);
    }

    function acceptOffer(uint256 _offerId) external payable {
        address buyer = msg.sender;
        require(offers[_offerId].isActive == true, "Offer is not active.");
        require(offers[_offerId].seller != buyer, "You can not buy your own product.");
        require(msg.value == offers[_offerId].priceInEther, "Please pay the item's price.");

        offers[_offerId].buyer = buyer;
        offers[_offerId].isActive = false;

        emit OfferBought(_offerId, buyer, block.timestamp);
    }
}

Any help or insight would be greatly appreciated.

Answer 1

Creating increasing ids is not a good idea, not even in regular software development. Doing this would make all your data easily indexable and exploitable by attackers.

In regular software development we generate random ids like UUID or other formats. But in smart contracts is not that easy to get random ids.

What I recommend you is to create an id derived from the data itself, from data that you know should and will not change. In your case, you can use the fields of the offer that you know will never change, encode it and create a keccak256 hash as its id, this way the id is actually related to the data and can be derived from it easily, just like how the transactions and blocks hashes are derived from the transactions and blocks themselves.

I refactored your code to handle this:

contract MarketPlace {

    struct Offer {
        bytes32 id;
        uint256 priceInWei;
        uint256 createdAt;
        address seller;
        address buyer;
        string itemName;
        bool isActive;
    }

    mapping(bytes32 => Offer) public offers;

    event OfferCreated(bytes32 id, uint256 price, uint256 createdAt);

    event OfferBought(bytes32 id, address buyer, uint256 boughtAt);

    function createOffer(uint256 _priceInWei, string memory _name) external returns(bytes32 offerHash) {

        require(_priceInWei > 0, "Amount must be positive");
        require(keccak256(abi.encodePacked(_name)) != keccak256(""), "Offer should have a valid name");

        offerHash = generateOfferHash(_priceInWei, _name);

        Offer memory offer = Offer(offerHash, _priceInWei, block.timestamp, msg.sender, address(0), _name, true);
        
        offers[offerHash] = offer;

        emit OfferCreated(offerHash, _priceInWei, block.timestamp);
    }

    function generateOfferHash(uint256 _priceInWei, string memory _name) internal view returns(bytes32) {
        // Using values from the offer that we know should and will not change, to create the offer hash (offer id).
        // If there is a value that should and will change, then we should not use it to create the offer's hash.
        return keccak256(abi.encodePacked(
            _priceInWei,
            block.timestamp,
            msg.sender,
            _name
        ));
    }

    function acceptOffer(bytes32 _offerId) external payable {
        address buyer = msg.sender;
        Offer storage offer = offers[_offerId];
        require(offer.isActive, "Offer is not active.");
        require(offer.seller != buyer, "You can not buy your own product.");
        require(msg.value == offer.priceInWei, "Please pay the item's price.");

        offer.buyer = buyer;
        offer.isActive = false;

        emit OfferBought(_offerId, buyer, block.timestamp);
    }

}

Notice now I recommend to create the Offer instance in memory first, at once, instead of accessing multiple times the storage like your code does. Creating it in memory and then writing once to storage while saving it in the mapping is more gas efficient because every time you do offers[id].property = value you read from storage which is a bit more expensive than reading from memory.

Check the opcodes list and the amount of gas they consume and notice how MLOAD (memory load) consumes less gas than SLOAD (storage load) here: https://github.com/crytic/evm-opcodes

Edit

I refactored the whole contract.

I renamed priceInEther to priceInWei to make it more clear. I added a check for the _name so you don't allow and empty string as the offer name. I refactored the acceptOffer function.

Answer 2

The point of a mapping is that the keys must always be unique - you could never have two with the same ID. You are correct though that you could have a scenario where you make an entry with an Id, which is then quickly overwritten by another entry with the same id key.

I'd suggest creating a uuid for your uint256 entry instead of incrementing a value. You can't generate uuids at the moment that easily, but you'd be safe using a value in the offer object with keccak256 hashing to generate something pretty safe to a uuid. You could add an extra check as well saying like "if the mapping key already has value - then generate and try a new uuid". Which now that I think of it, you could also do for your standard value incrementing lol. Just add a check beforehand that it's not already present.