fix: rename devnet and reduce scope for Google Drive #1418
Conversation
WalkthroughThis change updates the build configuration and codebase to replace references to the "schnapps" devnet flavor with "devnet," adjusts resource paths and application IDs, and introduces new build config fields for the devnet flavor. It also refactors Google Drive authorization logic by delegating authorization request construction to the ViewModel and service layers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditProfileActivity
participant EditProfileViewModel
participant GoogleDriveService
participant Google Identity API
User->>EditProfileActivity: Initiates Google Drive authorization
EditProfileActivity->>EditProfileViewModel: getGoogleDriveAuthRequest()
EditProfileViewModel->>GoogleDriveService: getAuthRequest()
GoogleDriveService-->>EditProfileViewModel: AuthorizationRequest
EditProfileViewModel-->>EditProfileActivity: AuthorizationRequest
EditProfileActivity->>Google Identity API: Begin authorization with AuthorizationRequest
Google Identity API-->>EditProfileActivity: Result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
||
| fun getAuthRequest() = AuthorizationRequest | ||
| .builder() | ||
| .setRequestedScopes( | ||
| listOf(Scope(DriveScopes.DRIVE_FILE)) | ||
| ).build() |
There was a problem hiding this comment.
Scopes reduced from DRIVE (all functions) to DRIVE_FILES (only files created by this app)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wallet/.gitignore (1)
7-7: Consider keeping the oldschnapps/releaseignore while artifacts still existIf any CI/CD or local developer machines still produce
schnapps/releaseoutput, its removal from.gitignorecould accidentally stage large binary artefacts until every reference is fully purged.
Keep the pattern for one release cycle or verify that directory is no longer generated anywhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
wallet/devnet/res/drawable-xhdpi/currency_symbol_dash.pngis excluded by!**/*.pngwallet/devnet/res/drawable-xhdpi/currency_symbol_mdash.pngis excluded by!**/*.pngwallet/devnet/res/drawable-xhdpi/currency_symbol_udash.pngis excluded by!**/*.pngwallet/devnet/res/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngwallet/devnet/res/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngwallet/devnet/res/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngwallet/devnet/res/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngwallet/devnet/res/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.png
📒 Files selected for processing (7)
wallet/.gitignore(1 hunks)wallet/build.gradle(3 hunks)wallet/src/de/schildbach/wallet/Constants.java(0 hunks)wallet/src/de/schildbach/wallet/payments/RequestWalletBalanceTask.java(1 hunks)wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/EditProfileViewModel.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/utils/GoogleDriveService.kt(2 hunks)
💤 Files with no reviewable changes (1)
- wallet/src/de/schildbach/wallet/Constants.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
📚 Learning: the explore feature in the dash-wallet application uses different naming conventions for different t...
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1370
File: fastlane/Fastfile:176-176
Timestamp: 2025-04-04T07:11:07.664Z
Learning: The explore feature in the dash-wallet application uses different naming conventions for different types of databases: preloaded and test databases still use the original naming convention (e.g., "explore.db"), while production databases use the v2 naming scheme (e.g., "explore-v2.db"). This inconsistency is intentional.
Applied to files:
wallet/src/de/schildbach/wallet/payments/RequestWalletBalanceTask.java
📚 Learning: in the dash-wallet project, database migrations must account for pre-populated databases. when new c...
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/model/Merchant.kt:52-52
Timestamp: 2025-05-07T14:19:08.146Z
Learning: In the dash-wallet project, database migrations must account for pre-populated databases. When new columns are added to model classes, empty migrations with incremented version numbers are used instead of ALTER TABLE statements, as the downloaded database will already contain the new columns. This prevents SQL errors when trying to add columns that already exist.
Applied to files:
wallet/build.gradle
📚 Learning: in the dash wallet app, dashpayconfig methods like istransactionmetadatainfoshown() already use with...
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Applied to files:
wallet/src/de/schildbach/wallet/ui/dashpay/utils/GoogleDriveService.ktwallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt
📚 Learning: in the dashpay/dash-wallet project, explicitly unregistering viewpager2 callbacks is not considered ...
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: wallet/src/de/schildbach/wallet/ui/payments/PaymentsFragment.kt:111-119
Timestamp: 2025-05-12T09:14:36.656Z
Learning: In the dashpay/dash-wallet project, explicitly unregistering ViewPager2 callbacks is not considered critical since memory leaks haven't been observed in practice with the memory profiler.
Applied to files:
wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt
📚 Learning: in dash wallet, datastore operations (like those in dashpayconfig through baseconfig) already handle...
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Applied to files:
wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
wallet/src/de/schildbach/wallet/ui/dashpay/utils/GoogleDriveService.kt (1)
98-102: LGTM! Clean centralization of authorization request creation.The new
getAuthRequest()method properly encapsulates the creation of the Google Drive authorization request with the appropriateDRIVE_FILEscope. This refactoring improves separation of concerns by moving authorization logic into the service layer.wallet/src/de/schildbach/wallet/ui/dashpay/EditProfileViewModel.kt (1)
289-291: LGTM! Proper delegation to service layer.The
getGoogleDriveAuthRequest()method correctly delegates authorization request creation to theGoogleDriveService, maintaining clean separation between the ViewModel and service layers.wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt (1)
340-340: LGTM! Improved architecture with proper delegation.The refactoring successfully moves authorization request creation from the Activity to the ViewModel/Service layers. This improves separation of concerns and makes the authorization logic more testable and reusable.
wallet/build.gradle (2)
296-297:res.srcDirspath looks goodRenaming the resource directory to
devnet/resaligns with the flavour rename and keeps resource separation intact.
No issues spotted here.
366-366: Double-check downstream references to the newapplicationIdChanging the
applicationIdtoorg.dash.dashpay.devnetmeans:
- Any manifest-placeholders, test runners, Firebase config files, and Play-Store internal-track packages referencing the old ID must be updated.
- Push-notification sender IDs and OAuth redirect URIs may also need updates.
Please verify those external touch-points.
wallet/src/de/schildbach/wallet/payments/RequestWalletBalanceTask.java (1)
343-345: Plain-HTTP endpoint – verify necessityThe dev-net block-explorer URL uses plain
http://. Even in non-production networks this invites MITM tampering and unreliable results.
- Confirm the Insight instance genuinely lacks TLS.
- If TLS is available, switch to
https://.- Otherwise, consider pinning the certificate or at least logging a warning about the insecure transport.
No code change proposed until the endpoint capabilities are confirmed.
| def topperKeyId = props.getProperty("TOPPER_KEY_ID_SANDBOX", "\"TOPPER_KEY_ID\"") | ||
| def topperWidgetId = props.getProperty("TOPPER_WIDGET_ID_SANDBOX", "\"TOPPER_WIDGET_ID\"") | ||
| def topperPrivateKey = props.getProperty("TOPPER_PRIVATE_KEY_SANDBOX", "\"TOPPER_PRIVATE_KEY\"") | ||
| buildConfigField("String", "TOPPER_KEY_ID", topperKeyId) | ||
| buildConfigField("String", "TOPPER_WIDGET_ID", topperWidgetId) | ||
| buildConfigField("String", "TOPPER_PRIVATE_KEY", topperPrivateKey) | ||
| buildConfigField("String", "ZENLEDGER_CLIENT_ID", "\"\"") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Secrets exposure & property fallback consistency
Great to see sandbox-specific TOPPER_* fields added. Two minor points:
-
These keys end up in the generated
BuildConfig.java. Even for devnet, avoid putting real secrets in plain text; double-check thatservice.propertiesis git-ignored and that CI injects them via secure variables. -
Immediately above (Lines 367-376) the flavour fetches sandbox Uphold credentials into
imgurClientId/Secret, but then discards them and hard-codes"UPHOLD_CLIENT_ID"/"UPHOLD_CLIENT_SECRET". That looks like a stale copy-paste.
If you actually want the sandbox values, wire them intobuildConfigFieldsimilarly to what you just did for theTOPPER_*keys.
- buildConfigField("String", "UPHOLD_CLIENT_ID", "\"UPHOLD_CLIENT_ID\"")
- buildConfigField("String", "UPHOLD_CLIENT_SECRET", "\"UPHOLD_CLIENT_SECRET\"")
+ buildConfigField("String", "UPHOLD_CLIENT_ID", imgurClientId)
+ buildConfigField("String", "UPHOLD_CLIENT_SECRET", imgurClientSecret)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In wallet/build.gradle around lines 377 to 383, avoid exposing real secrets in
BuildConfig by ensuring service.properties is git-ignored and secrets are
injected securely in CI. Also, fix the stale hard-coded Uphold sandbox
credentials above these lines by replacing the fixed strings with properties
fetched from service.properties, similar to how TOPPER_* sandbox keys are
handled, to maintain consistency and proper secret management.
There was a problem hiding this comment.
What is the alternative? How can I avoid plain text secrets in BuildConfig? @coderabbitai
There was a problem hiding this comment.
@HashEngineering Great question! Here are several alternatives to avoid plain text secrets in BuildConfig:
1. Encrypted SharedPreferences + Runtime Injection
Store secrets encrypted and decrypt at runtime:
// Instead of BuildConfig.TOPPER_KEY_ID
class SecretManager @Inject constructor(
@ApplicationContext private val context: Context
) {
fun getTopperKeyId(): String {
return EncryptedSharedPreferences.create(...)
.getString("topper_key_id", "") ?: ""
}
}2. Android Keystore + Server Injection
Most secure approach - fetch secrets from your backend at runtime:
// App requests secrets from your server using device/user authentication
class ApiKeyRepository {
suspend fun getTopperCredentials(): TopperCredentials {
return api.getCredentials(deviceId, userToken)
}
}3. ProGuard Obfuscation (Minimal Protection)
Better than nothing, but still reversible:
buildTypes {
release {
minifyEnabled true
// Obfuscates field names but values still visible
}
}4. Native Library Storage
Store in C++ with some obfuscation:
// secrets.cpp
extern "C" JNIEXPORT jstring JNICALL
Java_com_yourapp_SecretManager_getTopperKey(JNIEnv *env, jobject) {
// Still not 100% secure but harder to extract
return env->NewStringUTF(decrypt("encrypted_key_here"));
}5. Hybrid Approach (Recommended)
- Development/Staging: Use
BuildConfigfor convenience - Production: Runtime injection from secure server
- Sandbox keys: Can stay in
BuildConfig(less sensitive)
class ConfigManager {
fun getTopperKeyId(): String {
return if (BuildConfig.DEBUG) {
BuildConfig.TOPPER_KEY_ID // Dev convenience
} else {
secureStorage.getTopperKey() // Production security
}
}
}Reality check: Client-side apps inherently expose some data. Focus on:
- Keep production secrets server-side when possible
- Use sandbox/test keys in builds
- Implement key rotation
- Monitor for API key abuse
For your devnet flavor, using sandbox keys in BuildConfig is probably acceptable since they're not production credentials.
🧠 Learnings used
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
There was a problem hiding this comment.
Thank you. This will be considered in a future story.
There was a problem hiding this comment.
@HashEngineering You're welcome! That makes perfect sense - keeping this PR focused on the devnet renaming and Google Drive scope changes is the right approach. The security improvements can definitely be addressed in a dedicated story where they can get proper attention and testing.
Thanks for considering the suggestions for future implementation!
🧠 Learnings used
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
|
This has been reviewed by a team member. |
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Improvements
Chores