-
Notifications
You must be signed in to change notification settings - Fork 259
[Feature] Load BIP-85 Child Seeds #785
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
base: dev
Are you sure you want to change the base?
Conversation
|
In the current draft, a child seed can generate its own bip85 child seeds. That is absolutely unacceptable to me and will need a check to prevent it. |
|
Also definitely need the TODO bullet point you list in the description to differentiate the child seeds in the list of seeds loaded in memory. |
2fede30 to
842e6cb
Compare
|
Thanks for the review, Keith. I’ve completed all my remaining TODO items for this PR and incorporated all the changes you suggested. The PR description has been updated to reflect the new changes, and this is now ready for another review. A couple of points for discussion:
|
It being info only and not actually navigating anywhere is definitely a violation of our UX rules. I think the parentage is well represented Seeds listing. So I think the
Then that info-only button isn't necessary anymore. |
|
But I wonder if the children should be sorted by their child index instead. vs |
|
I don't think altering the fingerprint color for child seeds is the best way to go. If there is any differentiation, maybe it should be via a custom icon designed by @easyuxd (see the "Finalize Child Seed" screenshot in the PR description). At the very least I'd leave it to Easy to weigh in on the color (stick with fingerprint blue or a diff alt color?). The arrows in "Select Signer" and "Verify Address" should also be reviewed. I think the arrow icon itself is fine, though something more like ↳ shaped might be better. And I don't think it should be shaded blue in "Verify Address". In fact, it's probably an implementation miss that one screen renders the fingerprints in the buttons white while in the other it's blue. Another thought: maybe the SeedOptionsView for a child (the "c1969012" screenshot) could replace the fingerprint icon with the big "#" icon and then add the child index number as I suggested above to give us: "#14: c1969012" |
0b62e43 to
b8d6ffc
Compare
|
Thanks again, Keith. I’ve made all the suggested changes (also updated screenshots in the PR desc):
On the icons:
In the meantime, until Easy has time to design the new icons, I’m also open to reverting to the fingerprint icon for the |
|
This will be a bit of annoying fussing about, but I'd prefer that the various screenshots that show the in-memory seeds list (there are about 4-5) stay "clean" with no bip85 children in the list. The rationale is that we'll be constantly leveraging these screenshots for tutorials and other materials so we should always have a beginner-friendly "clean" screenshot. And then add in whatever more advanced feature screenshots / variants we might need (e.g. PSBTOverviewView_op_return). So I would just create a single alternate version of "In-Memory Seeds" that includes a child. I'd place it just after SeedOptionsView_bip85_child_seed. I don't think we need a bip85'ed variant for all the other seed list screenshots. |
|
Here’s the latest set of changes after addressing the review comments:
Also, I've rebased the branch onto |
466c999 to
4dbd137
Compare
|
I realized there's an issue with storing Say the following seeds are loaded: In this case, the Now, suppose the user loads a child seed for Seed A: Here, the The point is that we should not store I’ve updated the PR to store the parent That said, I also think we should avoid passing around In my opinion, it would be much more robust to use the actual Looking ahead, if we ever support newer seed types, having direct |
|
@Chaitanya-Keyal I just reviewed this and tested it on my device. Looks quite great 👍 One thing on child seeds of child seeds: |
4a7677a to
732ae2f
Compare
bdefa12 to
47f5001
Compare
47f5001 to
2f016ed
Compare
Description
Fixes: #354.
This PR introduces support for directly loading BIP-85 generated seeds into the UX.
SeedBIP85FinalizeView, with this updated flow in mind.BIP85ChildSeedand refactors codebase to eliminate the need of passing aroundbip85_data.SeedOptionsView.New Screenshots
Other minor changes:
SeedDiscardView.SelectSeedViewsconsistent with other In-Memory Seeds views and use white for the fingerprint icon instead of blue.TODO
SeedOptionsViewand differentiate child seeds/show parent relationship in Seeds List Views.This pull request is categorized as a:
Checklist
pytestand made sure all unit tests pass before submitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os: