-
Notifications
You must be signed in to change notification settings - Fork 50
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
Allow emitting & parsing of bytes per dagjson codec spec #166
Conversation
Do we have test vectors for this yet? I'd be especially interested in what happens for pernicious data like: |
You can see the round-trip test I put in the PR. that's the test vector i included. |
here was my start at that: ipld/js-dag-json@ef139b7 - branch is |
assert.throws(() => decode(encode({ '/': link.toString(), x: 'bip' })))
assert.throws(() => decode(encode({ '/': { bytes: 'mS7ldeA', x: 'bip' } })))
assert.throws(() => decode(encode({ '/': { bytes: 'mS7ldeA' }, x: 'bip' })))
assert.throws(() => decode(encode({ '/': { bytes: 'mS7ldeA', x: 'bip' }, bop: 'bip' }))) there's your combo-breaker |
this is more strict than the current implementation. I would argue that adding that strictness is a separate PR from this one. I signed up to be able to interpret bytes through a round trip of IPFS / ipld-prime, not fully-fix dag-json |
(d'oh, I was blind on my first review and didn't see some of the tests, sorry for that.) So the one case I proposed as being specifically interested in is trying to hammer: encountering the maximally confusing things several times in a row, and only getting to the combo breaker at the very end. The reason I want to poke that case in particular is to make sure we don't have exceptionally weird bugs that emerge from the limited look-behind window. Given the state of the specs otherwise, that's the only thing I really feel compelled to push on. |
To confirm: in the case you propose, it will be parsed simply as a map, and will not gain bytes because it does not match our strict-match criteria. In the same way it would not be matched or rejected by the link detection, it will likewise not be matched or rejected by the new byte detection. |
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 for the additional tests. I think I'm not scared of this now! 👍
fix #165
The 'allowLinks' flag to
marshal
/unmarshal
seems at this point to have extended past it's original name. Would happily take suggestions for a better a name.