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

Add limited support for electrum segwit seeds #513

Merged
merged 22 commits into from
Jul 11, 2024
Merged

Conversation

BamaHodl
Copy link
Contributor

@BamaHodl BamaHodl commented Dec 4, 2023

Description

Add support for electrum segwit seeds (Issue #438 ) so that users with electrum seeds will not need to move utxos to a BIP-39 seed to use SeedSigner as their signing device.

It requires one new option to enable Electrum seed support in Advanced settings (disabled by default), which will allow the user the option to enter Electrum seed phrases on screens that allow the user to enter BIP-39 seed phrases. Per the PR comments, SeedQR has been disabled for Electrum seeds.

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

  • I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

  • No, I’m a fool
  • Yes
  • N/A

I have tested this PR on the following platforms/os:

@jdlcdl
Copy link

jdlcdl commented Dec 9, 2023

I've taken a first look at this pr as well as a single test:

  • Can recover a 12 word electrum seed, was offered to switch to "electrum", chose "custom extension" and recovered the correct bip32 wallet with my 13th word.
  • was able to backup/restore via seed words.
  • was able to backup/restore via 25x25 standard SeedQR (because compact couldn't possibly work w/o checksum bits).
  • receive and change addresses in address-explorer are matching electrum wallet (but I suspect this is a separate derivation path). address verification is not picking-up on this after scanning an address from electrum.

much more to check, but this is interesting for many I suspect. ty @BamaHodl

@jdlcdl
Copy link

jdlcdl commented Dec 18, 2023

As of commit 89f6cec,

I have reviewed, and tested, nothing newly broken.

For Address Verification, it appears to search standard native-segwit bip84 path instead of m/0', but I understand that for electrum seeds, not all functionality is expected to work.

@BamaHodl
Copy link
Contributor Author

For Address Verification, it appears to search standard native-segwit bip84 path instead of m/0', but I understand that for electrum seeds, not all functionality is expected to work.

My mistake, I was unaware it was not working for electrum seeds. The way the derivation path is set for address verification is to take a default derivation path for bip-39 seeds rather than looking it up based on the seed itself so I missed that one. A simple fix was made to simply check if the seed is electrum before doing the search, and override the derivation path if so. Address verification works with this latest change.

… to pass the is_electrum status to it to export xpub for electrum seeds properly
@jdlcdl
Copy link

jdlcdl commented Dec 18, 2023

Address verification works with this latest change.

as of 1f0a4dd, I confirm.

Very nice BamaHodl!

@jdlcdl
Copy link

jdlcdl commented Dec 19, 2023

Without testing and just with a glance of 289b382

I wonder if that commit, all by itself, might better be a refactoring pull-request separate from this one.

I just say that because it's sort of off topic from "electrum support", even if it does reduce the need for "is_electrum". A smaller pull-requests is easier to review and 'ack'. I'd hate to see some progress not go forward because it were waiting for other progress to be reviewed and accepted.

Up to you. Thanks again for nice improvements.

@BamaHodl
Copy link
Contributor Author

I see your reasoning from the procedure aspect--should probably be a PR on its own. Was just trying to clean it up a bit to avoid needing to do electrum-specific stuff there.
I've backed out that commit

@jamesturnernz
Copy link

Can confirm that this would be a useful comparability feature with a popular open source wallet. Being able to bring people into SS from Electurm would be useful. Thank you

@kdmukai
Copy link
Contributor

kdmukai commented Mar 3, 2024

@BamaHodl, can you find a sensible spot in the docs to add info about Electrum seed import? Especially clarification about which Electrum format is supported (I had no idea they updated to a version with the bip39 wordlist; should say that we don't support their older wordlist). Also note any other limitations/caveats when in this mode.

@kdmukai
Copy link
Contributor

kdmukai commented Mar 3, 2024

I'm very uncomfortable with the idea of facilitating SeedQR creation for an Electrum seed. I can confirm that it does work to scan back in. But it somewhat muddies the water of what a SeedQR encodes and would likely be wholly unsupported by other software/hardware that has adopted the SeedQR standard.

It seems like EITHER we:

  • push hard for Electrum seeds to be explicitly part of the SeedQR standard and advocate that all other implementations do the same (not my preference)
  • OR we, as the reference SeedQR implementation, keep the standard simple and focused.

My main interest in supporting this PR is to give users with Electrum seeds the ability to use their seed in SeedSigner, BUT if it's a clunkier, less convenient experience for them, so be it.

SeedSigner w/Electrum seed makes the most sense for rare cases of emergency signing / bailing you out of trouble. And to a lesser extent, external verification (e.g. address explorer). But Electrum seed usage in general should be discouraged. And maybe even more functionality should be disabled (e.g. a bip85 child seed derived from an Electrum seed parent will still yield a bip39 seed... or we can just say, "No, not even gonna go there").

@@ -427,6 +427,21 @@ def __post_init__(self):
)
self.components.append(self.fingerprint_icontl)

Copy link
Contributor

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.

Copy link
Contributor Author

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

def __post_init__(self):
self.show_back_button = False
self.title = "Switch to Electrum?"
self.is_bottom_list: bool = True
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

I am leaning strongly toward this approach now:

#513 (comment)

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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).
This also removes the need for a new screen altogether possibly, at least during seed entry

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:
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing an elif button_data[selected_menu_num] == self.CANCEL branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@BamaHodl BamaHodl Mar 4, 2024

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

only support electrum seeds while in electrum mode and only support bip-39 otherwise

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.

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",
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't say "It appears". It IS an Electrum seed, as far as we can tell. "switch" still bugs me.

This may be too extreme, but I'm slightly more inclined to a more drastic presentation:

Screenshot 2024-03-03 at 5 17 44 PM

Copy link
Contributor Author

@BamaHodl BamaHodl Mar 4, 2024

Choose a reason for hiding this comment

The 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

@kdmukai
Copy link
Contributor

kdmukai commented Mar 3, 2024

I haven't fully thought this through, but I feel like we should be able to isolate all of the Electrum-specific code in seed.py by subclassing Seed:

class Seed:
    ...

class ElectrumSeed(Seed):
    ...

That would then shift the "is it bip39 or is it Electrum" logic to https://github.com/BamaHodl/seedsigner/blob/dev/src/seedsigner/models/seed_storage.py#L90-L91 where if the Seed instantiation fails, we can try ElectrumSeed instead.

Also here if we support Electrum SeedQRs:
https://github.com/BamaHodl/seedsigner/blob/dev/src/seedsigner/views/scan_views.py#L75-L77

Perhaps a higher-level util could further abstract that away (e.g. a SeedParser in seed.py that returns either a Seed or an ElectrumSeed).

@@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

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

The hard-coding here and the limited Electrum support in embit_util.get_standard_derivation_path() seems pretty awkward, but I don't have an improvement suggestion yet.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

@3rdIteration
Copy link
Contributor

3rdIteration commented Mar 4, 2024

I had a look at this and do think it's a good idea to support importing these seeds. The main challenge with Electrum seeds is that 1/16 of them is also a valid BIP39 seed, so any support for them should be manually enabled, as opposed to prompting if the checksum is correct for Electrum. (Or should only offer to try Electrum if the BIP39 seed has an invalid checksum, but a valid Electrum version bit, which doesn't seem to be the logic in the PR at the moment.)

@kdmukai
Copy link
Contributor

kdmukai commented Mar 4, 2024

The main challenge with Electrum seeds is that 1/16 of them is also a valid BIP39 seed, so any support for them should be manually enabled, as opposed to prompting if the checksum is correct for Electrum.

Oof, agreed. I think maybe an explicit two-step process:

  • An optional advanced setting to enable support for Electrum seeds (disabled by default)
  • If enabled, that adds a new menu item to explicitly "Load Electrum Seed" (in the "Seed" menu). Then only in that specific flow do we even try to parse an Electrum seed; all other seed imports can remain strictly bip39.

@BamaHodl
Copy link
Contributor Author

BamaHodl commented Mar 4, 2024

1/16 of them is also a valid BIP39 seed

I believe it's more like 1 out of 16 million? Basically, if the hash of the seed starts with "100" in ascii, not binary (e.g. 3 full bytes must match, 1 in 2^24 probability)

Also, the reverse is not possible--electrum-generated seeds will never have a valid BIP-39 checksum because there is a check to discard those when electrum wallet generates seeds to avoid that confusion. But you are correct that 1 in 2^24 BIP-39 seeds will be a valid electrum seed--that's why I made it require user confirmation rather than just assuming they meant to enter an electrum seed. It's a very small chance, but we don't want to skip any edge cases by making assumptions for sure

@3rdIteration
Copy link
Contributor

3rdIteration commented Mar 4, 2024

1/16 of them is also a valid BIP39 seed

I believe it's more like 1 out of 16 million? Basically, if the hash of the seed starts with "100" in ascii, not binary (e.g. 3 full bytes must match, 1 in 2^24 probability)

Also, the reverse is not possible--electrum-generated seeds will never have a valid BIP-39 checksum because there is a check to discard those when electrum wallet generates seeds to avoid that confusion. But you are correct that 1 in 2^24 BIP-39 seeds will be a valid electrum seed--that's why I made it require user confirmation rather than just assuming they meant to enter an electrum seed. It's a very small chance, but we don't want to skip any edge cases by making assumptions for sure

Electrum only added this check in mid 2021. (And other wallets which generate Electrum compatible seeds may not check it at all) Basically any segwit compatible electrum seed generated before then has a 1/16 chance of being a valid BIP39 seed too.

See here: spesmilo/electrum#6001

@berlinxray
Copy link

berlinxray commented May 21, 2024

Today, I tested this promising addition to Seedsigner and managed to sign a transaction.
Unfortunately, Electrum, Sparrow and Bluewallet could't parse the animated QR-Code to import the signed transaction.
Saved Electrum seeds don't seem to remember their electrum origin... and following transactions cannot be signed.
I would like to see the option to add a BIP39 passphrase to an Electrum seed.
Looking forward to this valuable addition to Seedsigner!

@berlinxray
Copy link

berlinxray commented Jun 20, 2024

Finally, I managed to sign a transaction with Sparrow wallet, Seedsigner and an Electrum seed :-)
Sending the signed transaction via QR-Code to Sparrow was successful, broadcasting too...

My previous problems may have been caused by a large transaction size and Electrum having trouble reading animated QR-Codes...
I forgot Electum calls seed passwords a "seed extension". This change does add an option for a custom extension. I apologise for my ignorance.

@BamaHodl
Copy link
Contributor Author

Finally, I managed to sign an transaction with Sparrow wallet, Seedsigner and an Electrum seed :-) Sending the signed transaction via QR-Code to Sparrow was successful, broadcasting too...

My previous problems may have been caused by a large transaction size and Electrum having trouble reading animated QR-Codes... I forgot Electum calls seed passwords a "seed extension". This change does add an option for a custom extension. I apologise for my ignorance.

Thanks for the update, and apologies for not seeing your prior comment! Glad you were able to both get the tx signing and custom extension features working using Sparrow as your coordinator. It should be noted that the official Electrum wallet software is not yet a supported coordinator for the SeedSigner, so you are correct that loading and signing transactions with Electrum software will not work currently, in particular because as you mentioned animated QR formats are not yet supported. For my use what I do is load my Electrum seed into the SeedSigner using this new feature, export the xpub into a supported coordinator software (Sparrow is my favorite), and then everything works as expected as any other seed would.

I am taking another look at this PR and will get it merge-ready again at least from a testing and merge conflict standpoint so that we can hopefully get this in the main repo and a future production release if all goes well.

@berlinxray
Copy link

I am taking another look at this PR and will get it merge-ready again at least from a testing and merge conflict standpoint so that we can hopefully get this in the main repo and a future production release if all goes well.

That would be great! Thanks!

@3rdIteration
Copy link
Contributor

@BamaHodl I did some work on adding full legacy support that I think I will bring in another PR. (As there didn't seem to be any objections)

Basically once this is in there, the rest of the legacy Electrum stuff will pretty much "just work" with minimal extra work.

@BamaHodl
Copy link
Contributor Author

@BamaHodl I did some work on adding full legacy support that I think I will bring in another PR. (As there didn't seem to be any objections)

That's awesome! Just curious what exactly you mean when you refer to legacy, do you mainly mean supporting p2pkh script type? That was my main reason for not attempting any of the other electrum seed types other than native segwit, namely p2pkh not currently implemented in seedsigner. If we can get p2pkh support then adding the older electrum seed types should be relatively straight-forward. These are the electrum seed types I found in my research:

Name Wordlist Num Words Script Type Electrum version
Native SegWit BIP-39 12 p2wpkh >=3.0
Standard BIP-39 12 p2pkh or multi-sig p2sh >=2.7 and <3.0
Standard BIP-39 13 p2pkh or multi-sig p2sh >=2.0 and <2.7
"Old" Legacy 12 p2pkh <2.0

- Not applicable for Electrum seed types
- SeedQR backups
- Since Electrum seeds are not supported by other SeedQR implementations, it would be dangerous to use SeedQR as a backup tool for Electrum seeds and is thus disabled
- Custom derivations
Copy link

@jdlcdl jdlcdl Jun 29, 2024

Choose a reason for hiding this comment

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

I can understand not doing this for bip85 because the user might NOT be expecting a bip39 mnemonic. But wondering why no custom derivation for electrum seeds??? Is it about electrum not using standard derivation paths?

Copy link
Contributor

Choose a reason for hiding this comment

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

Electrum doesn't use standard derivation paths, or let users edit them, they are basically hardcoded based on the seed type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to explicitly add the hard coded derivations in SeedSigner in this document.

Suggestion:

  • Custom Derivations
    • Hard coded derivation path and script types in SeedSigner to match Electrum. These are m/0h for single sig and m/1h for multisig to match Electrum

@newtonick
Copy link
Collaborator

@BamaHodl we are planning a pre release soon. Hopefully before the end of July. Would you be able to resolve the merge conflict on this PR (It looks like just a whitespace issue) before I do code review and testing? Thanks!

@BamaHodl
Copy link
Contributor Author

@BamaHodl we are planning a pre release soon. Hopefully before the end of July. Would you be able to resolve the merge conflict on this PR (It looks like just a whitespace issue) before I do code review and testing? Thanks!

Fixed

@newtonick
Copy link
Collaborator

A lesser known feature of SeedSigner is that it can actually scan and decode a single frame PSBT QR (Base43) from Electrum. This feature has never been advertised or really supported, but I remember working on it. It wasn't really practical since there was not Electrum supported way to get the signed PSBT back into Electrum.

image

@newtonick
Copy link
Collaborator

newtonick commented Jul 2, 2024

Overall I'm good with this PR minus a few changes. I really hope @kdmukai finds time to update his review of this PR. I have a feeling there might be something he'd want to see updated since all the changes @BamaHodl has made since the beginning of March. I appreciate this great contribution @BamaHodl!

Copy link
Collaborator

@newtonick newtonick left a comment

Choose a reason for hiding this comment

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

Please add import for WarningScreen to tools_view.py. Without this dependency an error is thrown in Address Explorer for Electrum seeds.

src/seedsigner/views/tools_views.py Show resolved Hide resolved
- Not applicable for Electrum seed types
- SeedQR backups
- Since Electrum seeds are not supported by other SeedQR implementations, it would be dangerous to use SeedQR as a backup tool for Electrum seeds and is thus disabled
- Custom derivations
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to explicitly add the hard coded derivations in SeedSigner in this document.

Suggestion:

  • Custom Derivations
    • Hard coded derivation path and script types in SeedSigner to match Electrum. These are m/0h for single sig and m/1h for multisig to match Electrum

docs/electrum.md Outdated Show resolved Hide resolved
src/seedsigner/models/settings_definition.py Outdated Show resolved Hide resolved
@3rdIteration
Copy link
Contributor

I'm happy to add the code to support legacy Electrum addresses in a separate PR (That is trivial once this and the other Legacy address type PR are merged)

@BamaHodl
Copy link
Contributor Author

BamaHodl commented Jul 8, 2024

I'm happy to add the code to support legacy Electrum addresses in a separate PR (That is trivial once this and the other Legacy address type PR are merged)

I'm also going to attempt integrating the old Standard seed types that use P2PKH. Might be able to accomplish full historical electrum seed support now with your changes. The difficulty with supporting them all will be the UX. Will have to give the user options to use 12 word seed phrases, 13 word seed phrases (with a warning this is NOT the custom extension), and the option to load from the old, pre-2.0 wordlist (i.e. not BIP-39 wordlist) to call it full coverage.

@3rdIteration
Copy link
Contributor

I'm happy to add the code to support legacy Electrum addresses in a separate PR (That is trivial once this and the other Legacy address type PR are merged)

I'm also going to attempt integrating the old Standard seed types that use P2PKH. Might be able to accomplish full historical electrum seed support now with your changes. The difficulty with supporting them all will be the UX. Will have to give the user options to use 12 word seed phrases, 13 word seed phrases (with a warning this is NOT the custom extension), and the option to load from the old, pre-2.0 wordlist (i.e. not BIP-39 wordlist) to call it full coverage.

Yea the really old seeds are certainly a bit of an issue UX wise. It could be that a separate PR is required which allows users to enable/disable different seed types in a similar manner to how script types are enabled/disabled. (Which would also make it easier to add other things like SLIP39 or other future seed standards which may emerge)

BamaHodl and others added 2 commits July 8, 2024 09:39
Update to clarify Electrum Segwit derivations and add link to Elecrum seed versioning documentation
…s and correct WarningScreen import in tools_views.py
@BamaHodl BamaHodl requested a review from newtonick July 8, 2024 14:55
@BamaHodl
Copy link
Contributor Author

BamaHodl commented Jul 8, 2024

I'm happy to add the code to support legacy Electrum addresses in a separate PR (That is trivial once this and the other Legacy address type PR are merged)

playing around with the older electrum p2pkh seeds here with the addition of your legacy script support PR: https://github.com/BamaHodl/seedsigner/tree/LegacyElectrum

I haven't tested everything yet but so far it's as you said: it just works without having to change much.

@newtonick
Copy link
Collaborator

tACK

@newtonick newtonick merged commit 3e0b69b into SeedSigner:dev Jul 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged Not Yet Released
Development

Successfully merging this pull request may close these issues.

8 participants