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

WCIP-2: New exchangeKey flow #118

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@pedrouid
Copy link
Member

commented May 6, 2019

Related to WCIP-2 (WalletConnect/WCIPs#2)

PS - Requires #121

@pedrouid pedrouid changed the title New exchangeKey flow WCIP-2: New exchangeKey flow May 6, 2019

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Still work in progress, but please take a look. cc @ligi

pedrouid added some commits May 6, 2019

@pubkey

This comment has been minimized.

Copy link

commented May 9, 2019

I recommend to not import the complete eth-crypto module.

Instead of

import EthCrypto from 'eth-crypto';
const identity = await EthCrypto.createIdentity();

do

import { createIdentity } from 'eth-crypto';
const identity = await createIdentity();

This will reduce your bundle-size.

pedrouid added some commits May 9, 2019

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

I recommend to not import the complete eth-crypto module.

Thanks! I will do that!

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

BTW @ligi, I just tested the new exchangeKey flow with Example Dapp and Test Wallet and it worked flawlessly. Fixed a bunch of encoding and async issues but you can review the PR for more detail!

I didn't find any performance issues or hiccups with both the signing challeng and key update.
Let me know your feedback

@ligi

This comment has been minimized.

Copy link
Member

commented May 9, 2019

with which devices did you test?

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

with which devices did you test?

With my personal devices which are not on the low-end side, but I will create deploy previews for you to test it.

@ligi

This comment has been minimized.

Copy link
Member

commented May 9, 2019

yea - I do not think it is noticeable with high-end devices in fast networks - but could be noticed with low-end devices and/or bad networks - and the protocol should cater for this IMHO.
Also thinking over it more - it just feels wrong from the protocol perspective - starting with a symetric key and then trying to make things secure afterwards. It feels like an ugly patch.
I am getting more and more convinced that the process must start with an asymetric key - means a public key for a keypair is in the URL and not a symmetric key.

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

yea - I do not think it is noticeable with high-end devices in fast networks - but could be noticed with low-end devices and/or bad networks - and the protocol should cater for this IMHO.
Also thinking over it more - it just feels wrong from the protocol perspective - starting with a symetric key and then trying to make things secure afterwards. It feels like an ugly patch.
I am getting more and more convinced that the process must start with an asymetric key - means a public key for a keypair is in the URL and not a symmetric key.

This would however be a major breaking change and I'm not sure if it's not slower as well because of how many times you are actually encrypting and decrypting using assmytric keys.

EXAMPLE:

Peer A displays QR Code with PubKeyA
Peer B scans QR Code
Peer B sends PubKeyB encrypted with PubKeyA
Peer A decrypts PubKeyB and sends SessionRequest
Peer B decrypts SessionRequest and displays to user
Peer B sends Session approval or rejection

If approved
Peer A will send a SymKey for exchangeKey
Peer B exchanges key with SymKey and responds with success
Peer A receives exchangeKey success
---- session established ----

I think your proposal is a more elegant solution but it's both a breaking change and a more expensive proccess than the current WCIP-2 proposal

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

PS - also you would need both parties to generate assymetric keys while the WCIP only requires one peer to generate keys

@ligi

This comment has been minimized.

Copy link
Member

commented May 9, 2019

no - this is how I see it:

Peer A displays QR Code with PubKeyA
Peer B scans QR Code
Peer B sends symkey encrypted for PubKeyA
..

@ligi

This comment has been minimized.

Copy link
Member

commented May 9, 2019

PS - also you would need both parties to generate assymetric keys while the WCIP only requires one peer to generate keys

no - only one side needs to do this - and the side that is not a phone in most cases

pedrouid added some commits May 10, 2019

@rmeissner

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

it's both a breaking change

I think we should not bother if it is breaking or not, since it is a beta breaking chances are (annoying, but) fine. :)

@ligi

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@rmeissner fully agree
we should move the discussion here: WalletConnect/WCIPs#2 and I signal to close this PR (unmerged)

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

I have to disagree, I understand your POV in this discussion but I have spent almost a year poking every wallet and dapp to adopt WalletConnect. Breaking changes are not something we can afford right now.

The legacy version (version < 0.7.x) was a poor architecture to fulfill the WalletConnect mission and after we were hit with enormous friction to get it adopted, I decided to completely re-design to meet the requirements of multiple wallets and dapps at the same time.

The current version (v1.0.0-beta.x) is faster, more private and compatible with current projects in this ecosystem. Yet since its release in January 2019, we are still yet to see 100% migration from the legacy version. This is a huge problem for a project that aims to bring a common interface between all Dapps and Wallets.

The v1.0.0-beta hasn't had any breaking changes in its protocol since beta.1 release in January and yet we are still seeing some friction as we continue to developer better developer tools to ease adoption.

WalletConnect has reached significant momentum in the last month with the adoption of projects like Binance DEX, Trust Wallet, Uniswap, InstaDapp, Rainbow, MakerDAO, Gnosis Safe, etc. Hence I repeat that we cannot afford breaking changes right now.

Technology is nothing if it's not used, we will continue to improve WalletConnect incrementally and eventually we will publish an even better version with the v2.0 upgrade. Regarding the Semver spec, we would have to bump the protocol to version 2 if we were to follow @ligi proposal.

I whiteboarded with @jinchung and debated the pros and cons of the alternative proposal for WCIP-2 and we concluded that it was indeed a better solution to refactor the exchangeKey flow. However it would completely change the way WalletConnect works completely right now.

This is why I'm happy to add the alternative proposal for exchangeKey to the WalletConnect v2.0 Protocol. It will be far easier to separate the current architecture with a major version change without breaking support for everyone right now. Since we premeditatedly included the version number on the WalletConnect URI scheme as part of the EIP-1328, it will be a smooth integration for Wallets to parse it and handle both versions correctly.

I've tested this current proposal (WCIP-2) with existing Dapps and Wallets and it worked flawlessly as the exchangeKey in this design aims to be added security instead of a requirement, making it backwards compatible and extremely easy to be adopted by current projects that support WalletConnect.

I would like to ask you to postpone the alternative proposal to the v2.0 spec so that we can let the v1.0 adoption continue without killing the current momentum. I will be in Berlin this weekend to attend DTN where there will be talks about libp2p, scuttlebut and other p2p protocols that we could use for the v2.0. Let's start working out the details of the v2.0 asap instead of changing the v1.0.

@ligi

This comment has been minimized.

Copy link
Member

commented May 13, 2019

hm - but both proposals are breaking changes ..
Still strongly signal to not adopt WCIP-2

@rmeissner

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

Ok I understand your motivation, but in this case I would propose to drop the -betaX from the version name ;), since for me as a developer that would be a sign that there will not be breaking changes anymore and it would motivate me to migrate from 0.7 to 1.0.0

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

hm - but both proposals are breaking changes ..

You can use the Deploy Previews with existing WalletConnect implementations. It's a non-breaking change

@ligi

This comment has been minimized.

Copy link
Member

commented May 13, 2019

both are breaking changes

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Ok I understand your motivation, but in this case I would propose to drop the -betaX from the version name ;), since for me as a developer that would be a sign that there will not be breaking changes anymore and it would motivate me to migrate from 0.7 to 1.0.0

This is not the reason why projects have taken this long to migrate from v0.7.x to v1.0.0. Most projects have other backlog priorities or are still testing the migration on staging

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

both are breaking changes

WCIP-2 is not a breaking change. It does not break when two clients of different versions interact.

Let's say WCIP-2 is introduced with beta.23 and you have a wallet with beta.22. If the Wallet does not respond to signing challenge then the exchangeKey flow is dropped by the Dapp. Same thing happens the other way around because if the Dapp does not have WCIP-2 implemented then the exchangeKey flow is not initiated at all.

@rmeissner

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

Most projects have other backlog priorities or are still testing the migration on staging

WalletConnect being in the beta state is a reason why it has a lower priority for us in the Safe iOS App development than other features.

Not saying that this will magically boost adoption, but if there is really a commitment to "no breaking changes" I would say lets remove the -betaX it is a good sign.

Edit: Lets ask the other way around, what are the reasons for having the beta state? ... also sorry this is probably something that should be discussed somewhere else :/

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Edit: Lets ask the other way around, what are the reasons for having the beta state? ... also sorry this is probably something that should be discussed somewhere else :/

Because I want to refactor the exchangeKey flow before having WalletConnect go through an audit. Hence why I want this change to introduced asap so that we can get the audit done and drop the beta tag.

This is a much simpler solution even if it's not as elegant as the alternative proposal.

@ligi

This comment has been minimized.

Copy link
Member

commented May 13, 2019

if the Wallet does not respond to signing challenge then the exchangeKey flow is dropped by the Dapp.

ok if this is optional it can be a non-breaking change. But I think then WCIP-2 should explicitly state that this is optional. I was not reading it this way.

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Alright, I will re-write the proposal to make it clearer of how it handles the JSON-RPC error responses.

pedrouid added some commits May 22, 2019

@pedrouid

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

Requires #121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.