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
Added multisig wallets to the Android application #2279
Conversation
…t multisig wallet creation.
…aining the master public key).
…od; some updates to Daemon model.
…ialog to match the approach used in the Send dialog.
… transactions for multisig wallets.
… again from the same wallet.
Thanks, this looks very good. I'll post a more detailed review tomorrow. |
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.
I haven't tested everything yet, but here are my comments so far. These are mostly layout and style issues which should be easy to fix.
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/Send.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/Send.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/Main.kt
Outdated
Show resolved
Hide resolved
Sorry for being so slow, I'll have more comments soon. Meanwhile, it would be a good idea to merge from the Electron-Cash master branch and test the result to make sure your changes still work with the current version. |
No problem. I'm also a bit busier these days, but I'm working on the requested changes. Will do that. |
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.
Final comments:
android/app/src/main/java/org/electroncash/electroncash3/Transactions.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/Send.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
OK, that's the last batch. Overall I think you've done a great job, especially since this is your first contribution to the project. I'm sure the users are going to like it. |
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.
All of your changes yesterday look good, except for this one:
android/app/src/main/java/org/electroncash/electroncash3/Main.kt
Outdated
Show resolved
Hide resolved
…y KeystoreDialog is handled for multiple cosigners.
… dilaog argument; updated the way closeDialogs() works.
…d ColdLoad; added a comment about 'keystores' in NewWallet.
…ransaction dialogs in Send.kt
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.
The ColdLoad updates look mostly fine, just a few more comments:
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/ColdLoad.kt
Outdated
Show resolved
Hide resolved
…stead of string comparison; fixed an issue with status text wrapping.
…log title for signing of loaded transactions.
With my latest commits, I think I have addressed all of the comments so far. I'm now waiting on final reviews and potential changes/fixes. Thank you for your help and additional clarification on some of the issues. I have also learned a lot by working on this project and from the reviews, and I'm glad to be able to contribute to this codebase. |
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.
No problem, I'm happy to help. This was an ambitious piece of work for someone new to the project, and I think you've done about as well as anyone could have.
Here are a few remaining issues with NewWallet. I'll review the transaction details updates separately.
android/app/src/main/java/org/electroncash/electroncash3/NewWallet.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/NewWallet.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/NewWallet.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/NewWallet.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/electroncash/electroncash3/NewWallet.kt
Outdated
Show resolved
Hide resolved
…thod; updated the way default progress and max values are set for multisig sliders.
… moved risky argument modification from doInBackground() to onPostExecute()
This pull request implements the multisig wallet feature into the Android application, in addition to some other changes. The main changes are as follows: