AdChain / AdChainRegistry

adChain registry smart contracts
Apache License 2.0
30 stars 10 forks source link

OOP-lite: back from the dead #35

Closed skmgoldin closed 6 years ago

skmgoldin commented 6 years ago

As per previous conversations, I've removed the resolve function from the Challenge library in https://github.com/AdChain/AdChainRegistry/commit/a49bc46fabb6ca65b0749a8e766fd69514d4f12a:

The resolve member function of the Challenge struct made state changes
which would result in only the partial completion of the resolution
task. This was more dangerous than the code it de-duped, because it
made it seem that other *necessary* state changes that might be required
in the complete resolution of a challenge might be done by the resolve
method. They're not. Challenge resolutions are heterogenous, and short
of being able to pass function types into a resolve method, the resolver
itself needs to operate on the internal state of the Challenge struct.
More important to keep that totally explicit than it is to dedupe a
small number of lines. (Though if, in the future, we could solve this
more elegantly that would be highly desirable).

With that out of the way, I feel this PR is something of a half-measure but still an overall improvement to the codebase. Compare: https://github.com/AdChain/AdChainRegistry/compare/develop...a49bc46fabb6ca65b0749a8e766fd69514d4f12a

This does not actually reduce our testing burden, but it does allow us to dedupe some code and separate concerns. With the resolve method removed it doesn't confuse who gets to change internal state: the contract in which the Challenge resides must mutate its state to resolve a challenge, and we bundle that up in one private (implicitly dangerous) method.

The final commit in this branch makes a bunch of formatting improvements.

BREAKING CHANGES: determineReward -> challengeWinnerReward calculateVoterReward -> voterReward

Just name changes. Arguments and returns are the same.

skmgoldin commented 6 years ago

If a user calls claimReward directly, state will not be messed up, but an expected event in the wrapper contract might not fire, which could confuse some UIs.

kangarang commented 6 years ago

lgtm @skmgoldin !