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

Help detect typos in device add phrase #1529

Merged
merged 2 commits into from
Dec 14, 2015
Merged

Conversation

patrickxb
Copy link
Contributor

Checks words entered are in list
Checks number of words
Short timeout on Hello, custom error message

Fixes CORE-2232

r? @maxtaco

@@ -149,6 +154,9 @@ func (p *provisioner) runProtocolWithCancel() (err error) {
p.canceled = true
return ErrCanceled
case err = <-ch:
if _, ok := err.(rpc.CanceledError); ok && !p.helloReceived {
return ErrHelloTimeout
Copy link
Contributor

Choose a reason for hiding this comment

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

does this error export properly over RPC? We'll likely need it for the desktop client, I'm guessing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not the error that goes over RPC. The engine checks for it and returns its own error. But the engine error isn't exportable, so I'll fix that.

@maxtaco
Copy link
Contributor

maxtaco commented Dec 14, 2015

Looks great. 👍

My only reservation is, did we pick the right timeout? Though I see arguments for and against.

@patrickxb
Copy link
Contributor Author

Not sure. We had the one bug report where the user had a really slow connection to keybase.io...so maybe 15s isn't enough.

Checks words entered are in list
Checks number of words
Short timeout on Hello, custom error message
@patrickxb patrickxb merged commit ce8c507 into master Dec 14, 2015
@patrickxb patrickxb deleted the patrickxb/CORE-2232 branch December 14, 2015 22:17
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.

2 participants