-
Notifications
You must be signed in to change notification settings - Fork 179
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
Add limited support for electrum segwit seeds #513
Changes from 7 commits
c1d320f
89f6cec
eaa29ab
1f0a4dd
289b382
656110b
f8aca10
8f0a8fe
40520f8
8c9fd5c
1820997
2355c53
ce9113f
0fc3ce4
8a83f32
c0f1ad6
b430d50
d483864
a28f162
4ea2d72
46061c5
75d9f5f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -427,6 +427,21 @@ def __post_init__(self): | |
) | ||
self.components.append(self.fingerprint_icontl) | ||
|
||
class SeedSwitchElectrumModeScreen(ButtonListScreen): | ||
button_data: list = None | ||
|
||
def __post_init__(self): | ||
self.show_back_button = False | ||
self.title = "Switch to Electrum?" | ||
self.is_bottom_list: bool = True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Switch" isn't quite the right verb here. Would "Electrum Seed Detected" fit in the title? If not, the title can just say "Electrum Seed" since the body text will have more info. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same response as below--agree on removing switch language but it is in a tiny doubt if the user meant to enter BIP-39 or electrum. Alternatively we could check if it meets both checksums and only have a confirmation screen in that edge case, and go with the Electrum Seed declaration and warning if it is a valid electrum seed but not a valid BIP-39 checksum. Does that sound good? Confirmation screen if it meets both checks, warning screen if it's electrum-only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am leaning strongly toward this approach now: In which case there is no ambiguity about what the user is trying to do. Metaphorically: enter via the "Electrum seed" door or gtfo with that Electrum seed! (similarly, no attempt(?) should be made to even check if it's a valid bip39 seed if entered via the Electrum seed flow) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, It's making more sense to me too, especially after learning the collisions are more often than I had initially thought. And in this way it keeps the BIP-39 flow completely unaffected by default even when they happen to collide with a valid Electrum seed (apparently 9/2048 chance which is much higher than I had calculated). |
||
|
||
super().__post_init__() | ||
|
||
self.components.append(TextArea( | ||
text="It appears this is an Electrum-style seed, switch it to be so? Some functions are not supported for Electrum seeds", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree we can make the warning more extreme and remove the 'switch' wording. But there is a small possibility it is in fact a BIP-39 seed, which is why I put a confirmation screen |
||
screen_y=self.top_nav.height + GUIConstants.COMPONENT_PADDING, | ||
is_text_centered=True, | ||
)) | ||
|
||
|
||
@dataclass | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -289,6 +289,9 @@ def run(self): | |
class SeedFinalizeView(View): | ||
FINALIZE = "Done" | ||
PASSPHRASE = "BIP-39 Passphrase" | ||
CUSTOM_EXTENSION = "Custom Extension" | ||
SWITCH = "Switch" | ||
CANCEL = "Cancel" | ||
|
||
def __init__(self): | ||
super().__init__() | ||
|
@@ -297,9 +300,26 @@ def __init__(self): | |
|
||
|
||
def run(self): | ||
if self.seed.electrum_seed_bytes: | ||
# see if user wants to enter electrum mode since it's a valid electrum seed | ||
button_data = [self.SWITCH, self.CANCEL] | ||
selected_menu_num = self.run_screen( | ||
seed_screens.SeedSwitchElectrumModeScreen, | ||
button_data=button_data | ||
) | ||
if button_data[selected_menu_num] == self.SWITCH: | ||
self.seed.switch_to_electrum() | ||
# recalculate fingerprint | ||
self.fingerprint = self.seed.get_fingerprint(network=self.settings.get_value(SettingsConstants.SETTING__NETWORK)) | ||
elif not self.seed.seed_bytes: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It falls through and behaves correctly (i.e. tries to treat it as BIP-39), but agree it can be more clear at least with a elif with a comment there There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a user select "Cancel", they're explicitly saying, "Oh, never mind, I don't want to load this Electrum seed." So I think the better behavior would be to discard the seed and just return us to Home. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you mean now. I had intended the flow to be -- if it's valid electrum seed, ask the user if they intended that and if not, then fall through and try to treat it as BIP-39. So in this new case it should have 3 buttons then--treat as Electrum, treat as Bip-39, and cancel (or just remove cancel altogether). If the preferred way is to do the 2-step (which I'm leaning more to myself), it could be that the user puts the SS in "Electrum mode" in advanced settings, and then we don't even need an extra screen--only support electrum seeds while in electrum mode and only support bip-39 otherwise, i.e. behavior doesn't change at all for bip-39 seeds in default mode even if their seed collides with a valid electrum seed. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's too restrictive for me. In my mind, SeedSigner has a core bip39 foundation. Electrum seed support may be added on the side, but it should never override/exclude the main bip39 use cases. That's why I like the explicit "Load Electrum Seed" option; it provides a single, specific path for this one side option. And if you go through the normal bip39 mechanisms and load a mnemonic that could also be a valid Electrum seed, it doesn't matter, we should never even check the Electrum side in the normal flows. Doesn't exist. |
||
# we only got here because it was a valid electrum seed | ||
# thus if user didn't mean to enter an electrum seed, it's invalid | ||
raise InvalidSeedException(f"Invalid seed entered") | ||
|
||
button_data = [self.FINALIZE] | ||
passphrase_button = self.CUSTOM_EXTENSION if self.seed.is_electrum else self.PASSPHRASE | ||
if self.settings.get_value(SettingsConstants.SETTING__PASSPHRASE) != SettingsConstants.OPTION__DISABLED: | ||
button_data.append(self.PASSPHRASE) | ||
button_data.append(passphrase_button) | ||
|
||
selected_menu_num = self.run_screen( | ||
seed_screens.SeedFinalizeScreen, | ||
|
@@ -311,7 +331,7 @@ def run(self): | |
seed_num = self.controller.storage.finalize_pending_seed() | ||
return Destination(SeedOptionsView, view_args={"seed_num": seed_num}, clear_history=True) | ||
|
||
elif button_data[selected_menu_num] == self.PASSPHRASE: | ||
elif button_data[selected_menu_num] == passphrase_button: | ||
return Destination(SeedAddPassphraseView) | ||
|
||
|
||
|
@@ -323,7 +343,8 @@ def __init__(self): | |
|
||
|
||
def run(self): | ||
ret = self.run_screen(seed_screens.SeedAddPassphraseScreen, passphrase=self.seed.passphrase) | ||
passphrase_title="Custom Extension" if self.seed.is_electrum else "BIP-39 Passphrase" | ||
ret = self.run_screen(seed_screens.SeedAddPassphraseScreen, passphrase=self.seed.passphrase, title=passphrase_title) | ||
|
||
if ret == RET_CODE__BACK_BUTTON: | ||
return Destination(BackStackView) | ||
|
@@ -612,7 +633,9 @@ def __init__(self, seed_num: int, sig_type: str): | |
def run(self): | ||
from .tools_views import ToolsAddressExplorerAddressTypeView | ||
args = {"seed_num": self.seed_num, "sig_type": self.sig_type} | ||
if len(self.settings.get_value(SettingsConstants.SETTING__SCRIPT_TYPES)) == 1: | ||
seed = self.controller.storage.seeds[self.seed_num] | ||
script_types = [SettingsConstants.NATIVE_SEGWIT] if seed.is_electrum else self.settings.get_value(SettingsConstants.SETTING__SCRIPT_TYPES) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This confused me a bit as a user. What's the rationale for not supporting other single sig script types w/Electrum seeds? And if it has to stay limited to Native Segwit, I think a notification screen might be needed (e.g. "Note: Only native segwit is supported for Electrum seeds" w/a "Next" button). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In electrum wallet the versioning indicates the script type. Ascii "100" version means native segwit and only native segwit. I may be able to add legacy seeds eventually as well, but I've only worked on native segwit version so far, so allowing that option wouldn't be useful until supporting legacy seed types. There is no electrum seed version for taproot yet either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think we need to tell the user that right now they're only going to get native segwit addrs. Then later if/when more options are available, it's the normal button list. Consider the user who's trying to get legacy types but doesn't realize the limitation. It'd be pure frustration that the UI just skips straight to native segwit. |
||
if len(script_types) == 1: | ||
# Nothing to select; skip this screen | ||
args["script_type"] = self.settings.get_value(SettingsConstants.SETTING__SCRIPT_TYPES)[0] | ||
|
||
|
@@ -799,7 +822,8 @@ def run(self): | |
derivation_path = embit_utils.get_standard_derivation_path( | ||
network=self.settings.get_value(SettingsConstants.SETTING__NETWORK), | ||
wallet_type=self.sig_type, | ||
script_type=self.script_type | ||
script_type=self.script_type, | ||
is_electrum=self.seed.is_electrum | ||
) | ||
|
||
if self.settings.get_value(SettingsConstants.SETTING__XPUB_DETAILS) == SettingsConstants.OPTION__DISABLED: | ||
|
@@ -814,10 +838,11 @@ def run(self): | |
|
||
try: | ||
embit_network = NETWORKS[SettingsConstants.map_network_to_embit(self.settings.get_value(SettingsConstants.SETTING__NETWORK))] | ||
version = embit.bip32.detect_version( | ||
derivation_path, | ||
default="xpub", | ||
network=embit_network | ||
version = embit_utils.detect_version( | ||
derivation_path, | ||
self.settings.get_value(SettingsConstants.SETTING__NETWORK), | ||
self.sig_type, | ||
self.seed.is_electrum | ||
) | ||
root = embit.bip32.HDKey.from_seed( | ||
self.seed.seed_bytes, | ||
|
@@ -845,6 +870,7 @@ def run(self): | |
dict(seed_num=self.seed_num, | ||
coordinator=self.coordinator, | ||
derivation_path=derivation_path, | ||
sig_type=self.sig_type | ||
) | ||
) | ||
|
||
|
@@ -854,7 +880,7 @@ def run(self): | |
|
||
|
||
class SeedExportXpubQRDisplayView(View): | ||
def __init__(self, seed_num: int, coordinator: str, derivation_path: str): | ||
def __init__(self, seed_num: int, coordinator: str, derivation_path: str, sig_type: str = SettingsConstants.SINGLE_SIG): | ||
super().__init__() | ||
self.seed = self.controller.get_seed(seed_num) | ||
|
||
|
@@ -884,7 +910,9 @@ def __init__(self, seed_num: int, coordinator: str, derivation_path: str): | |
network=self.settings.get_value(SettingsConstants.SETTING__NETWORK), | ||
qr_type=qr_type, | ||
qr_density=qr_density, | ||
wordlist_language_code=self.seed.wordlist_language_code | ||
wordlist_language_code=self.seed.wordlist_language_code, | ||
is_electrum=self.seed.is_electrum, | ||
sig_type=sig_type | ||
) | ||
|
||
|
||
|
@@ -1299,7 +1327,7 @@ def run(self): | |
num_modules_standard = 29 | ||
num_modules_compact = 25 | ||
|
||
if self.settings.get_value(SettingsConstants.SETTING__COMPACT_SEEDQR) != SettingsConstants.OPTION__ENABLED: | ||
if seed.is_electrum or self.settings.get_value(SettingsConstants.SETTING__COMPACT_SEEDQR) != SettingsConstants.OPTION__ENABLED: | ||
# Only configured for standard SeedQR | ||
return Destination( | ||
SeedTranscribeSeedQRWarningView, | ||
|
@@ -1650,6 +1678,9 @@ def __init__(self, seed_num: int = None): | |
else: | ||
self.seed = None | ||
self.address = self.controller.unverified_address["address"] | ||
# override derivation path if it's electrum as bip-39 path was set by default | ||
if self.seed and self.seed.is_electrum: | ||
self.controller.unverified_address["derivation_path"] = 'm/0h' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The hard-coding here and the limited Electrum support in |
||
self.derivation_path = self.controller.unverified_address["derivation_path"] | ||
self.script_type = self.controller.unverified_address["script_type"] | ||
self.sig_type = self.controller.unverified_address["sig_type"] | ||
|
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.
Our opinionated / slightly non-standard formatting: 3 blank lines between classes or other elements at the top nesting level, 2 blank lines between functions or other elements at nested levels.
We're not fully consistent with that everywhere, but mostly so.
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 will make the spacing consistent--was just an oversight