Make Value's flat/cbor decoder consistent with unValueData#7669
Make Value's flat/cbor decoder consistent with unValueData#7669
Conversation
SeungheonOh
left a comment
SeungheonOh
left a comment
There was a problem hiding this comment.
Looks good. I noticed some change in behavior but looks like those are intended and I think new behaviors are better.
| innerLen <- CBOR.decodeMapLen | ||
| inner <- replicateM innerLen ((,) <$> CBOR.decode <*> CBOR.decode) | ||
| pure (currency, inner) | ||
| buildValueWith "Value CBOR decoder" pure pure outer |
There was a problem hiding this comment.
We have slight change in behavior here I think. Previously pack was used which would've normalized and allowed not-normal values to be parsed. However, buildValueWith will fail if non-normal value is given.
I think this is a better behavior. Just making a note.
There was a problem hiding this comment.
Okay. realized this is intended after reading the new test cases. Seems good.
| ( \(cData, tsData) -> | ||
| (,) | ||
| <$> unB cData | ||
| <*> case tsData of Map ts -> pure ts; _ -> fail "unValueData: non-Map constructor" |
There was a problem hiding this comment.
nitpick : we can probably have unMap here instead of ugly single line case
| | i == 0 || i < unQuantity minBound || i > unQuantity maxBound -> | ||
| fail "unValueData: invalid quantity" | ||
| | otherwise -> pure (UnsafeQuantity i) | ||
| | Just q <- quantity i -> pure q |
There was a problem hiding this comment.
quantity won't ensure i /= 0 which is invalid quantity here(unless we want to change behavior of unValueData)
buildValueWith handles this correctly.
| <> show (unQuantity x) | ||
| Just q -> pure q | ||
|
|
||
| buildValueWith |
There was a problem hiding this comment.
documentation about what this function actually enforces/when it fails would be nice.
2 participants
Factored out helper function
buildValueWith, which is used by all three. Fixes #7602