-
-
Notifications
You must be signed in to change notification settings - Fork 16
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 any combination of input format and output format. #69
base: master
Are you sure you want to change the base?
Conversation
There's no technical reason why this shouldn't be allowed. Closes BlockchainCommons#63
It's not needed anymore, since seedtool-cli now allows any combination of input and output format.
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.
You need to add many more tests to prove that any input format to any output format also work.
That's a bit of a high standard, considering that there were no tests before ;-) Anyway, I've added a testcase that tests all meaningful combinations of input/output formats. Let me know what you think. The test might actually have discovered a bug; shouldn't the output of these two commands be the same? Note that the checksum words are different. $ src/seedtool --deterministic TEST | src/seedtool --deterministic TEST --out sskr
tuna acid epic gyro many meow able able able next edge lamb liar city girl draw visa roof logo jolt city waxy jury trip dark safe arch flew what
$ src/seedtool --deterministic TEST | src/seedtool --deterministic TEST --in hex --out sskr
tuna acid epic gyro edge next able able able next edge lamb liar city girl draw visa roof logo jolt city waxy jury trip dark loud song main atom |
I believe all the tests are here: https://github.com/BlockchainCommons/seedtool-cli/blob/master/src/test.sh |
It was missing a `--in hex`, causing it to use random input for SSKR. This happened to *almost* work because the deterministic seed meant "random" produced the same result as reading from stdin.
I have fixed the test now. @wolfmcnally what do you think? Would this be good enough or do you need more tests than that? |
There's no technical reason why this shouldn't be allowed.
Closes #63