1

I have a dapp where an admin can upload a number of cakes (20 to 30). People can come into the site and decide whether they want to buy a cake uploaded by admin.

So for smart contract, I made an array of uint to store each id of Cake whenever admin uploads. Once the buyer bought one of the cakes, it would delete that id of Cake in the array. I decided to delete it to avoid loops as much as possible since it would cause out of gas while I iterate the array to return the remaining list of cakes to buyers. How does it sound in terms of optimization?

Below is my implementation. Are there any better solutions or is this enough to continue?

struct Cake {
    uint id;
    address buyer;
}

mapping (uint => Cake) public cakes;
uint[] public cakeIds;

function addNewCake(uint _id) public {
    cakes[_id] = Cake(_id, 0x0);

    cakeIds.push(_id);
}

function buyCake(uint _id) public {      
    Cake storage cake = cakes[_id];       
    cake.buyer = msg.sender;

    removeCakeInArray(_id);
}

function removeCakeInArray(uint _id) private {
    for (uint i = 0; i <= getNumOfCakes(); i++) {            
        if (cakeIds[i] == _id) {
            remove(i);                 
        }    
    }
}

function remove(uint index) private {
    if (index >= getNumOfCakes()) return;

    for (uint i = index; i < getNumOfCakes() - 1; i++){
        cakeIds[i] = cakeIds[i+1];
    }
    cakeIds.length--;
}

function getAllUnsoldCakes() public view returns (uint[]) {
    uint length = getNumOfCakes();  
    uint[] memory ids = new uint[](length);

    for (uint i = 0; i < length; i++) {       
        uint cakeId = cakeIds[i];
        Cake memory cake = cakes[cakeId];

        if (cake.buyer == 0x0) {
            ids[i] = cake.id;             
        }
    }

    return ids;
}

function getNumOfCakes() public view returns (uint) {
    return cakeIds.length;
}
1

These loops, particularly in remove(), are a red flag: Your array can grow to an unbounded length, including if someone increases it maliciously, at which your contract will hit gas limits and become unusable. This is only OK if you can put explicit bounds on the length of the list - for example if someone tries to add an 11th unsold cake, revert the transaction, because the cake shop shelves are full.

If you really need to be able to retrieve a list of unsold cakes with a solidity function call, a safer approach is to use a linked list, as this can be updated for the same cost regardless of the length of the list. This still gives you a gas cost to worry about when you read the list, but that may be OK if it's only going to be used reading data from a node, rather than being called as part of a transaction.

However, do you really need to be able to get a list of unsold cakes with a contract function call? It may be better to transfer that work elsewhere - for example, log events when you add or sell a cake, then read those events from your dapp javascript and use that to build the list you display to the user.

| improve this answer | |
  • Thanks for your comments. I'm not sure why remove() could be a red flag. What are the scenarios someone can increase an array on purpose? – bbusdriver Jun 17 '18 at 0:32
  • 1
    I guess ifaddNewCake() is restricted to admin (which in your code it isn't) and they're completely trusted and the whole contract is broken anyhow their key is stolen then you're OK. If any of these things doesn't hold, a malicious user can just call addNewCake() until the array is long enough that the loop in remove()uses more gas than the block gas limit. – Edmund Edgar Jun 17 '18 at 0:55
  • Makes sense. Also, for calling the unsold cakes, I'm thinking to use .json files in the first place to display all the cakes and mark it as sold if the cake is sold by a buyer (simply store the buyer's address and cake id in the blockchain). Would this make it smoother? – bbusdriver Jun 17 '18 at 1:15
  • Sold or not affects whether the transaction goes through so I'd probably do that part on the blockchain, but putting any human-readable details (name of cake etc) in json files on ipfs and storing the ipfs address on the blockchain may well make sense. – Edmund Edgar Jun 17 '18 at 22:36
1

Seconding Edmond's concerns about the loops. In my opinion, the need to enumerate the list is questionable. The contract could work on an append-only basis with a simple flag to indicate "sold" and events that chronical additions and removals. Clients would then be able to compute, for themselves, which cakes remain available for sale and the contract would merely enforce a rule that no cake can be sold twice. Similarly, ordering lists can usually be external to storage contracts.

In the case that another contract might be interested in successfully navigating the list, you might need to make this information efficiently discoverable without reliance on events. This would include the "delete" concern.

In that case, consider the Mapped Struct with Delete pattern described over here: Are there well-solved and simple storage patterns for Solidity? with an elaborated description over here: https://medium.com/@robhitchens/solidity-crud-part-1-824ffa69509a

Hope it helps.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.