1
struct Locker {
  uint creationTime;
  uint holdTime;
  uint balance;
}

mapping (address => Locker[]) lockersByAddress;

function withdrawAll() public {
  Locker[] storage lockers = lockersByAddress[msg.sender];
  for (uint i = 0; i < lockers.length; i++) {
    if (lockers[i].creationTime + lockers[i].holdTime < now) {
      msg.sender.transfer(lockers[i].balance);
      Withdrawal(msg.sender, lockers[i].balance);
      delete lockers[i];
    }
  }
}

At the moment, running withdrawAll successfully transfers the specified funds and triggers the Withdrawal event, but doesn't delete the Locker struct from the array. The same Locker is still available after running the function.

I assumed that using the storage keyword in my lockers declaration would create a pointer to the array of Locker structs in-memory, but then why isn't delete doing anything?

Any help would be greatly appreciated.

0

I assumed that using the storage keyword in my lockers declaration would create a pointer to the array of Locker structs in-memory, but then why isn't delete doing anything?

It is creating a pointer, but the problem here is that you are zeroing out the data in the struct, but not actually removing it from the element from the array.

I took the liberty of extrapolating your code to be the following to demonstrate this:


contract Lockers {
    struct Locker {
      uint creationTime;
      uint holdTime;
      uint balance;
   }

   mapping (address => Locker[]) lockersByAddress;

   event Withdrawal(address sender, uint balance);

    function store(uint holdTime) external payable {
        Locker memory locker = Locker(now, holdTime, msg.value);
        lockersByAddress[msg.sender].push(locker);
    }

    function withdrawAll() public {
      Locker[] storage lockers = lockersByAddress[msg.sender];
      for (uint i = 0; i < lockers.length; i++) {
        if (lockers[i].creationTime + lockers[i].holdTime < now) {
          msg.sender.transfer(lockers[i].balance);
          Withdrawal(msg.sender, lockers[i].balance);
          delete lockers[i];
        }
      }
    }

    function getNumLockers(address owner) external view returns(uint) {
        return lockersByAddress[owner].length;
    }

    function getLockerDetails(address owner, uint index) external view returns(uint creationTime, uint holdTime, uint balance) {
        Locker memory locker = lockersByAddress[owner][index];
        creationTime = locker.creationTime;
        holdTime = locker.holdTime;
        balance = locker.balance;
    }
}

In Remix, I 'stored' a Locker element and then called your withdrawAll function.

What I saw when I inspected the state of the contract using my getNumLockers and getLockerDetails functions was that the locker Struct still existed; just will all of the data zeroed out.

Before calling withdrawAll: enter image description here

After calling withdrawAll: enter image description here

What you are actually trying to achieve based on the function name (deleting all of the data/structs in the array) can be achieved as so:


function withdrawAll2() public {
      Locker[] memory memLockers = lockersByAddress[msg.sender];
      // Avoid any re-entrancy issues
      delete lockersByAddress[msg.sender];
      for (uint i = 0; i < memLockers.length; i++) {
        if (memLockers[i].creationTime + memLockers[i].holdTime < now) {
          msg.sender.transfer(memLockers[i].balance);
          Withdrawal(msg.sender, memLockers[i].balance);
        }
      }
    }

Notes:

  1. It's a bad practice to update the state after a transfer because of the potential for reentrancy attacks, so I've moved the delete to before transfers (which, therefore, required the need to store the array of Lockers in memory).
  2. You should be aware that if the array of lockers grows too large that it might take more gas than is allowed in a block to complete the withdrawAll function - preventing a user from being able to withdraw their funds.
  3. If one locker can't have the funds withdrawn at the time then it isn't possible to "withdrawAll" and this causes problems both in your implementation and the one that I've suggested - more thought should probably be put into the design and how you would deal with empty values at an array index (and how you would deal with working with arrays in a cost effective way).
0

Locker[] storage lockers = lockersByAddress[msg.sender] is creating a local Locker array in the scope of the function withdrawAll , deleting lockers[i] only delete the element in the scope of the function, but not in the mapping.

Try using lockersByAddress[msg.sender][i] instead of lockers[i] in your loop.

You could also create an internal function getLockers() and return the Locker[] array as storage so it would act like a pointer storage

0

@steveJaxon nailed it. In addition, @Saul: you may also need to prevent the long loop if lockers array grows bigger by adding a limit to the number of lockers to withdraw at a time. Yes it may cost your users gas by calling the method multiple times maybe but it's way better than not being able to execute the method at all when the array becomes bigger than the gas allowed for a block. However, I hope we can see more solid solutions for this long loop issues from Ethereum Team instead of putting everything on the developer head to figure out.

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.