Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Sjlver
Copy link

@Sjlver Sjlver commented May 1, 2022

There's no technical reason why this shouldn't be allowed.

Closes #63

Sjlver added 2 commits May 1, 2022 15:40
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.
Copy link
Collaborator

@wolfmcnally wolfmcnally left a 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.

@Sjlver
Copy link
Author

Sjlver commented May 2, 2022

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

@ChristopherA
Copy link
Contributor

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.

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.
@Sjlver
Copy link
Author

Sjlver commented May 15, 2022

I have fixed the test now.
What I had thought to be a bug was actually not a bug, just an artifact of using the same deterministic seed in two invocations of seedtool-cli.

@wolfmcnally what do you think? Would this be good enough or do you need more tests than that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 2023 Q4 Priority
Development

Successfully merging this pull request may close these issues.

BIP39 to SSKR and reverse
3 participants