Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add change reward threshold in live contract. Closes #80 #81

Merged
merged 9 commits into from
Aug 1, 2024
Merged

Add change reward threshold in live contract. Closes #80 #81

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6427653
Add change reward threshold in live contract. Closes #80
juliangruber Jul 29, 2024
cb1b59b
add test for increase threshold
juliangruber Jul 30, 2024
94fc92f
test if even score=0 updates participants
juliangruber Jul 30, 2024
69aea2c
add passing tests
juliangruber Jul 31, 2024
4e94211
add passing tests
juliangruber Jul 31, 2024
6a8bd2b
add passing tests
juliangruber Jul 31, 2024
aef6647
add passing tests
juliangruber Jul 31, 2024
4038dd6
add passing tests
juliangruber Jul 31, 2024
0c25798
fix test
juliangruber Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ will be called and the round finishes.

### `.balanceOf(address account) view -> uint`

### `.participantIsReadyForTransfer(address account) view -> bool`

## Roles

### `.EVALUATE_ROLE()`
Expand Down
16 changes: 14 additions & 2 deletions src/Balances.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,28 @@ contract Balances {
return scheduledForTransfer.length;
}

function participantIsReadyForTransfer (address participant) public view returns (bool) {
for (uint i = 0; i < readyForTransfer.length; i++) {
if (readyForTransfer[i] == participant) {
return true;
}
}
return false;
}

function increaseParticipantBalance(
address payable participant,
uint amount
) internal {
uint oldBalance = balances[participant];
uint newBalance = oldBalance + amount;
balances[participant] = newBalance;
if (newBalance <= minBalanceForTransfer) {
bajtos marked this conversation as resolved.
Show resolved Hide resolved
return;
}
if (
oldBalance <= minBalanceForTransfer &&
newBalance > minBalanceForTransfer
oldBalance <= minBalanceForTransfer
|| !participantIsReadyForTransfer(participant)
Comment on lines +49 to +50
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great idea!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all covered in passing tests now, no contract changes necessary

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can fix this if it becomes a problem

) {
readyForTransfer.push(participant);
}
Expand Down
39 changes: 39 additions & 0 deletions test/ImpactEvaluator.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,30 @@ contract ImpactEvaluatorTest is Test {
impactEvaluator.setScores(0, addresses, scores);
}

function test_SetScoresAfterUpdateMinBalanceForTransfer() public {
juliangruber marked this conversation as resolved.
Show resolved Hide resolved
ImpactEvaluator impactEvaluator = new ImpactEvaluator(address(this));
vm.deal(payable(address(impactEvaluator)), 100 ether);

impactEvaluator.adminAdvanceRound();
impactEvaluator.adminAdvanceRound();

address payable[] memory addresses = new address payable[](2);
addresses[0] = payable(vm.addr(1));
addresses[1] = payable(0x000000000000000000000000000000000000dEaD);
uint[] memory scores = new uint[](2);
scores[0] = 1;
scores[1] = impactEvaluator.MAX_SCORE() - scores[0];

impactEvaluator.setScores(1, addresses, scores);
assert(!impactEvaluator.participantIsReadyForTransfer(addresses[0]));

impactEvaluator.setMinBalanceForTransfer(0);
impactEvaluator.adminAdvanceRound();

impactEvaluator.setScores(2, addresses, scores);
juliangruber marked this conversation as resolved.
Show resolved Hide resolved
assert(impactEvaluator.participantIsReadyForTransfer(addresses[0]));
}

function test_AdvanceRoundCleanUp() public {
ImpactEvaluator impactEvaluator = new ImpactEvaluator(address(this));

Expand Down Expand Up @@ -691,4 +715,19 @@ contract ImpactEvaluatorTest is Test {

assertEq(vm.addr(1).balance, 0);
}

function test_ParticipantIsReadyForTransfer() public {
ImpactEvaluator impactEvaluator = new ImpactEvaluator(address(this));
address payable[] memory addresses = new address payable[](2);
addresses[0] = payable(vm.addr(1));
addresses[1] = payable(vm.addr(2));
uint[] memory balances = new uint[](2);
balances[0] = 50 ether;
balances[1] = 50 ether;
impactEvaluator.addBalances{ value: 100 ether }(addresses, balances);

assert(impactEvaluator.participantIsReadyForTransfer(addresses[0]));
assert(impactEvaluator.participantIsReadyForTransfer(addresses[1]));
assert(!impactEvaluator.participantIsReadyForTransfer(payable(vm.addr(3))));
}
}
Loading