feat: add support for 24 word recovery phrases #1346
Conversation
…word count on wallet creation.
WalkthroughThis pull request introduces a new activity for selecting the wallet's security level, along with corresponding UI and string resource updates. The changes include a new layout for presenting word count options, modifications to string resources to support both 12-word and 24-word recovery phrases, and updates to wallet creation methods to accept a seed word count. Additionally, minor adjustments are made to enforce context non-nullability and to update mnemonic word count validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OnboardingActivity
participant SelectSecurityLevelActivity
participant OnboardingViewModel
participant WalletFactory
User->>OnboardingActivity: Click "Create New Wallet"
OnboardingActivity->>SelectSecurityLevelActivity: Launch security level selection
SelectSecurityLevelActivity->>OnboardingActivity: Return selected seed word count
OnboardingActivity->>OnboardingViewModel: Call createNewWallet(onboardingInvite, seedWordCount)
OnboardingViewModel->>WalletFactory: create(networkParams, seedWordCount)
WalletFactory-->>OnboardingViewModel: Return created wallet
OnboardingViewModel-->>OnboardingActivity: Wallet creation success
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
wallet/src/de/schildbach/wallet/util/MnemonicCodeExt.kt (1)
63-64: LGTM! Consider documenting security implications.The changes correctly implement BIP39 support for both 12 and 24-word recovery phrases. The validation logic and error message are accurate.
Consider adding a code comment explaining the security implications of choosing between 12 and 24 words (128 vs 256 bits of entropy) to help developers understand the trade-offs.
@Throws(MnemonicException::class) fun check(context: Context, words: List<String>) { + // BIP39 supports 12 words (128 bits) or 24 words (256 bits) of entropy + // 24 words provide higher security at the cost of longer phrases if (words.size != 12 && words.size != 24) { throw MnemonicException.MnemonicLengthException("Word list size must be 12 or 24")wallet/src/de/schildbach/wallet/ui/onboarding/SelectSecurityLevelActivity.kt (3)
8-10: Add state restoration for activity recreation.The selected word count should be preserved when the activity is recreated (e.g., on configuration changes).
class SelectSecurityLevelActivity : SecureActivity() { private lateinit var binding: ActivityPhrasewordcountBinding private var selectedWordCount: Int = 12 + + override fun onSaveInstanceState(outState: Bundle) { + super.onSaveInstanceState(outState) + outState.putInt(EXTRA_WORD_COUNT, selectedWordCount) + } + + override fun onRestoreInstanceState(savedInstanceState: Bundle) { + super.onRestoreInstanceState(savedInstanceState) + setMode(savedInstanceState.getInt(EXTRA_WORD_COUNT, 12)) + }
22-28: Add content descriptions for accessibility.The word count options should be accessible to screen readers.
binding.twelveWordsOption.setOnClickListener { setMode(12) } + binding.twelveWordsOption.contentDescription = getString(R.string.twelve_words_option_description) binding.twentyFourWordsOption.setOnClickListener { setMode(24) } + binding.twentyFourWordsOption.contentDescription = getString(R.string.twenty_four_words_option_description)
49-51: Define word count constants in the companion object.Extract the hard-coded word count values into named constants for better maintainability.
companion object { const val EXTRA_WORD_COUNT = "extra_word_count" + const val WORD_COUNT_12 = 12 + const val WORD_COUNT_24 = 24 + const val DEFAULT_WORD_COUNT = WORD_COUNT_12 }wallet/res/layout/activity_phrasewordcount.xml (1)
87-88: Fix inconsistent ID naming.The IDs for the 24-word option use "non_contested" prefix which seems unrelated to its purpose. Consider using consistent naming like:
-android:id="@+id/non_contested_name_title" +android:id="@+id/twenty_four_words_option_title" -android:id="@+id/non_contested_name_description" +android:id="@+id/twenty_four_words_option_description"Also applies to: 96-97
wallet/res/values/strings.xml (1)
420-421: Ensure Consistent Naming & Clear Messaging for New Recovery Option StringsThe new string resources have been added to support the recovery phrase feature. However, note that one string is named with a "_message" suffix ("select_security_level_12_words_message") and the other with a "_title" suffix ("select_security_level_24_words_title"). Please verify that this naming difference is intentional and that the UI components display them appropriately. Also, consider if the label "Very secure" clearly communicates that it corresponds to a 24-word recovery phrase option, in comparison to the plain "12 word recovery phrase" text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
wallet/AndroidManifest.xml(1 hunks)wallet/res/layout/activity_phrasewordcount.xml(1 hunks)wallet/res/values/strings-extra.xml(2 hunks)wallet/res/values/strings.xml(1 hunks)wallet/src/de/schildbach/wallet/service/WalletFactory.kt(3 hunks)wallet/src/de/schildbach/wallet/ui/OnboardingActivity.kt(4 hunks)wallet/src/de/schildbach/wallet/ui/OnboardingViewModel.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/utils/GoogleDriveService.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/onboarding/SelectSecurityLevelActivity.kt(1 hunks)wallet/src/de/schildbach/wallet/util/MnemonicCodeExt.kt(1 hunks)
🔇 Additional comments (4)
wallet/src/de/schildbach/wallet/service/WalletFactory.kt (2)
33-33: Verify the secure random number generation implementation.The code imports both
LinuxSecureRandomandSecureRandom. WhileSecureRandomis used for seed generation, ensure thatLinuxSecureRandomis properly initialized for enhanced entropy on Linux systems.Also applies to: 53-53
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if LinuxSecureRandom is initialized elsewhere rg -A 5 "LinuxSecureRandom"Length of output: 1843
Secure Random Initialization Verified
The search confirms that
LinuxSecureRandomis actively instantiated inWalletApplication.java(see thenew LinuxSecureRandom();line), ensuring enhanced entropy on Linux systems. This initialization meets the secure random number generation requirement.
- Location:
wallet/src/de/schildbach/wallet/WalletApplication.java— Proper instantiation ofLinuxSecureRandom.- Implication:
WalletFactory.ktimportsLinuxSecureRandom, relying on the initialization already performed in the application entry point, which is appropriate.
104-119: Verify the entropy calculation for 24-word phrases.The implementation doubles the entropy bits for 24-word phrases. This aligns with BIP-39 specifications where:
- 12 words = 128 bits of entropy
- 24 words = 256 bits of entropy
However, let's verify this is the standard approach.
✅ Verification successful
🌐 Web query:
What is the correct entropy for 24-word BIP39 mnemonic phrases?💡 Result:
The correct entropy for a 24-word BIP39 mnemonic phrase is 256 bits of cryptographic randomness[3][4][9]. This value adheres to BIP39 specifications and ensures compatibility with wallets supporting this standard.
Key details:
Entropy requirements:
- Must be 256 bits (32 bytes) long[3][4][9].
- Must be a multiple of 32 bits (required by BIP39's checksum mechanism)[9][10].
- Converted through SHA-256 hashing to generate an 8-bit checksum[8].
Conversion process:
Entropy (256 bits) + Checksum (8 bits) = Total (264 bits) 264 bits ÷ 11-bit word indexes = 24 words[9][11]Security implications:
- Provides equivalent security to 2^256 possible combinations[3][7]
- Requires physical dice rolls or other true randomness sources for secure generation[6][7]
For comparison:
Word Count Entropy Bits Checksum Bits 12 128 4 24 256 8 This structure ensures mathematical validity while maintaining human-readable backup capabilities[4][9].
Citations:
- 1: https://www.cs.utexas.edu/~moore/acl2/manuals/current/manual/index-seo.php?xkey=BITCOIN____BIP39-ENTROPY-TO-MNEMONIC&path=4368%2F2215%2F6197%2F11870%2F14684
- 2: https://github.com/massmux/HowtoCalculateBip39Mnemonic
- 3: https://www.ledger.com/academy/topics/security/what-is-the-entropy
- 4: https://www.ledger.com/academy/bip-39-the-low-key-guardian-of-your-crypto-freedom
- 5: https://docs.rs/tiny-bip39/latest/bip39/struct.Mnemonic.html
- 6: https://selfcustody.github.io/krux/getting-started/usage/generating-a-mnemonic/
- 7: https://github.com/bluescan/bip39tools/blob/main/README.md
- 8: https://www.cs.utexas.edu/~moore/acl2/manuals/current/manual/index-seo.php?xkey=BITCOIN____BIP39-ENTROPY-TO-WORD-INDEXES&path=4369%2F2215%2F6197%2F11871
- 9: https://thebitcoinmanual.com/articles/seed-phrase-entropy/
- 10: https://learnmeabitcoin.com/technical/keys/hd-wallets/mnemonic-seed/
24-word mnemonic entropy calculation confirmed
The code correctly doubles the 128-bit entropy value (used for 12-word phrases) to reach 256 bits for 24-word phrases, which is in line with BIP-39 specifications.
wallet/AndroidManifest.xml (1)
100-104: LGTM!The activity declaration follows the same pattern as other similar activities in the app.
wallet/res/values/strings-extra.xml (1)
125-125: LGTM!The validation message has been correctly updated to support both 12 and 24-word phrases.
| fun createNewWallet(onboardingInvite: InvitationLinkData?, seedWordCount: Int) { | ||
| walletApplication.initEnvironmentIfNeeded() | ||
| val wallet = walletFactory.create(Constants.NETWORK_PARAMETERS) | ||
| val wallet = walletFactory.create(Constants.NETWORK_PARAMETERS, seedWordCount) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate seedWordCount parameter.
Add validation to ensure only supported word counts (12 or 24) are accepted.
fun createNewWallet(onboardingInvite: InvitationLinkData?, seedWordCount: Int) {
+ require(seedWordCount == 12 || seedWordCount == 24) { "Unsupported word count: $seedWordCount" }
walletApplication.initEnvironmentIfNeeded()
val wallet = walletFactory.create(Constants.NETWORK_PARAMETERS, seedWordCount)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun createNewWallet(onboardingInvite: InvitationLinkData?, seedWordCount: Int) { | |
| walletApplication.initEnvironmentIfNeeded() | |
| val wallet = walletFactory.create(Constants.NETWORK_PARAMETERS) | |
| val wallet = walletFactory.create(Constants.NETWORK_PARAMETERS, seedWordCount) | |
| fun createNewWallet(onboardingInvite: InvitationLinkData?, seedWordCount: Int) { | |
| require(seedWordCount == 12 || seedWordCount == 24) { "Unsupported word count: $seedWordCount" } | |
| walletApplication.initEnvironmentIfNeeded() | |
| val wallet = walletFactory.create(Constants.NETWORK_PARAMETERS, seedWordCount) | |
| } |
| fun getSigninAccount(context: Context?): GoogleSignInAccount? { | ||
| val opts = getGoogleSigninOptions() | ||
| val account = GoogleSignIn.getLastSignedInAccount(context) | ||
| val account = GoogleSignIn.getLastSignedInAccount(context!!) |
There was a problem hiding this comment.
Replace non-null assertion with safe null handling.
Using !! operator can lead to runtime crashes. Consider handling null context gracefully.
fun getSigninAccount(context: Context?): GoogleSignInAccount? {
val opts = getGoogleSigninOptions()
- val account = GoogleSignIn.getLastSignedInAccount(context!!)
+ if (context == null) {
+ log.warn("getSigninAccount called with null context")
+ return null
+ }
+ val account = GoogleSignIn.getLastSignedInAccount(context)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun getSigninAccount(context: Context?): GoogleSignInAccount? { | |
| val opts = getGoogleSigninOptions() | |
| val account = GoogleSignIn.getLastSignedInAccount(context) | |
| val account = GoogleSignIn.getLastSignedInAccount(context!!) | |
| fun getSigninAccount(context: Context?): GoogleSignInAccount? { | |
| val opts = getGoogleSigninOptions() | |
| if (context == null) { | |
| log.warn("getSigninAccount called with null context") | |
| return null | |
| } | |
| val account = GoogleSignIn.getLastSignedInAccount(context) | |
| // ... rest of the implementation | |
| } |
| private val selectWordCountLauncher = registerForActivityResult(ActivityResultContracts.StartActivityForResult()) { result -> | ||
| if (result.resultCode == RESULT_OK) { | ||
| val onboardingInvite = intent.getParcelableExtra<InvitationLinkData>(EXTRA_INVITE) | ||
| viewModel.createNewWallet( | ||
| onboardingInvite, | ||
| result.data!!.extras!!.getInt(SelectSecurityLevelActivity.EXTRA_WORD_COUNT) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential null values safely.
Using double non-null assertions (!!) on result.data and extras can lead to runtime crashes.
private val selectWordCountLauncher = registerForActivityResult(ActivityResultContracts.StartActivityForResult()) { result ->
if (result.resultCode == RESULT_OK) {
+ val wordCount = result.data?.extras?.getInt(SelectSecurityLevelActivity.EXTRA_WORD_COUNT)
+ if (wordCount == null) {
+ log.error("Failed to get word count from SelectSecurityLevelActivity result")
+ return@registerForActivityResult
+ }
val onboardingInvite = intent.getParcelableExtra<InvitationLinkData>(EXTRA_INVITE)
viewModel.createNewWallet(
onboardingInvite,
- result.data!!.extras!!.getInt(SelectSecurityLevelActivity.EXTRA_WORD_COUNT)
+ wordCount
)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private val selectWordCountLauncher = registerForActivityResult(ActivityResultContracts.StartActivityForResult()) { result -> | |
| if (result.resultCode == RESULT_OK) { | |
| val onboardingInvite = intent.getParcelableExtra<InvitationLinkData>(EXTRA_INVITE) | |
| viewModel.createNewWallet( | |
| onboardingInvite, | |
| result.data!!.extras!!.getInt(SelectSecurityLevelActivity.EXTRA_WORD_COUNT) | |
| ) | |
| } | |
| } | |
| private val selectWordCountLauncher = registerForActivityResult(ActivityResultContracts.StartActivityForResult()) { result -> | |
| if (result.resultCode == RESULT_OK) { | |
| val wordCount = result.data?.extras?.getInt(SelectSecurityLevelActivity.EXTRA_WORD_COUNT) | |
| if (wordCount == null) { | |
| log.error("Failed to get word count from SelectSecurityLevelActivity result") | |
| return@registerForActivityResult | |
| } | |
| val onboardingInvite = intent.getParcelableExtra<InvitationLinkData>(EXTRA_INVITE) | |
| viewModel.createNewWallet( | |
| onboardingInvite, | |
| wordCount | |
| ) | |
| } | |
| } |
| <androidx.constraintlayout.widget.ConstraintLayout | ||
| android:id="@+id/twelve_words_option" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toBottomOf="@+id/register_panel" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginTop="15dp" | ||
| android:background="@drawable/light_grey_border_item" | ||
| android:paddingHorizontal="20dp" | ||
| android:paddingVertical="12dp"> | ||
|
|
||
| <TextView | ||
| android:id="@+id/twelve_words_option_title" | ||
| style="@style/Subtitle1" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:text="@string/select_security_level_12_words_title" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toTopOf="parent" /> | ||
|
|
||
| <TextView | ||
| android:id="@+id/twelve_words_option_message" | ||
| style="@style/Caption.Secondary" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginEnd="20dp" | ||
| android:text="@string/select_security_level_12_words_message" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toBottomOf="@id/twelve_words_option_title" /> | ||
| </androidx.constraintlayout.widget.ConstraintLayout> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add content descriptions for accessibility.
Both option containers need content descriptions to improve accessibility for screen readers.
<androidx.constraintlayout.widget.ConstraintLayout
android:id="@+id/twelve_words_option"
+ android:contentDescription="@string/select_security_level_12_words_title"
...>
<androidx.constraintlayout.widget.ConstraintLayout
android:id="@+id/twenty_four_words_option"
+ android:contentDescription="@string/select_security_level_24_words_title"
...>Also applies to: 74-104
wallet/res/values/strings-extra.xml
Outdated
| <string name="select_security_level_title">Select Security Level</string> | ||
| <string name="select_security_level_message">You can secure your wallet with a 12 or 24 word recovery phrase.</string> | ||
| <string name="select_security_level_12_words_title">Secure</string> | ||
| <string name="select_security_level_24_words_message">24 word recovery phrase</string> | ||
| </resources> |
There was a problem hiding this comment.
Add missing string resource.
The string select_security_level_12_words_message is referenced in the layout but not defined here.
Add the missing string resource:
<string name="select_security_level_12_words_title">Secure</string>
+<string name="select_security_level_12_words_message">12 word recovery phrase</string>
<string name="select_security_level_24_words_message">24 word recovery phrase</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <string name="select_security_level_title">Select Security Level</string> | |
| <string name="select_security_level_message">You can secure your wallet with a 12 or 24 word recovery phrase.</string> | |
| <string name="select_security_level_12_words_title">Secure</string> | |
| <string name="select_security_level_24_words_message">24 word recovery phrase</string> | |
| </resources> | |
| <string name="select_security_level_title">Select Security Level</string> | |
| <string name="select_security_level_message">You can secure your wallet with a 12 or 24 word recovery phrase.</string> | |
| <string name="select_security_level_12_words_title">Secure</string> | |
| <string name="select_security_level_12_words_message">12 word recovery phrase</string> | |
| <string name="select_security_level_24_words_message">24 word recovery phrase</string> | |
| </resources> |
feat: add support for 24 word recovery phrases, add option to choose word count on wallet creation.
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit