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

support 32bits chainId with the latest Ledger app #202

Merged

Conversation

hackmod
Copy link

@hackmod hackmod commented Jul 29, 2018

Please see LedgerHQ/app-ethereum@8260268
for some larger chainId cases, returned signature v should be recomputed at the client side.

Please see LedgerHQ/app-ethereum@8260268
for some larger chainId cases, returned signature v should be recomputed at the client side.
@mkrufky
Copy link
Member

mkrufky commented Jul 29, 2018

Thank you for this, @hackmod . This patch is needed for Pirl support. I'll push up that PR now.

@yograterol
Copy link

@j-chimienti can you check this PR?

@hackmod
Copy link
Author

hackmod commented Jul 30, 2018

the latest Ledger app support 32bits chainId, but Ledger returns truncated v value (1-byte).
anyway, it contains signature v bit. so we can recalc v value by chainId * 2 + 35 + signature v bit (1 or 0)

Copy link

@j-chimienti j-chimienti left a comment

Choose a reason for hiding this comment

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

signTxLedger looks good.

Required

  1. fix line 245 (keep CLO EIP155Support)
  • We won't need to explicity remove EIP155Supported from CLO anymore \m/

             } else if (rawTx.chainId === 820) {
 	 
                 EIP155Supported = false;	                
 	 
       }

I've tested w/ CLO tx (w/ EIP155Supported) https://cloexplorer.org/tx/0x406f5e595f51492fbe5498a5508f03b6a3647f014dfaa568d217311f5ec2b203 and https://cloexplorer.org/tx/0xdfc367f5c84901414db69caf61da6d439044dd21718e7442d1745b952693ffd0

@mkrufky
Copy link
Member

mkrufky commented Jul 31, 2018

@j-chimienti This pull request is useful for other coins with large chain IDs, such as Pirl, Akroma and Musicoin (I have built apps for all three, and all three are on now Ledger's official roadmap)

I have addressed the CLO specifics in a separate PR #205

@j-chimienti
Copy link

@mkrufky right on! Splitting the PRs makes sense

@mkrufky
Copy link
Member

mkrufky commented Jul 31, 2018

Thank you @j-chimienti ... As I mentioned in #205 , we should wait for the next firmware to be released before we merge that one, but this pr #202 can be merged now, for the benefit of Pirl, Akroma, Musicoin and ESN(Ethersocial Network)

Pirl full 32 bit chain ID Ledger support: LedgerHQ/app-ethereum#18 (merged)
Pirl on Ledger's official roadmap: https://trello.com/c/wKrSk4ps/112-pirl-support

Akroma full 32 bit chain ID Ledger support: LedgerHQ/app-ethereum#19 (merged)
Akroma on Ledger's official roadmap: https://trello.com/c/TbLAr4Ik/113-akroma-support

Musicoin full 32 bit chain ID Ledger support: LedgerHQ/app-ethereum#20 (merged)
Musicoin on Ledger's official roadmap: https://trello.com/c/a3YFOxx1/114-musicoin-support

ESN(Ethersocial Network) full 32 bit chain ID Ledger support: LedgerHQ/app-ethereum#21

I built a Callisto app that also fully supports EIP-155 but I haven't (really) announced that one publicly yet. EDIT: I have now :-D

Callisto (CLO) full 32 bit chain ID Ledger support: LedgerHQ/app-ethereum#22 (merged)
Callisto on Ledger's official roadmap: https://trello.com/c/AawiAoSM/116-callisto-support

@j-chimienti
Copy link

Merging as will will fix this pr #202 can be merged now, for the benefit of Pirl, Akroma, Musicoin and ESN(Ethersocial Network)

But chainId will still be removed from CLO. #205 addresses issue, but will introduce breaking changes w/ trezor

Suggestion

Either we wait till Trezor to merge both, or add in previously requested changes

@hackmod
Copy link
Author

hackmod commented Aug 1, 2018

anyway, this PR has almost no effect for current Ledger/Trezor situation :)

@mkrufky
Copy link
Member

mkrufky commented Aug 1, 2018

@j-chimienti I just pushed up #206 containing the change that you asked for in a separate pull request. I will rebase #205 after #202 and #206 are merged

@mkrufky
Copy link
Member

mkrufky commented Aug 1, 2018

I would suggest to merge in the following order:

  1. enforce EIP-155 replay attack protection for Callisto (CLO) with Ledger hardware wallets #206 enforce EIP-155 replay attack protection for Callisto (CLO) with Ledger hardware wallets

  2. support 32bits chainId with the latest Ledger app #202 , this PR, actually enabling the feature

@hackmod
Copy link
Author

hackmod commented Aug 1, 2018

MyEtherWallet#1979 merged

@mkrufky
Copy link
Member

mkrufky commented Aug 1, 2018

Identical PR to this one just got merged upstream MyEtherWallet#1979

@Dexaran Dexaran merged commit ff61b67 into EthereumCommonwealth:mercury Aug 4, 2018
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.

5 participants