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

FromMnemonic better error & fix #197

Merged
merged 3 commits into from
Apr 30, 2019
Merged

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Apr 30, 2019

Issue Number

#192

Overview

Comments

@KtorZ KtorZ requested a review from piotr-iohk April 30, 2019 07:41
@KtorZ KtorZ self-assigned this Apr 30, 2019
\your mnemonic sentence."
ErrEntropy ErrInvalidEntropyLength{} ->
"Something went wrong when trying to generate the entropy from \
\the given mnemonic. As a user, there's nothing you can do."
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I am almost tempted to shove in a error "OMG SOMETHING WENT WRONG" here because really, we can never reach this branch. It'd mean we'd bypassed our Mnemonic type, which, as far as I am concerned, isn't possible currently.

@KtorZ KtorZ force-pushed the KtorZ/192/from-mnemonic-errors branch from 6298abb to c0bbe07 Compare April 30, 2019 07:49
ErrMnemonicWords ErrWrongNumberOfWords{} ->
"Invalid number of words: at least "
<> show (natVal (Proxy :: Proxy mw))
<> " words are expected."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it always say at least 15 for mnemonic_sentence and at least 9 for mnemonic_second_factor ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for mnemonic_second_factor (where we allow 9 or 12) it will show somewhat faulty message for, say, 15 words I suppose...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could be a bit more comprehensive, like:
Wrong number of words - N. Supported number of words are: mnemonic_sentence [15,18,21,24]; mnemonic_second_factor [9,12]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. No :)
The mnemonic length is actually encoded at the type-level here (that's what mw symbolizes) and, the error message is constructed based on this type. So, for any representation [9,12] or [15,18,21,24] or [12,18] and so forth, the error will always report on the first (smallest) length.

Copy link
Contributor

@piotr-iohk piotr-iohk Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for mnemonic_second_factor (where we allow 9 or 12) it will show somewhat faulty message for, say, 15 words I suppose...

I mean that when user provides 15 words for mnemonic_second_factor he will get Invalid number of words: at least 9 words are expected... well 15 is more than 9 so that's somewhat misleading

That's why I suggested:
Wrong number of words - N. Supported number of words are: mnemonic_sentence [15,18,21,24]; mnemonic_second_factor [9,12]

That is if you can throw in N (if not, we can drop it)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got you! That requires slightly more work than it seems actually. Because of the way operations at the type-level work on list (at least, here), we do define instance in an inductive / recursive manner and, we "loose" (we actually consume them) some type-level information as we do it. I can try something for a few minutes, but let's scope it to 15 minutes ^^"

@KtorZ
Copy link
Member Author

KtorZ commented Apr 30, 2019

Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 🥇 :)

@KtorZ KtorZ merged commit cb14335 into master Apr 30, 2019
@KtorZ KtorZ deleted the KtorZ/192/from-mnemonic-errors branch April 30, 2019 09:42
piotr-iohk pushed a commit that referenced this pull request Apr 30, 2019
KtorZ pushed a commit that referenced this pull request Apr 30, 2019
piotr-iohk pushed a commit that referenced this pull request Apr 30, 2019
KtorZ pushed a commit that referenced this pull request May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants