[PSM Checklists] General improvements after feedback

[amusingaxl]

• reorganize the checklist items
• fix bad precision value for mat
• fix coverage rules for LitePsmJob
• add missing checks for additional spell dependencies (main)

[SidestreamColdMelon]

Few other suggestions based on the previous review:

• Let's add a general section in the main checklist to check the dependencies folder (see ℹ️ Additional dependencies checks that I did in the review). Basically all files found inside dependencies have to be audited to become a "dependency" and therefore have to be diff-checked to match audited source. I believe this check is not LitePSM specific
• Let's edit all subchecks inside workable returns the correct value: and work has the desired effect: inside psm-checklists.md. Those values should be compared not with 0 (otherwise this job will be very expensive, requiring refill after every transaction), but with the specific threshold. Specific example: instead of IF rush() > 0, returns (true, litePsm.fill.selector) the check should say IF rush() >= rushThreshold, returns (true, litePsm.fill.selector). Or did you wanted to completely remove e2e tests requirement from the checklist?
• Let's fix Changes are tested via testNewOrUpdatedChainlogValues to say Changes are tested via testChainlogIntegrity and testChainlogValues

[amusingaxl]

[...] Or did you wanted to completely remove e2e tests requirement from the checklist?

I believe having that level of specificity in the checklists is a foot gun.
In this case, to synthetically create the desired state for the job to be activated was easy enough. For other modules, it might be too complicated.

We might have to analyze that case by case, which is not ideal.

Regarding the other points, I agree.

[SidestreamColdMelon]

I would recommend to only have checkboxes for points that can be checked / validated as "correct". In other words, if a reviewer doesn't need to go somewhere and actively ensure some rule, no checkbox should be present. Otherwise, the mental meaning of checkbox is mixed up and might lead to confusion during the review. In the worst case, a reviewer will tick some checkbox without even checking anything.

If you agree, please apply this rule beyond this line (above and below in the checklist). I can at least see the same issue on lines 37, 51, 54, 60, 79, 90, 95, 102, 108, 112, 124, 129, 130, 132, 139, 140, 147, 151

[amusingaxl]

For 102 and 108 it kind of makes sense to have the check because they are conditional branches, so it you mark it, it means you are following that branch.

Also for visual consistency, maybe we should not be that strict that items with 1 single sub-item should be inlined.

[SidestreamColdMelon]

it kind of makes sense to have the check because they are conditional branches, so it you mark it, it means you are following that branch.

But ticking the items found in the branch would already indicate which branch you followed. We also don't normally have checkboxes in all other branching IFs. I would recommend to remove them, but it's kind of hard to draw the line between e.g. IF on line 5 and IF on line 104, I wouldn't make it a blocker – sadly it's no a code for which we can have a linter

[SidestreamColdMelon]

• Let's fix Changes are tested via testNewOrUpdatedChainlogValues to say Changes are tested via testChainlogIntegrity and testChainlogValues

This was not yet addressed