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 all commits
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
186 changes: 167 additions & 19 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,149 @@ contract ImpactEvaluatorTest is Test {
impactEvaluator.setScores(0, addresses, scores);
}

function test_SetScoresAfterDecreaseMinBalanceForTransfer() public {
ImpactEvaluator impactEvaluator = new ImpactEvaluator(address(this));
vm.deal(payable(address(impactEvaluator)), 100 ether);

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

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

impactEvaluator.setScores(1, addresses, scores);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(1)),
false,
"participant 0 is not ready"
);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(2)),
true,
"participant 1 is ready"
);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(3)),
false,
"participant 2 is not ready"
);

impactEvaluator.setMinBalanceForTransfer(1);
impactEvaluator.adminAdvanceRound();

address payable[] memory addresses2 = new address payable[](4);
addresses2[0] = payable(vm.addr(1));
addresses2[1] = payable(vm.addr(2));
addresses2[2] = payable(vm.addr(3));
addresses2[3] = payable(0x000000000000000000000000000000000000dEaD);
uint[] memory scores2 = new uint[](4);
scores2[0] = 0;
scores2[1] = 0;
scores2[2] = 0;
scores2[3] = impactEvaluator.MAX_SCORE();
impactEvaluator.setScores(2, addresses2, scores2);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(1)),
true,
"participant 0 is now ready"
);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(2)),
true,
"participant 1 is still ready"
);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(3)),
false,
"participant 2 is still not ready"
);
assertEq(impactEvaluator.readyForTransfer(0), vm.addr(2));
assertEq(impactEvaluator.readyForTransfer(1), vm.addr(1));
vm.expectRevert();
impactEvaluator.readyForTransfer(2);
Comment on lines +297 to +298
Copy link
Contributor

Choose a reason for hiding this comment

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

What do these two lines do?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

assertEq(impactEvaluator.readyForTransfer.length, 2)

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

}

function test_SetScoresAfterIncreaseMinBalanceForTransfer() public {
ImpactEvaluator impactEvaluator = new ImpactEvaluator(address(this));
vm.deal(payable(address(impactEvaluator)), 100 ether);

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

address payable[] memory addresses = new address payable[](3);
addresses[0] = payable(vm.addr(1));
addresses[1] = payable(vm.addr(2));
addresses[2] = payable(vm.addr(3));
uint[] memory scores = new uint[](3);
// The minimum score required to be ready for transfer
scores[0] =
((impactEvaluator.MAX_SCORE() *
impactEvaluator.minBalanceForTransfer()) /
impactEvaluator.roundReward()) +
1;
scores[1] = 0;
scores[2] = impactEvaluator.MAX_SCORE() - scores[0];

impactEvaluator.setScores(1, addresses, scores);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(1)),
true,
"participant 0 is ready"
);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(2)),
false,
"participant 1 is not ready"
);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(3)),
true,
"participant 2 is ready"
);

impactEvaluator.setMinBalanceForTransfer(1 ether);
impactEvaluator.adminAdvanceRound();

address payable[] memory addresses2 = new address payable[](4);
addresses2[0] = payable(vm.addr(1));
addresses2[1] = payable(vm.addr(2));
addresses2[2] = payable(vm.addr(3));
addresses2[3] = payable(0x000000000000000000000000000000000000dEaD);
uint[] memory scores2 = new uint[](4);
scores2[0] = 0;
scores2[1] = 0;
scores2[2] = 0;
scores2[3] = impactEvaluator.MAX_SCORE();
impactEvaluator.setScores(2, addresses2, scores2);

assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(1)),
true,
"participant 0 is still ready, although now below the threshold"
);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(2)),
false,
"participant 1 is still not ready"
);
assertEq(
impactEvaluator.participantIsReadyForTransfer(vm.addr(3)),
true,
"participant 2 is still ready"
);

assertEq(impactEvaluator.readyForTransfer(0), vm.addr(1));
assertEq(impactEvaluator.readyForTransfer(1), vm.addr(3));
vm.expectRevert();
impactEvaluator.readyForTransfer(2);
}

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

Expand Down Expand Up @@ -472,18 +615,12 @@ contract ImpactEvaluatorTest is Test {
scores[0] = impactEvaluator.MAX_SCORE() / 2;
scores[1] = impactEvaluator.MAX_SCORE() / 2;
impactEvaluator.setScores(1, addresses, scores);
assertEq(
impactEvaluator.balanceHeld(),
100 ether
);
assertEq(impactEvaluator.balanceHeld(), 100 ether);

impactEvaluator.adminAdvanceRound();
addresses[0] = payable(0x000000000000000000000000000000000000dEaD);
impactEvaluator.setScores(2, addresses, scores);
assertEq(
impactEvaluator.balanceHeld(),
100 ether
);
assertEq(impactEvaluator.balanceHeld(), 100 ether);

impactEvaluator.releaseRewards();
assertEq(impactEvaluator.balanceHeld(), 100 ether);
Expand All @@ -505,18 +642,12 @@ contract ImpactEvaluatorTest is Test {
uint[] memory scores = new uint[](1);
scores[0] = impactEvaluator.MAX_SCORE();
impactEvaluator.setScores(1, addresses, scores);
assertEq(
impactEvaluator.availableBalance(),
0 ether
);
assertEq(impactEvaluator.availableBalance(), 0 ether);

impactEvaluator.adminAdvanceRound();
addresses[0] = payable(0x000000000000000000000000000000000000dEaD);
impactEvaluator.setScores(2, addresses, scores);
assertEq(
impactEvaluator.availableBalance(),
0 ether
);
assertEq(impactEvaluator.availableBalance(), 0 ether);

impactEvaluator.releaseRewards();
assertEq(impactEvaluator.availableBalance(), 0);
Expand Down Expand Up @@ -633,9 +764,9 @@ contract ImpactEvaluatorTest is Test {
balances[1] = 50 ether;

vm.expectRevert("Sum of balances must match msg.value");
impactEvaluator.addBalances{ value: 0 }(addresses, balances);
impactEvaluator.addBalances{value: 0}(addresses, balances);

impactEvaluator.addBalances{ value: 100 ether }(addresses, balances);
impactEvaluator.addBalances{value: 100 ether}(addresses, balances);
assertEq(
impactEvaluator.rewardsScheduledFor(addresses[0]),
50 ether,
Expand Down Expand Up @@ -664,7 +795,7 @@ contract ImpactEvaluatorTest is Test {
addresses[0] = payable(vm.addr(1));
uint[] memory balances = new uint[](2);
balances[0] = 100 ether;
impactEvaluator.addBalances{ value: 100 ether }(addresses, balances);
impactEvaluator.addBalances{value: 100 ether}(addresses, balances);

impactEvaluator.withdraw(payable(vm.addr(1)));

Expand All @@ -691,4 +822,21 @@ 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] = impactEvaluator.minBalanceForTransfer() / 2;
impactEvaluator.addBalances{value: balances[0] + balances[1]}(addresses, balances);

assert(impactEvaluator.participantIsReadyForTransfer(vm.addr(1)));
assert(!impactEvaluator.participantIsReadyForTransfer(vm.addr(2)));
assert(
!impactEvaluator.participantIsReadyForTransfer(payable(vm.addr(3)))
);
}
}
Loading