Abstract threshold calculation

[lehins]

Description

We need to have ability to answer if voting is allowed for a particular entity by reusing the current threshold calculation logic, thus reducing duplication. This is needed for #3685

Checklist

• Commit sequence broadly makes sense and commits have useful messages
• New tests are added if needed and existing tests are updated
• When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
  versioning process.
• The version bounds in .cabal files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
• All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages. New section is never added with the code changes. (See RELEASING.md)
• Code is formatted with fourmolu (use scripts/fourmolize.sh)
• Cabal files are formatted (use scripts/cabal-format.sh)
• hie.yaml has been updated (use scripts/gen-hie.sh)
• Self-reviewed the diff

[aniketd]

LGTM 👍 Thank you!

Just an idea: may be we could make PParams have VotingThreshold too.

[teodanciu]

There is something about this pattern that feels a little wrong.
I think I understand where it's coming from: for these is.*Allowed functions, we don't actually need to know anything other than whether voting.*ThresholdInternal returns NoVotingAllowed, while returning NoVotingAllowed in voting.*ThresholdInternal functions does not depend on the value of other parameters like pparam, isElectedCommittee or committee.
So in theory,
for isStakePoolVotingAllowed we could return True,
for isCommitteeVotingAllowed we could return True if the action is NoConfidence or NewCommittee
and
for isDRepVotingAllowed we could return True

,if I understand correctly the code.
But then this would mean we duplicate the logic between determining whether the voting is allowed and computing the actual threshold.

Semantically these parameters are optional (pparam, isElectedCommittee or committee), but I can see how making them optional is not gonna help.
On the other hand, passing emptyPparams makes it seem like we might depend on the value of the default value of the parameters.

Could we somehow extract this information (i.e.: what makes different actions return NoVotingAllowed) and then use it from both places?
I don't have a concrete idea for refactoring, just sharing my thoughts on this. Maybe we can think of a way to refactor it later if it's not the time for it now.

[lehins]

There is something about this pattern that feels a little wrong.

@teodanciu Good thing that coding is about facts, not feelings 😜. I think you getting the same feeling some people get about the Default class. Cause that is what that whole type class is about: providing some value, when the value is not particularly relevant.

But then this would mean we duplicate the logic between determining whether the voting is allowed and computing the actual threshold.

I don't understand this comment. The whole point of this PR is to prevent duplication of logic in two places: figuring the thresholds and figuring out whether voting is allowed or not. Which, I believe, implementation in this PR successfully avoids duplication.

Semantically these parameters are optional (pparam, isElectedCommittee or committee), but I can see how making them optional is not gonna help.

They are optional for figuring out whether voting is allowed or not, but they are required for figuring out the thresholds. In fact I could use def for those arguments in the former case, when they are optional.

On the other hand, passing emptyPparams makes it seem like we might depend on the value of the default value of the parameters.

We could pass undefined, but that would be a lot worse in my opinion. In any case this is an internal function with a comment describing why this is argument is optional.

Could we somehow extract this information (i.e.: what makes different actions return NoVotingAllowed) and then use it from both places?

Previous implementation did not allow us to reuse this logic at all for the two different use cases. This was my attempt at making it possible. I know of a way to make it possible to completely avoid using default values for arguments, namely use types that return functions that can be later either applied to arguments. This is an approach one would do in a strict language. However that would result in a more complex solution and more boiler plate, since it would require more types. Luckily for us Haskell is a lazy language, so we don't need to worry about creating more elaborate solutions in order to avoid redundant computation.

Maybe we can think of a way to refactor it later if it's not the time for it now.

We can always try to improve the code later. Most importantly this whole logic is localized to one module, so we don't need to worry about affecting any rules in a breaking way.

[teodanciu]

Looks good to me