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

Issue614 #720

Closed
wants to merge 11 commits into from
Closed

Issue614 #720

wants to merge 11 commits into from

Conversation

Slesarew
Copy link
Contributor

@Slesarew Slesarew commented Feb 8, 2021

feat!: add minimal support for multisignature Ed25519 algorithm, as mentioned in issue #614.

BREAKING CHANGE: upgrade is not possible, the app should be installed on a clean phone and identities re-generated as described in original manual

Ecdsa support as placeholder.

Refs #614

@maciejhirsz
Copy link
Contributor

Code looks good to me, can try to check the UI later. It would be nice to have some working test vectors in the rust codebase in RN substrate sign, plus the formatting there is a bit off.

What's the blocker from making this not backwards-compatible? Sr25519 is still supported after all. Anyone running signer on a hot phone with auto updates on will get their accounts bricked, which isn't exactly a nice UX, and wiping application data in iOS at least isn't something most users do.

@Slesarew
Copy link
Contributor Author

Slesarew commented Feb 9, 2021

Code looks good to me, can try to check the UI later. It would be nice to have some working test vectors in the rust codebase in RN substrate sign, plus the formatting there is a bit off.

Yeah, I really wanted some comments on this if it is going in the right direction first. This is minimally working code, but I would rather refactor the rust signer libs properly so that they are easy to read, with no repetitive code chunks and similar to Substrate reference libs. UI is also just bare minimum, only account details show what's happening - as far as I understood the ideas behind this app, the goal is to make it minimally distractive after all.

What's the blocker from making this not backwards-compatible? Sr25519 is still supported after all. Anyone running signer on a hot phone with auto updates on will get their accounts bricked, which isn't exactly a nice UX, and wiping application data in iOS at least isn't something most users do.

Actually, just removal of couple trivial lines of code. I decided to leave it in this state after short talk with @kirushik - indeed ideally the app should not be upgraded linearly. But now I realize that the user should probably have freedom to violate this guideline.

@novasamatech novasamatech deleted a comment from cla-bot-2021 bot Jun 3, 2021
@Slesarew
Copy link
Contributor Author

Obsolete; similar functionality already added in backend of staging branch

@Slesarew Slesarew closed this Jul 30, 2021
@Slesarew Slesarew deleted the issue614 branch January 19, 2022 21:47
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.

2 participants