[#19232] - Fix derivation path generation #19531
Conversation
Jenkins BuildsClick to see older builds (67)
|
|
@OmarBasem - since this pr is around derivation paths and you have a lot of knowledge in that area, do you think you could give it a quick happy path test on the functionality added? |
There was a problem hiding this comment.
Thanks for your PR @ulisesmac!
I have tried testing out the PR. I tried to create a new keypair and a new account and it worked fine. But when I tried to create another account using one of the existing keypairs it did not work. After I enter the password it gets stuck -- nothing happens.
IMG_4506.MP4
@OmarBasem thank you so much for spotting this issue! Yeah, makes sense, since I'm not considering that case, I thought it wasn't implemented. I'll revisit the implementation and fix it. |
Hey @OmarBasem @J-Son89 , a quick update on it, I'm currently working on it. The new account generation based on a keypair is incomplete or buggy in develop. No matter if we pick any keypair, the code always generates an account for the root/main/primary? keypair stored, the one for our main account. Besides the account creation, I think the keypair picker needs a lot of work, sometimes it's confusing what is being picked. Screencast.from.2024-04-08.17-42-34.webmIt took me a while to realize what the problems were and how to properly use it, as said before, I'm working on actually creating an account given a selected keypair. TBH I believe this is kind of a separate issue, but I don't want to merge this implementation without being complete, also because it'll require an immediate follow-up, so I'm addressing it in this PR. 👍 |
ulisesmac
force-pushed
the
19232-fix-derivation-path-creation
branch
from
d3472e1 to
d33b0dc
Compare
@ulisesmac it is working fine for me on develop. It was QA'ed in this PR #19333. Creating account from new keypair or an existing keypair works as expected. |
yeah it's a little unclear to me what issues exactly you are pointing to @ulisesmac ? 🤔 |
|
It's unclear for me what is the correct flow to derive an address for a keypair. I'll explain develop behaviour with a (replicable) example. Expected(?) (Status Desktop behaves like this):
Current (status mobile behavior):
And all accounts marked with |
|
@J-Son89 @OmarBasem |
|
@ulisesmac I think you are referring to a different issue. Creating a new account from a new keypair or an existing keypair works as expected on develop. Whether the derivation path is accurate or not shouldn't affect the functionality. |
ulisesmac
force-pushed
the
19232-fix-derivation-path-creation
branch
2 times, most recently
from
8f51bab to
6297318
Compare
ulisesmac
force-pushed
the
19232-fix-derivation-path-creation
branch
5 times, most recently
from
649a2e2 to
5bbc9d5
Compare
|
Hi @ulisesmac, thanks a lot for the fixes! Case:
The expected result in the issue is saying: "The Derivation path for the newly created account should be the same as it was when the account was removed" Could you please confirm, that is NOT correct? IIRC we asked you about this on the offsite, and your answer was that if the account is removed, we should continue to generate new accounts with derivation paths in the same order, that is for Acc3 it should be m/44'/60'/0'/0/3 (and that's how it works both in nightly and PR) I'm asking for a clarification again because the description of this PR indicates that #19232 is fixed and you mentioned there that
However, I don't see any changes in this PR compared to nightly. And on the other hand, it looks expected to me. PR: video_2024-05-22_15-59-27.mp4Does it mean that only the part of the issue where the UI froze when creating a new account was fixed? I'll update the issue description if needed just to make it clear. |
|
Hi @qoqobolo Sorry for not being clear enough in this PR, I'll explain it with more detail.
Yeah, I think these steps aren't not enough to replicate the issue
Exactly, this is NOT correct.
Yeah,
Maybe that's because of the specific case you tested, but they don't behave the same, I recorded a video showing the error in the latest of develop: Screencast.from.2024-05-22.10-35-26.webmAnd this is the same case in this PR: Screencast.from.2024-05-22.10-42-38.webm
Yes, I fixed the UI freezing, but the issue's description isn't correct, sorry, I should've updated it to properly describe the problem. Now, regarding to this PR, the issue shown in the first video I shared becomes worse when we generate new keypairs, the derivation path just makes no sense when deriving new accounts for new keypairs. This is being fixed in this PR. Additionally, in develop it looks as if we are generating new keypairs, but we are not, so this PR also fixes the keypair generation, this fix is very important, but I'm not sure what you can do to test that there ARE NOT new keypairs added in develop. It can be seen as an internal fix in the app, the important thing is that now new keypairs and new derived accounts are working. |
ulisesmac
force-pushed
the
19232-fix-derivation-path-creation
branch
from
1a6d3a3 to
5818b9f
Compare
|
@ulisesmac thank you very much for the detailed explanation! Everything is clear and I see that the issue is deeper than it seemed before
Yes, unfortunately, I don't know either but if you checked it yourself, it should be enough. Regarding testing the PR, I've checked the following cases:
What I couldn't test is the same cases for the And let me know if you have smth else in mind that I can test. |
This one is very interesting, I guess that's because you need to provide the seed phrase for every new keypair you added. and probably you are not able to send transactions too. I think those flows are being covered by the Wallet Settings team, but just guessing that's the issue. CC: @smohamedjavid any idea of what might be the issue here? |
Yeah, we already clarified that and we'll open an issue for it, basically it wasn't properly updated in a past PR and it also needs some fixes that are being done in this PR. The test cases are good enough, thanks for taking the time! 👍 |
|
You are right @ulisesmac.
@qoqobolo - If the primary device creates any new keypair, the secondary device will get only the key pair details without the keystore file. The secondary device should import the missing keypair from the device it was created (primary device in our case) or import using the Recovery phrase or Private key to perform any transactions on those accounts. P.S. A sneak peek of the feature. We can check the missing keypairs in settings |
|
@ulisesmac @smohamedjavid thanks for your clarifications guys! @ulisesmac no issues from my side, PR can be merged, thanks again for the amazing work! |
|
@ulisesmac @qoqobolo Amazing work!!! |
Fix hidden hook usage Improve logging for unsupported collectibles Fix re-rendering of collectibles in flat-list chore: clean up implementation and speed up animation remove apply animations to style chore: use default opacity chore: use default opacity chore: add issue to todo chore: use flex chore: flex Avatars/Community Avatar Component (#20147) Avatars/dApp Avatar Component (#20145) Update eth-archival pokt url Remove not implemented Notification settings from community longtap m… (#20169) pick between JSC & Hermes for Android (#20171) We implement both `JSC` and `Hermes` in build phase of `Android` which increases our `APK` size by ~ `2 MB`. This was fine before but currently we have to get below the `100 MB` limit. This commit implements the preferred engine after inferring `hermesEnabled` property from gradle.properties This property is modified at build time for release here https://github.com/status-im/status-mobile/blob/178d62bd276afffef5fe7a3f773e390d83336d9c/nix/mobile/android/build.nix#L17 and set for debug here https://github.com/status-im/status-mobile/blob/178d62bd276afffef5fe7a3f773e390d83336d9c/Makefile#L280 Which should further reduce the `APK` size by `2 MB`. [#19232] - Fix derivation path generation and keypair creation (#19531) * Add more default dependencies to slide button * Fix wallet account creation: derivation paths and keypairs
fixes #19232
fixes #19472
Summary
The original issue was about fixing derivation paths (#19232)
but while trying to solve it, I found more bugs that didn't let me proceed, so now this PR is fixing everything.
Please keep in mind that:
is still an issue, we need to clarify some things with desktop, but it's too much for this PR.
Fixes in this PR
m/44'/60'/0'/0/0) and they are incremented according to its keypair (again, keep in mind these accounts cannot be removed).Also refactored the code, e.g I moved some code to events, updated the related tests, improve the name of some events, etc.
Platforms
Steps to test
Derivation Path
All should work as expected, so that #19232 is no longer reproducible.
Keypair generation and derived accounts
Open Status
Create a new account
Go to wallet tab -> Add some new derived accounts for the existing (default) keypair
Add as many keypairs as you wish
To do this:
Press on "Edit"
Press on
+Generate new keypair
Add derived accounts for those new keypairs
Note: the UI to pick the keypair sometimes may not work as expected make sure the keypair is being properly selected, this is a different issue
Everything should work and the derivation paths are incremented according to the keypair selected.
NOTE for QA: @churik Has more context on these issues, since we already talked about it.
status: ready