-
Notifications
You must be signed in to change notification settings - Fork 238
[Feature]: Predict Last Seed Word #800
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?
[Feature]: Predict Last Seed Word #800
Conversation
2d9a899
to
e23e1af
Compare
563b607
to
5282821
Compare
Concept ACK, but implementation NACK. This would also limit the reusability of |
TIL, that there are different mnemonic formats. I have made this PR exclusively for BIP-39.
this can be fixed easily by adding a condition to ensure
i believe these are all niche features and are disabled by default. we can handle them the same way as described above. almost forgot, thank you for reviewing this PR @alvroble ! that said, lemme know what others think. |
@notTanveer thank you for your work!
This would mean disabling the functionality of predicting the last word in all cases where Another option is to directly use different screen and view for Electrum and other (to be supported) mnemonic phrase formats. This option would completely solve the problem, but I like it even less because it would duplicate a lot of code and would be the messy way of doing it.
Yes, it’s true that this feature is rarely used nowadays, but we shouldn’t allow functionality that SeedSigner already supports to break. Actually... I think the best approach would be to have a new class attribute in the Seed class (e.g. |
759a57a
to
df56291
Compare
we have two possible approaches:
let me know which one do you guys prefer? |
How about changing the current
At the mnemonic entry stage, we only set a pending mnemonic, not a pending seed. Since the seed instance is only created after all valid words have been entered, there’s no seed instance available at that point to check the type. So that approach doesn’t seem viable to me. |
thanks for the suggestion, @Chaitanya-Keyal and @alvroble ! updated the PR. feels like this is the best way forward. |
e472df3
to
6f27161
Compare
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.
Tested ACK on Raspberry Pi OS Manual Build as of 6f27161. I like this approach too.
There is too much bitcoin-related work being done in People have asked seriously about a version of SeedSigner for blind users. If and when that time comes, we'd need very different "Screen" implementations to present the data (perhaps a digital braille device or just a text-to-speech flow). But the goal is that all the important logic would already be contained within the Views, so swapping in alternate Screens should be kind of trivial. So... all of this wordlist filtering needs to happen in the View. |
I've been unhappy with our existing bip39 vs Electrum juggling in def init_pending_mnemonic(self, num_words:int = 12, is_electrum:bool = False):
self._pending_mnemonic = [None] * num_words
self._Seed_cls = Seed if not is_electrum else ElectrumSeed So then later we can avoid this: if self._pending_is_electrum:
seed = ElectrumSeed(self._pending_mnemonic)
else:
seed = Seed(self._pending_mnemonic) And just do: seed = self._Seed_cls(self._pending_mnemonic) I haven't tested this nor deeply explored if it creates any problems elsewhere. Also note that This all might make more sense as a separate refactor PR. |
the this test was causing the entire test suite to hang indefinitely during execution. the issue occurred at the final word entry stage, where the test attempted to input invalid final words ("zoo" and "zebra"). these entries triggered the |
a02faf1
to
b725a8b
Compare
class SeedStorage: | ||
PENDING_SEED_TYPE__BIP39 = "bip39" | ||
PENDING_SEED_TYPE__ELECTRUM = "electrum" | ||
# future TODO: PENDING_SEED_TYPE_SLIP39 = "slip39" |
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 think a deeper refactor would be better, SeedStorage should stay agnostic of seed types. The responsibility for type should live in the Seed classes themselves, and polymorphism should handle the differences.
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.
will be updated once #807 is in
b725a8b
to
d317dad
Compare
d317dad
to
f0cc53f
Compare
Description
fixes #500 .
displays only checksum-valid words when manually entering the final word of a seed phrase.
core implementation
get_valid_final_mnemonic_words()
inmnemonic_generation.py
technical details
this enhances the UX by filtering out invalid words (which don’t meet the checksum criteria) and makes typing faster by showing fewer possible matches.
EDIT:
the test_invalid_mnemonic test was removed because it tests behavior that is no longer possible with the new seed word prediction feature.
this test was causing the entire test suite to hang indefinitely during execution. the issue occurred at the final word entry stage, where the test attempted to input invalid final words ("zoo" and "zebra"). these entries triggered the get_valid_final_mnemonic_words() function, but since neither word exists in the computed valid word set, the test framework became stuck in an infinite loop
This pull request is categorized as a:
Checklist
pytest
and 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: