Voting power refactor : 1 cycle delay

[RonTuretzky]

See #56

I think the problem is that the most recent checkpoint block number could be higher than the block number at end of calculation period.

Yea I think I see the problem. The function wasn't designed with this in mind and the test don't cover it. Essentially if you minted after the end of the previous cycle this will fail.

[RonTuretzky]

@subject026 please pull and try again, I think this should be addressed with d4809e3

Also I added some comments to the code if you would like to read through, should be more understandable now

[bagelface]

imo there are way too many comments added to this and it's making the code messy. The code should be mostly self-commenting with exceptions made for bits of logic that are especially convoluted or require context external to the code. Overly simplistic comments are OK for Natspec comments only because they're meant to be parsed and displayed external to the source code like on Etherscan forms.

this is doing way too much

        // Check if the start time is before the end time
        if (_start > _end) revert StartMustBeBeforeEnd();
        // Check if the end time is after the current block
        if (_end > Time.blockNumber()) revert EndAfterCurrentBlock();
        // Get the latest checkpoint for the user
        uint32 latestCheckpointPos = BREAD.numCheckpoints(_account);
        // Check if the user has ever held $BREAD
        if (latestCheckpointPos == 0) revert NoCheckpointsForAccount();
        // Check if the user has ever held $BREAD in the interval by comparing the first mint to the end of the interval
        if (BREAD.checkpoints(_account, 0)._key > _end) return 0;
        // Starting to filter out irrelevant checkpoints that are not in the interval

[bagelface]

Is there a reason we're going through the trouble of converting everything between "timestamps" and "block numbers" instead of just using block numbers and approximating the duration of a cycle to be like 518400 blocks (roughly 30 days assuming Gnosis's 5 second block times)?

[RonTuretzky]

imo there are way too many comments added to this and it's making the code messy. The code should be mostly self-commenting with exceptions made for bits of logic that are especially convoluted or require context external to the code. Overly simplistic comments are OK for Natspec comments only because they're meant to be parsed and displayed external to the source code like on Etherscan forms.

this is doing way too much

        // Check if the start time is before the end time
        if (_start > _end) revert StartMustBeBeforeEnd();
        // Check if the end time is after the current block
        if (_end > Time.blockNumber()) revert EndAfterCurrentBlock();
        // Get the latest checkpoint for the user
        uint32 latestCheckpointPos = BREAD.numCheckpoints(_account);
        // Check if the user has ever held $BREAD
        if (latestCheckpointPos == 0) revert NoCheckpointsForAccount();
        // Check if the user has ever held $BREAD in the interval by comparing the first mint to the end of the interval
        if (BREAD.checkpoints(_account, 0)._key > _end) return 0;
        // Starting to filter out irrelevant checkpoints that are not in the interval

addressed by 2e9c094

[RonTuretzky]

Is there a reason we're going through the trouble of converting everything between "timestamps" and "block numbers" instead of just using block numbers and approximating the duration of a cycle to be like 518400 blocks (roughly 30 days assuming Gnosis's 5 second block times)?

addressed by 9e6a03a @bagelface