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

Support 12-word (128 bit) BIP39 mnemonic strings #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LeChatNoir69
Copy link

"entropy" parameter has been added to "karlsenwallet create" function to allow 12-words or 24-words phrase (default : 24-words).

…o allow 12-words or 24-words phrase (default : 24 words).
@lemois-1337 lemois-1337 self-requested a review January 12, 2024 00:13
@lemois-1337
Copy link
Collaborator

@LeChatNoir69 Many thanks for this pull request. Could you also extend the tests for it so that they test 128 and 256 bits?

cmd/karlsenwallet/libkaspawallet/transaction_test.go
cmd/karlsenwallet/daemon/server/split_transaction_test.go

There are still a lot of tests broken and we will fix them in near future, but lets fix it already for newly introduced features.

@LeChatNoir69
Copy link
Author

If no arg are passed to the function, it considers 256 bits by default. I'm ok to update tests functions but not sure from where theses functions must get the right length ?

@lemois-1337
Copy link
Collaborator

There should be 3 tests then:

  1. enforced with argument of 256 bits and check that result is correct.
  2. enforced with argument of 128 bits and check that result is correct.
  3. original test without argument, but ensure that the result always complies to 256 bit.

Imagine the case someone accidentally change the default from 256 to 128 with a pull request. The autotest - once the GitHub workflows are updated - will immediately fail and no manual test is needed.

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.

2 participants