Add change reward threshold in live contract. Closes #80

[juliangruber]

Closes #80

If the new balance is above the threshold and the participant isn't yet in the list of participants ready for transfer, add them. This is a good UX improvement, while only making setScores() a tad more expensive.

[bajtos]

Is it safe to skip the check !participantIsReadyForTransfer(participant)? I understand that oldBalance <= minBalanceForTransfer costs less gas than participantIsReadyForTransfer, which is a good optimisation.

I am struggling to understand if this is correct, though.

Situations to consider:

• minBalanceForTransfer is increased
• minBalanceForTransfer is decreased

[juliangruber]

If the min balance for transfer is increased

• participants that were below the threshold
  • nothing happens, as they are now even further below the threshold
• participants that were above the threshold before
  • and will be above the threshold after
    • they will stay in the array and not be added twice, as the situation is as if the threshold was never changed
  • and will be below the threshold after
    • they are kept in the readyForTransfer structure, and not removed. I think that's ok. They won't be added twice either (their old balance can't be below the threshold if they were already above it, and they are in the array of readyToTransfer participants)

If the min balance for transfer is decreased

• participants that were above the threshold
  • will stay in readyForTransfer, and not be removed (this code path doesn't exist)
  • will not be added twice, as
    • oldbalance <= minBalanceForTransfer is false (they were already above the previous higher threshold)
    • as is !participantIsReadyForTransfer(..) (they are already in the array)
• participants that were below the threshold
  • if they are still below the threshold, nothing happens
  • if they are now above the threshold, they are marked as ready

[bajtos]

I'll need to think about this a bit.

In the meantime, what do you think about adding tests for the different situations you described above? This would ensure that the implementation handles them exactly as it should.

[juliangruber]

great idea!

[juliangruber]

all covered in passing tests now, no contract changes necessary

[bajtos]

participants that were above the threshold before
and will be below the threshold after
they are kept in the readyForTransfer structure, and not removed. I think that's ok.

This means that we will pay rewards to these participants on the next payout cycle even though they don't meet the new threshold requirement.

I don't have a strong opinion about whether this is ok or not; I am happy to follow your decision.

I think it's worth adding a code comment to explain that this behaviour is intentional.

[juliangruber]

The comment is there in tests, to me that's good enough - if one would change the logic, this test would start failing. Wdyt?

[juliangruber]

We can fix this if it becomes a problem

[bajtos]

What do these two lines do?

Shouldn't we assert that impactEvaluator.readyForTransfer(2) returns false?

[bajtos]

If you are trying to assert that there are only 2 participants ready for transfer, then I think the following check is easier to understand and probably produces more helpful error message too.

assertEq(impactEvaluator.participantCountReadyForTransfer(), 2)

See

impact-evaluator/src/Balances.sol

Lines 21 to 23 in 087c50d

	function participantCountReadyForTransfer() public view returns (uint) {
	return readyForTransfer.length;
	}

Alternatively:

assertEq(impactEvaluator.readyForTransfer.length, 2)

[juliangruber]

What do these two lines do?

Shouldn't we assert that impactEvaluator.readyForTransfer(2) returns false?

Context is important: The line above tells the compiler that the next line should throw. This function is none that we provided, it's how Solidity exposes public arrays to contract consumers. It's effectively return array[index] but throws if index can't be found.

Here we want to assert how many participants are ready for transfer, and what their addresses are.

[bajtos]

Here we want to assert how many participants are ready for transfer, and what their addresses are.

Yes, that was my assumption too.

I am trying to say that the following code makes your intent more explicit / easier to understand, especially for people unfamiliar with solidity testing, like me.

assertEq(impactEvaluator.readyForTransfer.length, 2)

Compare that with your current version

vm.expectRevert();
impactEvaluator.readyForTransfer(2);

Anyhow, this is not a big deal; feel free to keep what you have now.

[juliangruber]

assertEq(impactEvaluator.readyForTransfer.length, 2)

This one isn't available in array getters, you can only get an element at an index

[juliangruber]

If you have anything more to add, hindsight please :)