search

Polimec / polimec-node

Decentralized community-driven funding protocol for Web3
https://www.polimec.org/
GNU General Public License v3.0
14 stars 6 forks source link

✅ Comprehensive settlement tests #304

Closed JuaniRios closed 5 months ago

JuaniRios commented 5 months ago

What?

How?

JuaniRios commented 5 months ago

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

Join @JuaniRios and the rest of your teammates on Graphite Graphite

JuaniRios commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

Merge activity