-
Notifications
You must be signed in to change notification settings - Fork 237
[Refactor] use _Seed_cls to handle seed type #807
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
PR description should document the fact that |
updated the PR: |
seed = ElectrumSeed(self._pending_mnemonic) | ||
else: | ||
seed = Seed(self._pending_mnemonic) | ||
seed = self._pending_Seed_cls(self._pending_mnemonic) | ||
return seed.get_fingerprint(network) | ||
except InvalidSeedException: | ||
return None |
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.
This new approach LGTM now
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 like the flexibility the seed_class
parameter permits now. Tested ACK on Raspberry Pi OS Manual Build as of 0198399 .
Description
Added
_pending_Seed_cls
reference to handle both Seed and ElectrumSeed.Removed unused
validate_mnemonic()
function.Updated
init_pending_mnemonic
to accept seed_class instead ofis_electrum
, making the code more extensible for future seed types (e.g., Shamir Secret, [Feature] Add SLIP-39 Shamir's secret sharing import support for SeedSigner #636).eliminates conditional logic throughout the codebase. instead of checking boolean flags and then deciding which class to instantiate, we directly store the class.
this follows the same pattern as Destination class adnd makes the code cleaner and less error prone.
This pull request is categorized as a:
Checklist
pytest
and made sure all unit tests pass before sumbitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os: