Enforce a certain number of tokens on redemption #102
Conversation
Also reduce duplication in the test implementation a bit And add some more error case tests And remove the old, no-longer used redemption parameter globals Some Integers becomes Ints for intra-module consistency
exarkun
requested review from
hacklschorsch and
a team
and removed request for
hacklschorsch
There was a problem hiding this comment.
Thanks for asking for a Review @exarkun. All I can say so far is I [✓] Viewed all files, and haven't found any errors obvious to me. That doesn't mean a whole lot currently I am afraid though, so beware :)
Thanks @hacklschorsch ! |
| | otherwise = Just $ groupSize + groupSizeAdjustment | ||
| where | ||
| (groupSize, remainder) = totalTokens `divMod` numGroups | ||
| groupSizeAdjustment = if groupNumber < remainder then 1 else 0 |
There was a problem hiding this comment.
This could probably use a comment, it took me a little time to understand what was going on. Maybe something like: "if the number of groups doesn't evenly divide the total tokens, the extra tokens are allocated evenly to the lower numbered groups"?
On the other hand, we could consider requiring that they divide exactly, and so have tokenCountForGroup part of RedemptionConfig
There was a problem hiding this comment.
(For the second point, it looks like the current configuration does meet this requirement)
| throwError $ jsonErr err500 $ OtherFailure "invalid redemption counter" | ||
| Just allowedTokenCount -> | ||
| if allowedTokenCount /= length tokens | ||
| then throwError $ jsonErr err400 $ OtherFailure "wrong number of tokens" |
There was a problem hiding this comment.
Should this have a structured error message, with the expected number of tokens?
There was a problem hiding this comment.
Maybe. I guess that would be easier to answer if we had a UX for handling any of these failure conditions. Right now what happens is ZKAPAuthorizer notes the error and usually tries exactly the same thing again a little later and no software exists to surface these error conditions to the user.
| = Unpaid -- ^ A voucher has not been paid for. | ||
| | DoubleSpend -- ^ A voucher has already been redeemed. | ||
| | OtherFailure Text -- ^ Some other unrecognized failure mode. | ||
| -- | Given counter was not in the expected range | ||
| | CounterOutOfBounds Integer Integer Integer | ||
| | CounterOutOfBounds Int Int Int |
There was a problem hiding this comment.
Not really related to this PR, but I noticed it while trying to understand it:
I don't think any of these values need to be a part of Result, as we never return them as values of type Result (but rather encode them as part of ServerError). I'm not sure how it should be structured, though this has some discussion of various possibilities, including some packages that help with it.
Fixes #99