✅ Comprehensive settlement tests

[JuaniRios]

What?

• Test more logic branches of the code

How?

• Check https://linear.app/polimec/issue/PLMC-544/settlement

[JuaniRios]

• ✅ Test freeze restrictions on contributions #308 : 3 dependent PRs (#310 , #311 , #312 )
• ✅ Test freeze restrictions on bids #307
• ✅ Test freeze restrictions on evaluations #306
• ✅ Comprehensive settlement tests #304 👈
• ✅ Comprehensive funding end tests #303 : 1 other dependent PR (#315 )
• 🕵🏻 Double-check evaluation reward benchmarks #302
• 💀 New Benchmarks #301
• ✨ Optimize wap calculation #299 : 1 other dependent PR (#300 )
• 🎨 Restructure funding functions file #296
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @JuaniRios and the rest of your teammates on Graphite

[vstam1]

Think its a good initial testing setup. I have left a few comments and do have some comments in general:

1. We have quite a lot of code duplication (settlement for successful + failed project for each type 95% of that code is the same). I know that you are more of the clear distinction between tests and don't mind code duplication, but I think it would be great if we could somehow dedup the tests at least a bit.
2. I think in certain situations we could get away with less code for the "setup" part of the tests. So instead of going step for step we could create a finished project with as input our specific bids, contributions or evaluations, correct? I think this would make the tests also clearer where there is a clear "setup" part and a clear "this is what we are testing" part.
3. I think we are over asserting a bit in certain situations. For example in the settle_failed_bids part, we assert for the two bids that the ct amount is 0 before and after settlement. I think we can already skip the one before in this situation. We also assert the issuer balance == 0 at each step before and after, but I think a single assert for issuer balance == 0 at the end is sufficient. This makes the code easier to read and the assertions more clear.

[JuaniRios]

Think its a good initial testing setup. I have left a few comments and do have some comments in general:

1. We have quite a lot of code duplication (settlement for successful + failed project for each type 95% of that code is the same). I know that you are more of the clear distinction between tests and don't mind code duplication, but I think it would be great if we could somehow dedup the tests at least a bit.
2. I think in certain situations we could get away with less code for the "setup" part of the tests. So instead of going step for step we could create a finished project with as input our specific bids, contributions or evaluations, correct? I think this would make the tests also clearer where there is a clear "setup" part and a clear "this is what we are testing" part.
3. I think we are over asserting a bit in certain situations. For example in the settle_failed_bids part, we assert for the two bids that the ct amount is 0 before and after settlement. I think we can already skip the one before in this situation. We also assert the issuer balance == 0 at each step before and after, but I think a single assert for issuer balance == 0 at the end is sufficient. This makes the code easier to read and the assertions more clear.

1. I addressed your specific comments, but I guess you mean that the same refactor can be applied in multiple places right?
2. Where possible I can fix that, but in most places we cannot because the instantiator only works when specific conditions apply, like for example all bids are accepted. We can in the future extend the instantiator to accept more cases and reduce the size of these functions.
3. I prefer over-asserting than under-asserting. For the case of asserting the same thing over several logic steps, I think it's preferable since we are making sure only specific changes are done, and not anything else by accident. But I'm happy to look at any specific cases you have in mind, like the one for removing the accountData, which I thought was ok to remove

[JuaniRios]

@vstam1 A lot of repetition comes from the fact that 2 participations have different multipliers, and so have a different checking logic. And I think introducing a loop doesn't make sense there. I commented this in one of the inner comments you left

[JuaniRios]

Merge activity

• Jun 11, 10:37 AM EDT: @JuaniRios started a stack merge that includes this pull request via Graphite.
• Jun 11, 10:37 AM EDT: @JuaniRios merged this pull request with Graphite.