Unreachable condition in checkSpenderTransfer

[adria0]

When a recipient is removed (removeAuthorizedRecipient), then

   s.addr2recipientId[_recipient] = 0

and

  s.recipients[idRecipient].activationTime = 0;

So, when in checkSpenderTransfer the check always returns false

    uint idRecipient = spender.addr2recipientId[_recipient];
    if (idRecipient == 0) return false; // already unauthorized
    idRecipient--;

and this condition is never executed (so can be removed):

    Recipient r = spender.recipients[idRecipient];
    if (r.activationTime == 0) {
        return false;
    }

[GriffGreen]

some of these "unreachable conditions" are not bad to have in the contract right? they just use a little extra gas but can act as sort of idiot checks...?

This one is probably not the best example, but in general... Are these bad to leave in for any other reason?

I think for critical functions having extra checks that may not be possible could be useful as sort of assertions like how we use safeMath?

[adria0]

@GriffGreen IMO each LoC - including of course paranoic checks - needs a reason to be there. So if we doing an extra check we need to explain the reason. If not, we are creating paranoia to code readers ("I need to put this extra check but I cannot explain why")

[sophiii]

@adriamb disagree. I think we should include some extra checks in the contract code.

They are called invariants which makes sure the state of the contract cannot enter a certain state. The point of them is that if we make a mistake in contract logic somewhere. Or there is a security bug. The invariants should mitigate if not prevent exploitation. With smart contract we need to develop defensively.

IMO a contract that controls ETH should include at least two different checks when balances are changed that certin rules are not broken.

This is my philosophy on keeping code lean Vrs extra checking

[adria0]

@sophii please, read again. I say that if you do it you need to explain why.

[GriffGreen]

@adriamb I couldn't agree more with each line of code needing justification.

I think the justification for redundant checks would be: A failure to prevent this occurrence would cause a loss of funds, so a redundant check is warranted