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

Handle Moonbeam accounts (Ethereum compatible) #787

Closed
joelamouche opened this issue Jul 28, 2021 · 9 comments · Fixed by #797
Closed

Handle Moonbeam accounts (Ethereum compatible) #787

joelamouche opened this issue Jul 28, 2021 · 9 comments · Fixed by #797

Comments

@joelamouche
Copy link

joelamouche commented Jul 28, 2021

NB: Moonbeam is an ethereum compatible parachain which uses unified accounts (https://docs.moonbeam.network/learn/unified-accounts/), which means the same ethereum key can be used to sign evm tx and substrate tx.

Problem

Currently, to use the parity signer with moonbeam, I can create an ethereum address and import it via QR code into the app.

  1. When I click 'ethereum' in the main menu, the bottom bar containing "Qr scanner" and "Derive New Account" doesn't display like it does for other chains. This prevents the user from signing transactions and having more than one account

  2. Ideally, we would like 'Moonbeam', 'Moonriver' and 'Moonbase' to be displayed in the list of networks in the "add network account" action (like centrifuge), which would be ethereum style addresses specifying network to the app in orde to prevent the "this account is available on all networks" warning

How can we fix that? I am willing to help

@Slesarew
Copy link
Contributor

I'm sorry, we are dropping ethereum compatibility here. And React Native as well. There are many ethereum-compatible wallets, we are focusing on making the Signer ultimately secure for substrate-based networks. So the next release will be substrate-only and quite different on the inside.

Unless somebody is willing to re-write at least basic ethereum features in Rust and maintain them. Feel free to fork, of course - the main branch will be forked into some legacy branch to be replaced with descendant of https://github.com/paritytech/parity-signer/tree/ios-native-port

@crystalin
Copy link

crystalin commented Aug 31, 2021

@Slesarew I don't understand why you are not supporting Ethereum compatible Substrate chains. It is supported by PolkadotJS and the rust libraries to support it are already developed (and Substrate uses those like ECDSA for default signature support)

Would it be possible to reconsider your decision ? People are adding our chain on Parity Signer but their account is broken.
Worst case could you prevent adding our chain on parity signer to avoid people complaining it doesn't work ?

@Slesarew
Copy link
Contributor

Slesarew commented Sep 1, 2021

why you are not supporting Ethereum compatible Substrate chains.

That is quite different from what this complaint was about. Sorry about the confusion. Everything Substrate-compatible would be supported as long as somebody maintains Signer updates through mechanism outlined here https://github.com/paritytech/parity-signer/tree/ios-native-port/rust/generate_message - please feel free to start testing generation on that branch! You can build prototype Signer from there or send me a qr code you generate, I'll let you know if it works. Of course, don't use it on real accounts, it's not tested properly yet!

Mind you that these upgrades could be generated by anyone (and signed), anyone trusting that signature would be able to use them. Or users could generate upgrades for themselves if they like. So testing this mechanism (and its manual) would be very benefitial for everyone.

As long as it it "ethereum-based", it is dropped. This is really classified by https://github.com/maciejhirsz/uos spec - if the payload starts with 53, it is supported, if it starts with something else, it's not. Thus, this was about sending ethereum-style payloads to Signed and I dropped it; Substrate-style messages will work out of the box as long as someone maintains the upgrades (at least it's planned that way).

The reason for this is that we are hardening against #724 and related things by actually downloading the whole metadata into signer while keeping it airgapped.

prevent adding our chain on parity signer

That is out of my control for now, we didn't have a release in long time and we are not releasing anything until this work is done. New Signer will have only few built-in networks and network maintainers will define what's available, as adding networks would be only done through scanning.

@joelamouche
Copy link
Author

@Slesarew To clarify: we want to be able to sign a substrate payload (generated by polkadot-js and displayed as a QR code for the mobile app to read) with the ethereum-compatible ECDSA algo.

So you would need to be able to create h160 ecdsa addresses, and sign payloads with them.

Before, the change was pretty easy because Parity Signer was using the keyring lib, which can generate h160 addresses and sign payloads. But I heard the new rust based parity signer refactor is dropping TS use and keyring in particular.

So the question is wether or not you would be willing to include the ECDSA rust library and use it for ethereum compatible parachains

That is out of my control for now

DO you mind refering us the lead on this project s that we can discuss with them on the possibility of including this.

Might be easier to talk on element:

@antoine_moonbeam:matrix.org

@Tbaut
Copy link
Contributor

Tbaut commented Sep 1, 2021

Before, the change was pretty easy because Parity Signer was using the keyring lib

Just a comment here, there's a misunderstanding. Parity Signer has never used JS based Keyring, the signing part has always been in Rust, both for substrate and ethereum accounts. The recent change gets rid of TS but doesn't change much in terms of signing. In any case since the signing part is implemented in Substrate, supporting Moonbeam is not doing something entirely exotic.

@Slesarew
Copy link
Contributor

Slesarew commented Sep 1, 2021

So you would need to be able to create h160 ecdsa addresses, and sign payloads with them.

Yeah, we already have it supported in backend, just need to pull it out in a sane manner. ECDSA is one of standard crypto algorithms and already could be used even to sign metadata updates!

Before, the change was pretty easy because Parity Signer was using the keyring lib, which can generate h160 addresses and sign payloads

Unfortunately not, implementing that in old framework was actually my first attempt to fix Signer and was not easy at the end. After writing a whole signing module in Rust, I had to move it gently in quite overloaded IU, as it was not there at all, as @Tbaut correctly mentions

@Slesarew Slesarew linked a pull request Sep 21, 2021 that will close this issue
@Slesarew
Copy link
Contributor

@joelamouche please have a look at this util in its current state, it will be used to import moonbeam and all other networks to Signer, now cryptography could be bound to network and Signer will just be used normally.

https://github.com/paritytech/parity-signer/tree/metadata-v14/rust/generate_message

@joelamouche
Copy link
Author

@Slesarew That looks good!
Q1: So users won't load the Metadata on the polkadot-js anymore? We will have to display the QR code on our website?
Q2: Is there anyway to test this as of now?

@Slesarew
Copy link
Contributor

Slesarew commented Oct 12, 2021

Q1: So users won't load the Metadata on the polkadot-js anymore? We will have to display the QR code on our website?

Yes, there will be a separate place to present these codes; I think (hope) polkadot-js might mirror it, so can anyone. Extension will probably also have these codes easily available.

Q2: Is there anyway to test this as of now?

We are finalizing UI for iOS which means that other than building this repo manually for iOS (which is trivial but of course requires XCode, test device, and rust toolchain as described in readme) or joining test group (which is not possible until things get more or less stable-ish) there is no way to test it in Signer. But please test whether you will be able to generate those QR videos for your network using the link in my previous comment here; any feedback would be very valuable.

Edit: All this might fail, though, if #819 is not ready (coming soon) - I haven't tested it with Moonbeam yet; together, these features should make Signer properly universal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants