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 3 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.
To what extent does the issue with chain ids above break our ability to interact with our internal testnet, out of curiosity?
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.
Just swapping out the current subproviders with ones provided by 0x - they provided more consistent exceptions, that I wasn't hellbent on reimplementing.
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.
They may not support the larger chainID. We'll see what we can do.
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.
An explanation of what this promise represents would be ideal 😬
Also I tend to use
@return
rather than@returns
.@return
is more widely and historically supported both in JS and across languages, so it's just a bit easier of a habit 🤷♂️