fix: avoid revocation overload

[kilted-andres]

#fixes #2982

[arty-name]

I don’t think increasing the interval or decreasing the batch size would have any positive effect, but at least the critical change is in place :)

[lukeg90]

I'm not a fan of all the if/else statements. What do you think of this?

for await (const expiredAttestation of getExpiredAttestations()) {
    // decides in which list to put and makes sure that is not included yet
    const targetList = shouldBeRemoved(expiredAttestation)
      ? attestationsToRemove
      : expiredAttestation.revoked === false
      ? attestationsToRevoke
      : attestationsToRemoveLater;

    if (isNotIncludedYetOn(targetList, expiredAttestation)) {
      targetList.push(expiredAttestation);
    }
  }

[arty-name]

Some attestations need to be pushed in both attestationsToRemoveLater and attestationsToRevoke, so this approach won’t work. I think the code duplication could be reduced by creating a function addIfMissing() to do the conditional pushes.

[lukeg90]

Oh, I overlooked that.

[arty-name]

If we combine two checks inside this if, some attestations that should be removed will instead be handled in the else branch, as if they were to revoke.

[arty-name]

I make similar mistakes occasionally, and I don’t know of a good way to avoid them

[kilted-andres]

Oh, you are right. Nice catch! Thanks.

It needs nested ifs then.

[kilted-andres]

Did it like this:

isNotIncludedYetOn(attestationsToRemove, expiredAttestation) && attestationsToRemove.push(expiredAttestation);

inside of the ifs

[arty-name]

I think the code duplication could be reduced by creating a function addIfMissing() to do the conditional pushes.

What do you think of this suggestion?

[kilted-andres]

It could be done.

Yeah, ok. I'll do it.