373 - add entry point for testing onboarding flow #1898
Conversation
|
⏳ Analyzing changes in this PR... ⏳ This might take a few minutes, please wait 📥 CommitsAnalyzing changes from base (
📁 Files being considered (30)🔄 app/build.gradle (1 hunk) autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- f8ccfdb: test: add entry point for testing onboarding flow
Files Processed (3)
- app/src/main/java/com/flowfoundation/wallet/page/wallet/presenter/WalletHeaderPresenter.kt (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/reactnative/ReactNativeActivity.kt (3 hunks)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/BridgeModels.kt (2 hunks)
Actionable Comments (3)
-
app/src/main/java/com/flowfoundation/wallet/page/wallet/presenter/WalletHeaderPresenter.kt [79-84]
best practice: "Temporary test code should not be committed"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/ReactNativeActivity.kt [169-173]
possible bug: "Missing null check in screen type handling"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/ReactNativeActivity.kt [206-210]
possible bug: "Missing null check in screen type handling"
Skipped Comments (0)
| cvReceive.setOnClickListener { | ||
| // TEMPORARY: Launch onboarding flow for testing | ||
| // TODO: Remove this and restore original receive functionality | ||
| ReactNativeActivity.launch(view.context, RNBridge.ScreenType.ONBOARDING) | ||
| // Original code: ReceiveActivity.launch(view.context) | ||
| } |
There was a problem hiding this comment.
The code contains a temporary modification of the receive button functionality for testing purposes. This should not be committed to the codebase as it overrides the actual receive functionality. Consider using feature flags or test configurations instead of modifying production code for testing.
| val screenString = when (it) { | ||
| RNBridge.ScreenType.SEND_ASSET -> "send-asset" | ||
| RNBridge.ScreenType.TOKEN_DETAIL -> "token-detail" | ||
| RNBridge.ScreenType.ONBOARDING -> "onboarding" | ||
| } |
There was a problem hiding this comment.
The when expression doesn't handle the case where screenType might be null. Consider adding an else branch or making the when expression exhaustive by handling all possible cases to prevent potential runtime errors.
| val screenString = when (screenType) { | ||
| RNBridge.ScreenType.SEND_ASSET -> "send-asset" | ||
| RNBridge.ScreenType.TOKEN_DETAIL -> "token-detail" | ||
| RNBridge.ScreenType.ONBOARDING -> "onboarding" | ||
| } |
There was a problem hiding this comment.
Similar to the previous issue, this when expression doesn't handle the case where screenType might be null. Add an else branch or make the expression exhaustive to prevent potential runtime errors.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- e9f8b25: feat: extending bridge for existing onboarding flow
Files Processed (2)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/BridgeModels.kt (3 hunks)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt (1 hunk)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [742-767]
security: "Remove debug logging of sensitive information"
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [779-780]
possible issue: "Potential username collision in account creation"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [868-874]
enhancement: "Missing activity state handling"
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 27b7188: pull from dev
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt (1 hunk)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [775-775]
possible bug: "Potential null pointer exception in string operation"
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [803-803]
possible issue: "Username generation could cause collisions"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [900-900]
maintainability: "Hard-coded Android version check"
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 8a7649d: feat: onboarding ui
Files Processed (2)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/BridgeModels.kt (2 hunks)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt (2 hunks)
Actionable Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [850-850]
security: "Potential security issue with username generation"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [1019-1038]
possible issue: "Missing default case handling in screen security level switch"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [877-877]
possible bug: "Potential null pointer exception in mnemonic handling"
| ioScope { | ||
| try { | ||
| // Auto-generate username using timestamp | ||
| val username = "user_${System.currentTimeMillis()}" |
There was a problem hiding this comment.
The username generation using raw timestamp (user_${System.currentTimeMillis()}) could be predictable and potentially create collisions. Consider adding randomization and validation to ensure uniqueness and safety of generated usernames.
| when (level) { | ||
| "secure" -> { | ||
| // Prevent screenshots and screen recording | ||
| currentActivity.window.setFlags( | ||
| android.view.WindowManager.LayoutParams.FLAG_SECURE, | ||
| android.view.WindowManager.LayoutParams.FLAG_SECURE | ||
| ) | ||
| android.util.Log.d(TAG, "setScreenSecurityLevel() - FLAG_SECURE enabled") | ||
| } | ||
| "normal" -> { | ||
| // Allow screenshots again | ||
| currentActivity.window.clearFlags( | ||
| android.view.WindowManager.LayoutParams.FLAG_SECURE | ||
| ) | ||
| android.util.Log.d(TAG, "setScreenSecurityLevel() - FLAG_SECURE disabled") | ||
| } | ||
| else -> { | ||
| android.util.Log.w(TAG, "setScreenSecurityLevel() - unknown level: $level") | ||
| } | ||
| } |
There was a problem hiding this comment.
The when statement for handling screen security levels silently logs unknown levels without any error handling. Consider throwing an exception or setting a default security level to ensure the application maintains a known security state.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- f821df9: feat: onboarding ui
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt (3 hunks)
Actionable Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [798-802]
security: "Unimplemented critical security feature marked as TODO"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [867-872]
security: "Error handling could expose sensitive data"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [847-847]
possible issue: "Potential username collision risk"
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 0e4cb3f: feat: onboarding ui entrypoint
Files Processed (3)
- app/src/main/java/com/flowfoundation/wallet/page/profile/subpage/wallet/WalletListActivity.kt (2 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/wallet/fragment/WalletUnregisteredFragment.kt (2 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/wallet/presenter/WalletHeaderPresenter.kt (1 hunk)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/wallet/fragment/WalletUnregisteredFragment.kt [47-48]
maintainability: "Inconsistent onboarding launch implementation"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/profile/subpage/wallet/WalletListActivity.kt [174-178]
best practice: "Intent flags should be set after all extras"
app/src/main/java/com/flowfoundation/wallet/page/wallet/fragment/WalletUnregisteredFragment.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 991dea6: feat: onboarding ui entrypoint
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt (3 hunks)
Actionable Comments (3)
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [772-772]
security: "Missing validation for recovery phrase word count"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [847-847]
security: "Weak username generation mechanism"
-
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt [1015-1036]
security: "Improper handling of unknown screen security levels"
Skipped Comments (0)
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
| currentActivity.runOnUiThread { | ||
| when (level) { | ||
| "secure" -> { | ||
| // Prevent screenshots and screen recording | ||
| currentActivity.window.setFlags( | ||
| android.view.WindowManager.LayoutParams.FLAG_SECURE, | ||
| android.view.WindowManager.LayoutParams.FLAG_SECURE | ||
| ) | ||
| android.util.Log.d(TAG, "setScreenSecurityLevel() - FLAG_SECURE enabled") | ||
| } | ||
| "normal" -> { | ||
| // Allow screenshots again | ||
| currentActivity.window.clearFlags( | ||
| android.view.WindowManager.LayoutParams.FLAG_SECURE | ||
| ) | ||
| android.util.Log.d(TAG, "setScreenSecurityLevel() - FLAG_SECURE disabled") | ||
| } | ||
| else -> { | ||
| android.util.Log.w(TAG, "setScreenSecurityLevel() - unknown level: $level") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The screen security level handling should throw an exception for unknown levels rather than just logging a warning. This ensures the app maintains a known security state:
else -> {
throw IllegalArgumentException("Unknown security level: $level")
}- Saves mnemonic to secure storage using Wallet.store() - Creates EOA Account in AccountManager (no Firebase UID) - Initializes WalletManager with new account - Returns success/error response
- Remove generateRecoveryPhrase() (using TypeScript implementation) - Remove createEOAAccount() stub (replaced by saveMnemonic) - Clean up unused code per code review feedback
- Resolved import conflicts in NativeFRWBridge.kt - Updated build.gradle to read GitHub credentials from local.properties
lealobanov
changed the title
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/NativeFRWBridge.kt
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| override fun createCOAAccount(promise: Promise) { |
There was a problem hiding this comment.
Try to split this bridge into multiple file, it's over 1400 lines+, not good
There was a problem hiding this comment.
@lmcmz I have moved these out into separate handlers to break up the bridge functionality across different files
app/src/main/java/com/flowfoundation/wallet/reactnative/bridge/BridgeModels.kt
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/reactnative/ReactNativeActivity.kt
Outdated
Show resolved
Hide resolved
Secure Enclave/COA accounts should NOT have a mnemonic or seed phrase. They use hardware-backed keys only, distinct from EOA accounts. Changes: - Removed BIP39 mnemonic generation from registerServer() - Removed global mnemonic storage (storeWalletPassword) - Added comments clarifying this is for Secure Type/COA accounts only - EOA accounts with seed phrase use the RN flow with saveMnemonic() This fixes the issue where Secure Enclave profile users were seeing EOA addresses in their account. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Secure Enclave accounts created with hardware-backed keys should NOT display an EOA chip in the sidebar. They only have: - Flow Cadence address (main account) - EVM/COA address (with purple EVM chip) Changes: - Removed getEOAAddressCached() check from setupLinkedAccountForHardwareBackedKey() - Added comment explaining that COA accounts don't have EOA addresses - EOA addresses are only for seed phrase-based accounts Fixes issue where Secure Enclave profile showed incorrect EOA chip in Android sidebar. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Recovery Phrase flow now creates a linked COA child account instead of a separate main account. Changes: - Added createLinkedCOAAccount() bridge method - Calls cadenceCreateCOAAccount() to create linked COA via Cadence transaction - Returns transaction ID for tracking This fixes the architecture where Recovery Phrase accounts should have: 1. Main Flow account (created via saveMnemonic) 2. EOA address (derived from mnemonic) 3. Linked COA child account (created via Cadence transaction) Secure Enclave accounts remain as main COA accounts (no linked child). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
app/src/main/java/com/flowfoundation/wallet/manager/account/AccountManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/manager/emoji/model/WalletEmojiInfo.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/page/dialog/accounts/AccountSwitchDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/page/main/presenter/MainContentPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/page/main/presenter/MainContentPresenter.kt
Show resolved
Hide resolved
app/src/main/java/com/flowfoundation/wallet/page/profile/subpage/wallet/WalletListActivity.kt
Outdated
Show resolved
Hide resolved
…-Android into onboarding-entrypoint
Related Issue
Closes onflow/FRW-monorepo#373
Summary of Changes
Need Regression Testing
Risk Assessment
Additional Notes
Screenshots (if applicable)