Skip to content
This repository has been archived by the owner on Jun 27, 2022. It is now read-only.

add Transport#setExchangeTimeout and use it in U2F instead of open timeout #67

Merged
merged 2 commits into from
Feb 8, 2018
Merged

add Transport#setExchangeTimeout and use it in U2F instead of open timeout #67

merged 2 commits into from
Feb 8, 2018

Conversation

gre
Copy link
Contributor

@gre gre commented Feb 8, 2018

this was a mistake to use the open timeout because the exchange timeout is another concept.
moreover, this was a bug that the TransportU2F was expecting a timeout in seconds and not in milliseconds like all other parts. Fixes #65

Technically, this commit is a breaking change because if you were using the U2F timeout, you should now use the .setExchangeTimeout instead. otherwise you will have the default of 30s. We probably will unroll a major release to be safe.

NB: some transport does not implement openTimeout / exchangeTimeout at the moment. we will incrementally improve this.

this was a mistake to use the open timeout because the exchange timeout is another concept.
moreover, this was a bug that the TransportU2F was expecting a timeout in seconds and not in milliseconds like all other parts.
Fixes #65

Technically, this commit is a breaking change because if you were using the U2F timeout, you should now use the .setExchangeTimeout instead. otherwise you will have the default of 30s. We probably will unroll a major release to be safe.

NB: some transport does not implement openTimeout / exchangeTimeout at the moment. we will incrementally improve this.
@amougel amougel self-requested a review February 8, 2018 12:45
Copy link
Contributor

@amougel amougel left a comment

Choose a reason for hiding this comment

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

LGTM

@gre gre merged commit 115396b into LedgerHQ:master Feb 8, 2018
@gre gre deleted the u2f-timeout-in-millis branch February 8, 2018 14:09
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants