This repository has been archived by the owner on Mar 28, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hardware wallets #184
Hardware wallets #184
Changes from all commits
18d65bd
0c615d5
afaec4e
0b9cdac
e2302ae
739304a
d26756e
50d39d7
ce2e747
621773c
6fd969f
c73798a
e25fc67
847449a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Our chainId is 1101 though, no?
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.
Yah - we may have changed the default for Geth? In either case, they're both >8 bits.
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.
Or geth changed theirs. Can't remember for sure.
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.
This workaround actually seems to do the opposite, masking any >8-bit chain id to only keep the lowest 8 bits. Guessing it's designed not to screw up when communicating with the ledger, rather than being designed to support larger chain ids?
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.
Yup. Poor wording on my end, deriving from a poor understanding. Basically, Ledger has implemented 32 bit chainID support in their device firmware, but the Ledger Ethereum app still transports the truncated 8 bit ID, I'm guessing. eg. one chain, Piri, has its 32 bit chainID working.
That and my assumptions were wrong - the LedgerSubprovider from 0x uses an older version of @ledgerhq/hw-app-eth, without this workaround. Soooo, going to address that in a bit.
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.
This may be a good opportunity to use an object to map wallet name -> connector.
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.
Let's at least log this error… We had this issue in the token dApp also where we were displaying a super-generic error and there was no easy way to get to the underlying one.
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.
Yep, got that covered. The error is set in
chooseWallet
, where it is thrown immediately after -https://github.com/keep-network/tbtc-dapp/blob/master/src/components/lib/ConnectWalletDialog.js#L52
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.
Do we need an extra
div
here?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.
As above re: the div.
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.
F' three.
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.
F' fo'.
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.
I would rather prefer a boutonnière, my dear sir.
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.
I'll chuck that in another PR - we've still got the lingering global styles on
<button>
.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.
Uh-oh, looks like some TypeScript annotations sneaked through on this JS file.
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.
Weirdly enough, they didn't break my build? create-react-app / babel must have some support, somewhere in those depths of hell.
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.
I guarantee it, yes. This is why babel makes me squeamish… I like to know what's actually going on <_<
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.
Could be a
const
yah?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.
Fixes it in what sense? The error message is better?
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.
Oops.
exchangeTimeout
is set unrealistically low by default (5s or so).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.
What a property name.
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.
I mean it's no AbstractSingletonProxyFactoryBean... 😄