-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix TxMetaText
generators.
#2862
Conversation
These tests fail, as the generator for `TxMetaText` values occasionally produces values that when UTF-8 encoded are longer than 64 bytes, which is disallowed.
The length of a `TxMetaText`, when UTF-8 encoded, must not be greater than 64 bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
lib/core/src/Cardano/Wallet/Gen.hs
Outdated
(T.pack . take 32 . getPrintableString <$> arbitrary) `suchThat` guardText | ||
genTxMetaText :: Gen Text | ||
genTxMetaText = | ||
(T.pack . take 32 . getPrintableString <$> arbitrary) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make this variable-length, with a hard maximum of 64 unicode characters, which is then filtered by guardTxMetaText
.
(T.pack . take 32 . getPrintableString <$> arbitrary) | |
(T.pack . getUnicodeString <$> scale (min 64) arbitrary) |
Also I believe it's fine to use UnicodeString
instead of PrintableString
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 58cc2af.
lib/core/src/Cardano/Wallet/Gen.hs
Outdated
shrinkTxMetaText t | ||
| n <= 1 = [] | ||
| otherwise = filter guardText [ T.take (n `div` 2) t, T.drop (n `div` 2) t ] | ||
| otherwise = filter guardTxMetaTextPrefix | ||
[ T.take (n `div` 2) t, T.drop (n `div` 2) t ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure guardText
isn't needed anymore - a relic of a previous JSON encoding of TxMetadata. So we could just re-use the UnicodeString
shrinker.
shrinkTxMetaText t = [T.pack s | u <- shrink (UnicodeString $ T.unpack t), let s = getUnicodeString u, length s > 0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newtype Large a = Large { unLarge :: a } deriving (Eq, Show) | ||
|
||
instance Arbitrary (Small TxMetadata) where | ||
arbitrary = Small <$> genSmallTxMetadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the definitions of genSmallTxMetadata
and genTxMetadata
to try and see why they couldn't be replaced with Test.QuickCheck.scale
.
The distinction seems to be that "small" generates non-recursive metadata values from the 3 primitive metadata types, and non-small generates metadata values of any depth.
So perhaps we could fix the naming. So remove Large
and rename Small to something like Primitive
or Simple
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed (hopefully) in dc70900.
This is a relic of a previous JSON encoding of TxMetadata.
bors r+ |
Build succeeded: |
Issue Number
#2861
Comments
This PR:
TxMetadata
so that they only produceTxMetaText
values where the UTF-8-serialized length does not exceed 64 bytes.genSimpleTxMetadata
andgenNestedTxMetadata
to emphasise the difference in generated values (simple vs nested).UnicodeString
to generateTxMetaText
content.genSimpleTxMetadata
andgenNestedTxMetadata
.