0

I recently wrote a function that generates 5 cards and saves them to an array:

function generateCards() public {
  for (var i = 0; i < 5; i++) {
    uint card = uint(keccak256(abi.encode(block.timestamp, block.difficulty))) % 52;
    while (status[card] == true) {
      card = uint(keccak256(abi.encode(block.timestamp, block.difficulty))) % 52;
      }
    status[card] = true;
    community.push(card);
  }
}

The problem is that this function is extremely inefficient. It is basically impossible to execute...

The while loop is the main culprit but I need it to ensure that the same card isn't generated twice. Any suggestions on how to make my code more efficient?

1
  • Keccak256 returns a bytes32, so it is more than enough to take 5 bytes from ir. Just map the first byte to 0..51, the second byte to 0..50, the third to 0..49 (when counting you skip previously selected cards) and so on. Use an uint256 (256 bits) to store selected cards as a flag bitmap instead of using a mapping.
    – Ismael
    Jul 3 '20 at 15:14
0

This function is not only inefficient, it is completely useless.

It cannot generate more than a single unique card by definition, because block.timestamp is not going to change during execution (i.e., a transaction never spans over multiple blocks).

But all you really need is to change this:

abi.encode(block.timestamp, block.difficulty)

To this:

abi.encode(block.timestamp, i)

And the probability of the while loop executing more than once will drop from 100% to nearly 0%.

In depth:

  1. You don't need both block.timestamp and block.difficulty, one of them is sufficient for randomness (and block.timestamp is preferable because it is guaranteed to change over different blocks)
  2. You need the i in order to ensure with almost 100% probability that different calls to keccak256 will yield different values

BTW, different users calling this function in the same block will get the same set of cards, so you might want to extend it to abi.encode(block.timestamp, msg.sender, i).

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.