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

Warn user if imported key has derivation path different from xpub's depth #2454

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

roshii
Copy link
Contributor

@roshii roshii commented Jun 2, 2024

Fixes #397

Copy link

netlify bot commented Jun 2, 2024

Deploy Preview for specter-desktop-docs canceled.

Name Link
🔨 Latest commit 10ea8a3
🔍 Latest deploy log https://app.netlify.com/sites/specter-desktop-docs/deploys/67634465c2d9c20008a6034c

@roshii roshii marked this pull request as draft June 2, 2024 20:27
@k9ert k9ert requested a review from stepansnigirev June 11, 2024 10:03
@k9ert
Copy link
Collaborator

k9ert commented Jun 11, 2024

@stepansnigirev maybe you can have a look whether that PR goes in the right direction.

@roshii
Copy link
Contributor Author

roshii commented Jun 11, 2024

for the record: I marked it as draft since I want to ensure users are properly notified - so far it is only printing a line to the logs

@stepansnigirev
Copy link
Collaborator

Makes sense. We should notify about these inconsistencies at least in the logs. Warnings in the UI would be even better.

@roshii
Copy link
Contributor Author

roshii commented Aug 19, 2024

I did try to have this notification sent top UI but my frontend knowledge is pretty limited. Since I will not have enough time to investigate UI modification, I will leave it as is, with a simple warning in the logs.
Hope this helps.

@roshii roshii marked this pull request as ready for review August 19, 2024 07:40
Copy link
Collaborator

@moneymanolis moneymanolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like there is off-by-one error, testing with this:

[8c24a510/84h/1h/0h]vpub5Y24kG7ZrCFRkRnHia2sdnt5N7MmsrNry1jMrP8XptMEcZZqkjQA6bc1f52RGiEoJmdy1Vk9Qck9tAL1ohKvuq3oFXe3ADVse6UiTHzuyKx

image

I've logged derivation_path out:

['m', '84h', '1h', '0h']

So this is where the mismatch comes from, it is counting the root whereas the depth of the xpub is counting from the root. @roshii wanna fix it?

Propagating the error to the UI is simple, just raise an Exception instead of the log statement (or in addition):

       raise Exception(
               f"xpup has a depth of {depth} while derivation path "
               f"indicates the key is {len(derivation_path)} levels deep"
           )

This code is taking the exceptions and their strings and propagates them to src/cryptoadvance/specter/server_endpoints/devices.py:

    @classmethod
    def parse_xpubs(cls, xpubs):
        xpubs = xpubs
        lines = [l.strip() for l in xpubs.split("\n") if len(l) > 0]
        failed = []
        keys = []
        for line in lines:
            try:
                keys.append(Key.parse_xpub(line))
            except Exception as e:
                failed.append(line + "\n" + str(e))
        return keys, failed
        

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.

warn user if imported key has derivation path different from xpub's depth
4 participants