Conversation
📝 WalkthroughWalkthroughMoves AppsFlyer initialization into WalletApplication with Play Services checks and deep-link extraction; adds UUID persistence; introduces WalletActivityTracker; adjusts manifests, BuildConfig and ProGuard rules; hardens invite/top-up validation; adds lint suppressions and assorted UI, API, and minor refactor changes. Changes
Sequence Diagram(s)sequenceDiagram
participant WalletApp as WalletApplication
participant GPS as GooglePlayServices
participant AppsFlyer as AppsFlyerLib
participant MainThread as MainLooper
participant Invite as InviteHandlerActivity
WalletApp->>GPS: checkAvailability()
alt Play Services available
WalletApp->>AppsFlyer: initialize(with template id, setCustomerId)
AppsFlyer-->>WalletApp: registerConversionListener()
AppsFlyer->>WalletApp: onConversionData(data)
WalletApp->>MainThread: post { extract deep-link (af_dp, deep_link_value, link, af_sub1) }
MainThread->>Invite: startActivity(with extracted deep-link)
else Play Services unavailable
WalletApp-->>AppsFlyer: skip initialization
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wallet/build.gradle (1)
389-409:⚠️ Potential issue | 🟡 Minor
devnetflavor inherits mainnetAPPSFLYER_TEMPLATE_ID— likely should use testnet variant.The
allblock setsAPPSFLYER_TEMPLATE_IDfrom the mainnet property. Both_testNet3andstagingoverride it withAPPSFLYER_TEMPLATE_ID_TESTNET, butdevnetdoes not. Since devnet is a development/test network, it should probably also use the testnet template ID.Suggested fix
devnet { applicationId = "org.dash.dashpay.devnet" ... buildConfigField("String", "ZENLEDGER_CLIENT_ID", "\"\"") buildConfigField("String", "ZENLEDGER_CLIENT_SECRET", "\"\"") + + def appsFlyerTemplateId = props.getProperty("APPSFLYER_TEMPLATE_ID_TESTNET", "\"\"") + buildConfigField("String", "APPSFLYER_TEMPLATE_ID", appsFlyerTemplateId) }
🤖 Fix all issues with AI agents
In `@wallet/src/de/schildbach/wallet/WalletActivityTracker.java`:
- Around line 119-123: logState() can NPE when currentActivity is null; add a
null guard before accessing currentActivity.getClass().getSimpleName() (e.g.,
compute a safe activityName variable or conditional expression) so the log call
always receives a non-null string; update the logState() method to use that safe
name and ensure callers like onActivityDestroyed that clear currentActivity
still log safely (refer to logState(), currentActivity, onActivityDestroyed, and
logName).
- Line 39: currentActivity is a strong Application-scoped reference causing a
leak; update WalletActivityTracker so onActivityDestroyed clears that reference.
Specifically, in the WalletActivityTracker class implement or modify
onActivityDestroyed(Activity activity) to check if activity == currentActivity
and if so set currentActivity to null (and ensure any related listeners or
callbacks tied to currentActivity are also removed/cleared there).
In `@wallet/src/de/schildbach/wallet/WalletApplication.java`:
- Around line 337-366: Remove production logging of full deep-link URLs (which
may contain PARAM_PRIVATE_KEY/WIF) in handleDeepLinkData and extractDeepLink:
stop calling log.info with the raw deepLinkValue/stringValue; instead either (a)
only log that a deep link was found and its source key (e.g., in extractDeepLink
log the key name but not the URL), or (b) conditionally log a redacted value
only when BuildConfig.DEBUG is true. Update handleDeepLinkData to avoid logging
deepLinkValue (use a presence/intent-launched message or debug-only masked
logging) and update extractDeepLink to log only the key name or a masked
representation (no full URL or query params).
- Around line 297-303: Remove the redundant call that sets a random UUID before
the real persistent ID: in the AppsFlyer initialization block (AppsFlyerLib
appsFlyerLib = AppsFlyerLib.getInstance(); appsFlyerLib.init(...); ...),
eliminate the call appsFlyerLib.setCustomerUserId(UUID.randomUUID().toString())
so only appsFlyerLib.setCustomerUserId(customerId) (where customerId =
config.getUniqueId()) is used; if a fallback is desired instead, wrap the
UUID.randomUUID() assignment to be used only when config.getUniqueId() is
null/empty and then call setCustomerUserId once with the chosen value.
- Line 300: The AppsFlyer debug logging is unconditionally enabled via
appsFlyerLib.setDebugLog(true); change it to enable debug logging only for
non-production builds by gating the call with BuildConfig.DEBUG (or equivalent
debug flag) so setDebugLog(true) runs only when BuildConfig.DEBUG is true and
setDebugLog(false) or omit the call in release; update the code near the
appsFlyerLib.setDebugLog invocation in WalletApplication.java to check
BuildConfig.DEBUG before enabling debug logging.
- Around line 247-253: The background thread running initializeAppsFlyer can
access config before it's assigned, causing an NPE; move the Configuration
initialization (config = new Configuration(...)) and any dependent setup
(autoLogout = new AutoLogout(config);
autoLogout.registerDeviceInteractiveReceiver(this);
registerActivityLifecycleCallbacks(...)) so that config is fully initialized on
the main thread before starting new Thread(this::initializeAppsFlyer).
Alternatively, capture a final local reference to the newly created
Configuration and pass it into the thread/initializeAppsFlyer call (or otherwise
synchronize access) so initializeAppsFlyer and its use of config.getUniqueId()
cannot run before config is set.
🧹 Nitpick comments (9)
wallet/src/de/schildbach/wallet/Constants.java (1)
99-99:SUPPORTS_INVITESis nowtruein every flavor — consider removing the flag.With this PR, all three flavor branches (
prod,staging/_testNet3,devnet) setSUPPORTS_INVITES = true. The flag is effectively always-on dead code. The existing TODO on line 65 already calls this out. Since the flag is non-final, it can't be eliminated by the compiler, so every call-site still evaluates the branch at runtime.Not blocking, but worth tracking cleanup as a fast-follow.
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (1)
828-834: Swallowed exception loses context for debugging.When the
ISDLOCK_VERSIONparse fails, the exception is silently discarded before retrying withISLOCK_VERSION. If both fail, only the second exception propagates, making it harder to diagnose the root cause. Consider logging the first failure at debug level.Proposed fix
} catch (e: Exception) { + log.debug("ISDLOCK_VERSION parse failed, trying ISLOCK_VERSION", e) InstantSendLock(wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityNotification.kt (1)
58-86: Consider using the int-basedstopForegroundAPI instead of suppressing the lint warning.The
@SuppressLint("WrongConstant")is masking the use of the deprecatedstopForeground(boolean)overload on line 85. Since the project's min SDK is 24, you can use the non-deprecated int-based API directly, which also eliminates the need for the suppression:♻️ Suggested fix
- `@SuppressLint`("WrongConstant") fun displayErrorAndStopService(isInvite: Boolean) { ... - service.stopForeground(true) + service.stopForeground(Service.STOP_FOREGROUND_REMOVE)wallet/src/de/schildbach/wallet/WalletBalanceWidgetProvider.java (1)
114-114: Consider identifying and fixing the root cause of the "WrongConstant" lint warning instead of suppressing it.A blanket
@SuppressLint("WrongConstant")on this entire method hides the specific issue. If possible, identify which constant is flagged (likelygetResources().getColor()without a theme on line 160, or a similar API misuse) and fix it directly. If the suppression is intentional, adding a brief comment explaining why would help future maintainers.wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (1)
458-458: Six identical@SuppressLint("WrongConstant")annotations scattered across this file — consider a targeted class-level or file-level suppression with a comment explaining the root cause.If these are all triggered by the same underlying lint issue (e.g.,
getSystemService()string constants orstopForeground(true)), a single@SuppressLintat the class level with an explanatory comment would be cleaner than decorating individual methods. Alternatively, fixing the root causes (e.g., usingContextCompat.getSystemService()orstopForeground(STOP_FOREGROUND_REMOVE)) would eliminate the need for suppressions entirely.Also applies to: 623-623, 694-694, 1345-1345, 1792-1792, 2050-2050
features/exploredash/proguard-rules.pro (1)
23-25: Theandroidx.paging.**keep rule is quite broad.
-keep class androidx.paging.** { *; }prevents all shrinking/obfuscation of the entire paging library. Typically only theDataSourcesubclass rule (line 25) is needed. The broad keep on line 24 may unnecessarily increase APK size. Consider whether it's actually needed or was added preemptively.wallet/src/de/schildbach/wallet/service/BootstrapReceiver.java (1)
61-63: Class-level@SuppressLint("WrongConstant")is broad — consider scoping to the specific method.Applying the suppression at the class level masks all
WrongConstantwarnings in the entire class. If only one method triggers the lint warning, applying it at that method level preserves lint coverage for the rest of the class.common/src/main/java/org/dash/wallet/common/Configuration.java (1)
507-515: UnnecessaryUUID.randomUUID()asgetStringdefault — use a simple sentinel instead.On line 509, since
prefs.contains(PREFS_UUID)is already checked, the default value ofgetStringshould never be reached. However, usingUUID.randomUUID().toString()as a default is misleading and wasteful — a new UUID object is allocated on every call even when the key exists. Worse, if an edge case causes the default to be returned (e.g., corrupted prefs returning null for a present key), the UUID won't be persisted, leading to a different ID on each call.Simpler and safer approach
public String getUniqueId() { - if (prefs.contains(PREFS_UUID)) { - return prefs.getString(PREFS_UUID, UUID.randomUUID().toString()); - } else { + String uuid = prefs.getString(PREFS_UUID, null); + if (uuid != null) { + return uuid; + } else { - String uuid = UUID.randomUUID().toString(); + uuid = UUID.randomUUID().toString(); prefs.edit().putString(PREFS_UUID, uuid).apply(); return uuid; } }wallet/build.gradle (1)
341-343:prodflavorAPPSFLYER_TEMPLATE_IDoverride is redundant.The
allblock on line 318-320 already reads from the sameAPPSFLYER_TEMPLATE_IDproperty. This override reads the identical property and sets the identical BuildConfig field. It can be removed unless you plan to change the property key in the future.
| private static final Logger log = LoggerFactory.getLogger(WalletActivityTracker.class); | ||
|
|
||
| private final WalletApplication app; | ||
| private Activity currentActivity; |
There was a problem hiding this comment.
currentActivity holds a strong reference — memory leak risk.
currentActivity is an Application-scoped field that is never cleared. After the last Activity is destroyed (onActivityDestroyed), it still holds a reference to that Activity, preventing it from being garbage collected for the lifetime of the process. This can retain the entire view hierarchy and associated resources.
Proposed fix: clear the reference in onActivityDestroyed
public void onActivityDestroyed(`@NonNull` Activity activity) {
log.info("{}: activity {} destroyed", logName, activity.getClass().getSimpleName());
activityCount--;
- logState();
+ if (currentActivity == activity) {
+ currentActivity = null;
+ }
+ if (currentActivity != null) {
+ logState();
+ }
if (activityCount == 0)
log.info("{}: app closed", logName);
}🤖 Prompt for AI Agents
In `@wallet/src/de/schildbach/wallet/WalletActivityTracker.java` at line 39,
currentActivity is a strong Application-scoped reference causing a leak; update
WalletActivityTracker so onActivityDestroyed clears that reference.
Specifically, in the WalletActivityTracker class implement or modify
onActivityDestroyed(Activity activity) to check if activity == currentActivity
and if so set currentActivity to null (and ensure any related listeners or
callbacks tied to currentActivity are also removed/cleared there).
| private void logState() { | ||
| log.info("{}: current: {}, activities: {} visible: {} foreground: {}", logName, | ||
| currentActivity.getClass().getSimpleName(), activityCount, | ||
| visibleActivityCount, foregroundActivityCount); | ||
| } |
There was a problem hiding this comment.
logState() will NPE if currentActivity is null.
currentActivity is initialized to null and is never null-checked in logState(). While the normal Activity lifecycle should prevent this, clearing the reference in onActivityDestroyed (recommended above) would trigger this. Add a null guard.
Proposed fix
private void logState() {
+ String activityName = currentActivity != null
+ ? currentActivity.getClass().getSimpleName()
+ : "none";
- log.info("{}: current: {}, activities: {} visible: {} foreground: {}", logName,
- currentActivity.getClass().getSimpleName(), activityCount,
+ log.info("{}: current: {}, activities: {} visible: {} foreground: {}", logName,
+ activityName, activityCount,
visibleActivityCount, foregroundActivityCount);
}📝 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 void logState() { | |
| log.info("{}: current: {}, activities: {} visible: {} foreground: {}", logName, | |
| currentActivity.getClass().getSimpleName(), activityCount, | |
| visibleActivityCount, foregroundActivityCount); | |
| } | |
| private void logState() { | |
| String activityName = currentActivity != null | |
| ? currentActivity.getClass().getSimpleName() | |
| : "none"; | |
| log.info("{}: current: {}, activities: {} visible: {} foreground: {}", logName, | |
| activityName, activityCount, | |
| visibleActivityCount, foregroundActivityCount); | |
| } |
🤖 Prompt for AI Agents
In `@wallet/src/de/schildbach/wallet/WalletActivityTracker.java` around lines 119
- 123, logState() can NPE when currentActivity is null; add a null guard before
accessing currentActivity.getClass().getSimpleName() (e.g., compute a safe
activityName variable or conditional expression) so the log call always receives
a non-null string; update the logState() method to use that safe name and ensure
callers like onActivityDestroyed that clear currentActivity still log safely
(refer to logState(), currentActivity, onActivityDestroyed, and logName).
| new Thread(this::initializeAppsFlyer).start(); | ||
| AppCompatDelegate.setCompatVectorFromResourcesEnabled(true); | ||
| log.info("WalletApplication.onCreate()"); | ||
| config = new Configuration(PreferenceManager.getDefaultSharedPreferences(this)); | ||
| autoLogout = new AutoLogout(config); | ||
| autoLogout.registerDeviceInteractiveReceiver(this); | ||
| registerActivityLifecycleCallbacks(new ActivitiesTracker() { | ||
| int activityCount = 0; | ||
| int foregroundActivityCount = 0; | ||
| int visibleActivityCount = 0; | ||
| final String logName = "activity lifecycle"; | ||
| @Override | ||
| protected void onStartedFirst(Activity activity) { | ||
|
|
||
| } | ||
|
|
||
| @Override | ||
| protected void onStartedAny(boolean isTheFirstOne, Activity activity) { | ||
| super.onStartedAny(isTheFirstOne, activity); | ||
| // force restart if the app was updated | ||
| // this ensures that v6.x or previous will go through the PIN upgrade process | ||
| if (!BuildConfig.DEBUG && myPackageReplaced) { | ||
| log.info("restarting app due to upgrade"); | ||
| myPackageReplaced = false; | ||
| restartService.performRestart(activity, true, true); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| protected void onStoppedLast() { | ||
| autoLogout.setAppWentBackground(true); | ||
| if (config.getAutoLogoutEnabled() && config.getAutoLogoutMinutes() == 0) { | ||
| sendBroadcast(new Intent(InteractionAwareActivity.FORCE_FINISH_ACTION)); | ||
| } | ||
| } | ||
|
|
||
| public void onActivityCreated(@NonNull Activity activity, Bundle bundle) { | ||
| if (activityCount == 0) | ||
| log.info("{}: app started", logName); | ||
| activityCount++; | ||
| log.info("{}: activity {} created", logName, activity.getClass().getSimpleName()); | ||
| logState(); | ||
| currentActivity = activity; | ||
| } | ||
|
|
||
| public void onActivityDestroyed(@NonNull Activity activity) { | ||
| log.info("{}: activity {} destroyed", logName, activity.getClass().getSimpleName()); | ||
| activityCount--; | ||
| logState(); | ||
| if (activityCount == 0) | ||
| log.info("{}: app closed", logName); | ||
| } | ||
|
|
||
| public void onActivityResumed(@NonNull Activity activity) { | ||
| foregroundActivityCount++; | ||
| logState(); | ||
| currentActivity = activity; | ||
| } | ||
|
|
||
| public void onActivityPaused(@NonNull Activity activity) { | ||
| foregroundActivityCount--; | ||
| logState(); | ||
| } | ||
|
|
||
| public void onActivityStarted(@NonNull Activity activity) { | ||
| visibleActivityCount++; | ||
| logState(); | ||
| currentActivity = activity; | ||
| } | ||
|
|
||
| public void onActivityStopped(Activity activity) { | ||
| visibleActivityCount--; | ||
| logState(); | ||
| } | ||
|
|
||
| private void logState() { | ||
| log.info("{}: activities: {} visible: {} foreground: {}", logName, | ||
| activityCount, visibleActivityCount, foregroundActivityCount); | ||
| } | ||
| }); | ||
| registerActivityLifecycleCallbacks(new WalletActivityTracker(this, config, autoLogout, restartService)); |
There was a problem hiding this comment.
Race condition: config may be null when initializeAppsFlyer runs.
initializeAppsFlyer is launched on a background thread at line 247, but config is initialized at line 250 on the main thread. The background thread can reach config.getUniqueId() (line 302) before config is assigned, causing a NullPointerException.
Proposed fix: initialize config before starting the thread
initLogging();
FirebaseApp.initializeApp(this);
- new Thread(this::initializeAppsFlyer).start();
AppCompatDelegate.setCompatVectorFromResourcesEnabled(true);
log.info("WalletApplication.onCreate()");
config = new Configuration(PreferenceManager.getDefaultSharedPreferences(this));
+ new Thread(this::initializeAppsFlyer).start();
autoLogout = new AutoLogout(config);📝 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.
| new Thread(this::initializeAppsFlyer).start(); | |
| AppCompatDelegate.setCompatVectorFromResourcesEnabled(true); | |
| log.info("WalletApplication.onCreate()"); | |
| config = new Configuration(PreferenceManager.getDefaultSharedPreferences(this)); | |
| autoLogout = new AutoLogout(config); | |
| autoLogout.registerDeviceInteractiveReceiver(this); | |
| registerActivityLifecycleCallbacks(new ActivitiesTracker() { | |
| int activityCount = 0; | |
| int foregroundActivityCount = 0; | |
| int visibleActivityCount = 0; | |
| final String logName = "activity lifecycle"; | |
| @Override | |
| protected void onStartedFirst(Activity activity) { | |
| } | |
| @Override | |
| protected void onStartedAny(boolean isTheFirstOne, Activity activity) { | |
| super.onStartedAny(isTheFirstOne, activity); | |
| // force restart if the app was updated | |
| // this ensures that v6.x or previous will go through the PIN upgrade process | |
| if (!BuildConfig.DEBUG && myPackageReplaced) { | |
| log.info("restarting app due to upgrade"); | |
| myPackageReplaced = false; | |
| restartService.performRestart(activity, true, true); | |
| } | |
| } | |
| @Override | |
| protected void onStoppedLast() { | |
| autoLogout.setAppWentBackground(true); | |
| if (config.getAutoLogoutEnabled() && config.getAutoLogoutMinutes() == 0) { | |
| sendBroadcast(new Intent(InteractionAwareActivity.FORCE_FINISH_ACTION)); | |
| } | |
| } | |
| public void onActivityCreated(@NonNull Activity activity, Bundle bundle) { | |
| if (activityCount == 0) | |
| log.info("{}: app started", logName); | |
| activityCount++; | |
| log.info("{}: activity {} created", logName, activity.getClass().getSimpleName()); | |
| logState(); | |
| currentActivity = activity; | |
| } | |
| public void onActivityDestroyed(@NonNull Activity activity) { | |
| log.info("{}: activity {} destroyed", logName, activity.getClass().getSimpleName()); | |
| activityCount--; | |
| logState(); | |
| if (activityCount == 0) | |
| log.info("{}: app closed", logName); | |
| } | |
| public void onActivityResumed(@NonNull Activity activity) { | |
| foregroundActivityCount++; | |
| logState(); | |
| currentActivity = activity; | |
| } | |
| public void onActivityPaused(@NonNull Activity activity) { | |
| foregroundActivityCount--; | |
| logState(); | |
| } | |
| public void onActivityStarted(@NonNull Activity activity) { | |
| visibleActivityCount++; | |
| logState(); | |
| currentActivity = activity; | |
| } | |
| public void onActivityStopped(Activity activity) { | |
| visibleActivityCount--; | |
| logState(); | |
| } | |
| private void logState() { | |
| log.info("{}: activities: {} visible: {} foreground: {}", logName, | |
| activityCount, visibleActivityCount, foregroundActivityCount); | |
| } | |
| }); | |
| registerActivityLifecycleCallbacks(new WalletActivityTracker(this, config, autoLogout, restartService)); | |
| AppCompatDelegate.setCompatVectorFromResourcesEnabled(true); | |
| log.info("WalletApplication.onCreate()"); | |
| config = new Configuration(PreferenceManager.getDefaultSharedPreferences(this)); | |
| new Thread(this::initializeAppsFlyer).start(); | |
| autoLogout = new AutoLogout(config); | |
| autoLogout.registerDeviceInteractiveReceiver(this); | |
| registerActivityLifecycleCallbacks(new WalletActivityTracker(this, config, autoLogout, restartService)); |
🤖 Prompt for AI Agents
In `@wallet/src/de/schildbach/wallet/WalletApplication.java` around lines 247 -
253, The background thread running initializeAppsFlyer can access config before
it's assigned, causing an NPE; move the Configuration initialization (config =
new Configuration(...)) and any dependent setup (autoLogout = new
AutoLogout(config); autoLogout.registerDeviceInteractiveReceiver(this);
registerActivityLifecycleCallbacks(...)) so that config is fully initialized on
the main thread before starting new Thread(this::initializeAppsFlyer).
Alternatively, capture a final local reference to the newly created
Configuration and pass it into the thread/initializeAppsFlyer call (or otherwise
synchronize access) so initializeAppsFlyer and its use of config.getUniqueId()
cannot run before config is set.
| AppsFlyerLib appsFlyerLib = AppsFlyerLib.getInstance(); | ||
| appsFlyerLib.init(BuildConfig.APPSFLYER_ID, null, this); | ||
| appsFlyerLib.setAppInviteOneLink(BuildConfig.APPSFLYER_TEMPLATE_ID); | ||
| appsFlyerLib.setDebugLog(true); | ||
| appsFlyerLib.setCustomerUserId(UUID.randomUUID().toString()); | ||
| String customerId = config.getUniqueId(); | ||
| appsFlyerLib.setCustomerUserId(customerId); |
There was a problem hiding this comment.
Redundant setCustomerUserId call — first value is immediately overwritten.
Line 301 sets a random UUID as customer ID, then lines 302-303 immediately overwrite it with the persistent UUID from config. The first call is dead code.
Proposed fix
appsFlyerLib.setDebugLog(true);
- appsFlyerLib.setCustomerUserId(UUID.randomUUID().toString());
String customerId = config.getUniqueId();
appsFlyerLib.setCustomerUserId(customerId);📝 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.
| AppsFlyerLib appsFlyerLib = AppsFlyerLib.getInstance(); | |
| appsFlyerLib.init(BuildConfig.APPSFLYER_ID, null, this); | |
| appsFlyerLib.setAppInviteOneLink(BuildConfig.APPSFLYER_TEMPLATE_ID); | |
| appsFlyerLib.setDebugLog(true); | |
| appsFlyerLib.setCustomerUserId(UUID.randomUUID().toString()); | |
| String customerId = config.getUniqueId(); | |
| appsFlyerLib.setCustomerUserId(customerId); | |
| AppsFlyerLib appsFlyerLib = AppsFlyerLib.getInstance(); | |
| appsFlyerLib.init(BuildConfig.APPSFLYER_ID, null, this); | |
| appsFlyerLib.setAppInviteOneLink(BuildConfig.APPSFLYER_TEMPLATE_ID); | |
| appsFlyerLib.setDebugLog(true); | |
| String customerId = config.getUniqueId(); | |
| appsFlyerLib.setCustomerUserId(customerId); |
🤖 Prompt for AI Agents
In `@wallet/src/de/schildbach/wallet/WalletApplication.java` around lines 297 -
303, Remove the redundant call that sets a random UUID before the real
persistent ID: in the AppsFlyer initialization block (AppsFlyerLib appsFlyerLib
= AppsFlyerLib.getInstance(); appsFlyerLib.init(...); ...), eliminate the call
appsFlyerLib.setCustomerUserId(UUID.randomUUID().toString()) so only
appsFlyerLib.setCustomerUserId(customerId) (where customerId =
config.getUniqueId()) is used; if a fallback is desired instead, wrap the
UUID.randomUUID() assignment to be used only when config.getUniqueId() is
null/empty and then call setCustomerUserId once with the chosen value.
| AppsFlyerLib appsFlyerLib = AppsFlyerLib.getInstance(); | ||
| appsFlyerLib.init(BuildConfig.APPSFLYER_ID, null, this); | ||
| appsFlyerLib.setAppInviteOneLink(BuildConfig.APPSFLYER_TEMPLATE_ID); | ||
| appsFlyerLib.setDebugLog(true); |
There was a problem hiding this comment.
setDebugLog(true) is enabled for all builds including production.
This enables verbose AppsFlyer debug logging in release builds, which can expose SDK internals, API keys, and configuration details in device logs. Consider gating on BuildConfig.DEBUG.
Proposed fix
- appsFlyerLib.setDebugLog(true);
+ appsFlyerLib.setDebugLog(BuildConfig.DEBUG);🤖 Prompt for AI Agents
In `@wallet/src/de/schildbach/wallet/WalletApplication.java` at line 300, The
AppsFlyer debug logging is unconditionally enabled via
appsFlyerLib.setDebugLog(true); change it to enable debug logging only for
non-production builds by gating the call with BuildConfig.DEBUG (or equivalent
debug flag) so setDebugLog(true) runs only when BuildConfig.DEBUG is true and
setDebugLog(false) or omit the call in release; update the code near the
appsFlyerLib.setDebugLog invocation in WalletApplication.java to check
BuildConfig.DEBUG before enabling debug logging.
| private void handleDeepLinkData(Map<String, Object> data) { | ||
| String deepLinkValue = extractDeepLink(data); | ||
| if (deepLinkValue != null) { | ||
| log.info("Processing deep link: {}", deepLinkValue); | ||
| new android.os.Handler(android.os.Looper.getMainLooper()).post(() -> { | ||
| Intent intent = new Intent(getApplicationContext(), de.schildbach.wallet.ui.InviteHandlerActivity.class); | ||
| intent.setData(Uri.parse(deepLinkValue)); | ||
| intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK); | ||
| startActivity(intent); | ||
| }); | ||
| } else { | ||
| log.warn("No deep link found in AppsFlyer data"); | ||
| } | ||
| } | ||
|
|
||
| private String extractDeepLink(Map<String, Object> data) { | ||
| // Define keys in order of preference | ||
| String[] possibleKeys = {"af_dp", "deep_link_value", "link", "af_sub1"}; | ||
| for (String key : possibleKeys) { | ||
| Object value = data.get(key); | ||
| if (value instanceof String) { | ||
| String stringValue = (String) value; | ||
| if (!stringValue.trim().isEmpty()) { | ||
| log.info("Found deep link in key '{}': {}", key, stringValue); | ||
| return stringValue; | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Deep link URLs containing private keys are logged in production.
handleDeepLinkData and extractDeepLink log the full deep link value (lines 340, 360). Invitation links include PARAM_PRIVATE_KEY (a WIF-encoded private key) in the URL query string. Logging this to device logs and logcat in production is a security risk — any app with READ_LOGS permission or adb access could capture it.
Consider redacting or omitting the URL value in logs, or only logging in debug builds.
🤖 Prompt for AI Agents
In `@wallet/src/de/schildbach/wallet/WalletApplication.java` around lines 337 -
366, Remove production logging of full deep-link URLs (which may contain
PARAM_PRIVATE_KEY/WIF) in handleDeepLinkData and extractDeepLink: stop calling
log.info with the raw deepLinkValue/stringValue; instead either (a) only log
that a deep link was found and its source key (e.g., in extractDeepLink log the
key name but not the URL), or (b) conditionally log a redacted value only when
BuildConfig.DEBUG is true. Update handleDeepLinkData to avoid logging
deepLinkValue (use a presence/intent-launched message or debug-only masked
logging) and update extractDeepLink to log only the key name or a masked
representation (no full URL or query params).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@wallet/src/de/schildbach/wallet/ui/dashpay/notification/UserAlertViewHolder.kt`:
- Around line 35-37: UserAlertViewHolder currently no-ops when
notificationItem.getUserAlertId() returns null (e.g., getIdFromStringRes/textMap
misses an entry); change the close button handler to detect a null id, log a
warning (mentioning getIdFromStringRes/textMap and the notificationItem) and
still perform a fallback dismiss so the button always works — for example invoke
onUserAlertDismiss with a safe sentinel id (e.g., "unknown_alert") or a
generated fallback from the notificationItem, and ensure the log uses your
existing logger so maintainers can trace missing textMap entries.
🧹 Nitpick comments (1)
wallet/src/de/schildbach/wallet/ui/dashpay/UserAlert.kt (1)
40-41: Consider a more idiomatic reverse lookup.
filter+map+firstOrNullcreates intermediate collections. A simpler idiomatic form:♻️ Suggested simplification
fun getIdFromStringRes(`@StringRes` stringResId: Int) = - textMap.filter { it.value == stringResId }.map { it.key }.firstOrNull() + textMap.entries.firstOrNull { it.value == stringResId }?.key
| notificationItem.getUserAlertId()?.let { id -> | ||
| onUserAlertDismiss.invoke(id) | ||
| } |
There was a problem hiding this comment.
Silent no-op when getUserAlertId() returns null.
If getIdFromStringRes doesn't find a match (e.g., a new alert type is added without updating textMap), the close button silently does nothing — the user taps dismiss and the alert stays. Consider logging a warning or falling back so the button remains functional:
Suggested defensive handling
closeBtn.setOnClickListener {
- notificationItem.getUserAlertId()?.let { id ->
- onUserAlertDismiss.invoke(id)
- }
+ val alertId = notificationItem.getUserAlertId()
+ if (alertId != null) {
+ onUserAlertDismiss.invoke(alertId)
+ } else {
+ log.warn("No alert ID found for stringResId={}", notificationItem.stringResId)
+ }
}🤖 Prompt for AI Agents
In
`@wallet/src/de/schildbach/wallet/ui/dashpay/notification/UserAlertViewHolder.kt`
around lines 35 - 37, UserAlertViewHolder currently no-ops when
notificationItem.getUserAlertId() returns null (e.g., getIdFromStringRes/textMap
misses an entry); change the close button handler to detect a null id, log a
warning (mentioning getIdFromStringRes/textMap and the notificationItem) and
still perform a fallback dismiss so the button always works — for example invoke
onUserAlertDismiss with a safe sentinel id (e.g., "unknown_alert") or a
generated fallback from the notificationItem, and ensure the log uses your
existing logger so maintainers can trace missing textMap entries.
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@wallet/src/de/schildbach/wallet/ui/invite/CreateInviteViewModel.kt`:
- Line 54: The public property invitations currently exposes a mutable
MutableStateFlow allowing external mutation; change it to a private mutable
backing property named _invitations:
MutableStateFlow<List<Invitation>>(emptyList()) and expose a public read-only
invitations = _invitations.asStateFlow(); then update the collector that
currently assigns to invitations (the write on line referenced) to write to
_invitations instead; ensure imports for asStateFlow() are present and adjust
any call sites to the now-immutable invitations.
In `@wallet/src/de/schildbach/wallet/ui/more/ConfirmInviteDialogFragment.kt`:
- Around line 74-78: The catch block in ConfirmInviteDialogFragment that
currently calls log.info("error sending transaction:", e) should use log.error
(or log.warn) to reflect the severity; update the exception logging in the catch
within the method that sends the invite transaction (the catch that updates
binding.confirmMessage.text and binding.confirmMessage.isVisible) to call
log.error and include the exception object so the stacktrace is recorded.
- Line 55: The code computes Coin.valueOf(args.amount) twice; instead of
recomputing it for inviteAmount, reuse the previously assigned viewModel.amount:
remove the second Coin.valueOf(args.amount) and set inviteAmount (or use
inviteAmount variable) from viewModel.amount (or eliminate inviteAmount and
reference viewModel.amount directly) so you don't duplicate the conversion
(referencing viewModel.amount, args.amount, and inviteAmount to locate the
spots).
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@wallet/src/de/schildbach/wallet/ui/invite/CreateInviteViewModel.kt`: - Line 54: The public property invitations currently exposes a mutable MutableStateFlow allowing external mutation; change it to a private mutable backing property named _invitations: MutableStateFlow<List<Invitation>>(emptyList()) and expose a public read-only invitations = _invitations.asStateFlow(); then update the collector that currently assigns to invitations (the write on line referenced) to write to _invitations instead; ensure imports for asStateFlow() are present and adjust any call sites to the now-immutable invitations. In `@wallet/src/de/schildbach/wallet/ui/more/ConfirmInviteDialogFragment.kt`: - Around line 74-78: The catch block in ConfirmInviteDialogFragment that currently calls log.info("error sending transaction:", e) should use log.error (or log.warn) to reflect the severity; update the exception logging in the catch within the method that sends the invite transaction (the catch that updates binding.confirmMessage.text and binding.confirmMessage.isVisible) to call log.error and include the exception object so the stacktrace is recorded. - Line 55: The code computes Coin.valueOf(args.amount) twice; instead of recomputing it for inviteAmount, reuse the previously assigned viewModel.amount: remove the second Coin.valueOf(args.amount) and set inviteAmount (or use inviteAmount variable) from viewModel.amount (or eliminate inviteAmount and reference viewModel.amount directly) so you don't duplicate the conversion (referencing viewModel.amount, args.amount, and inviteAmount to locate the spots).wallet/src/de/schildbach/wallet/ui/invite/CreateInviteViewModel.kt (1)
54-54:invitationsexposes a mutableMutableStateFlowpublicly.External consumers can mutate the state directly. Consider the private-mutable / public-immutable pattern:
- val invitations = MutableStateFlow<List<Invitation>>(listOf()) + private val _invitations = MutableStateFlow<List<Invitation>>(listOf()) + val invitations = _invitations.asStateFlow()And update the collector on line 58 to write to
_invitations.As per coding guidelines: "Use private mutable
_uiStatewith public immutableuiStateviaasStateFlow()in ViewModels."🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@wallet/src/de/schildbach/wallet/ui/invite/CreateInviteViewModel.kt` at line 54, The public property invitations currently exposes a mutable MutableStateFlow allowing external mutation; change it to a private mutable backing property named _invitations: MutableStateFlow<List<Invitation>>(emptyList()) and expose a public read-only invitations = _invitations.asStateFlow(); then update the collector that currently assigns to invitations (the write on line referenced) to write to _invitations instead; ensure imports for asStateFlow() are present and adjust any call sites to the now-immutable invitations.wallet/src/de/schildbach/wallet/ui/more/ConfirmInviteDialogFragment.kt (2)
74-78: Consider usinglog.errorfor the exception path.
log.infoon an exception during invite transaction send understates the severity.log.errororlog.warnwould be more appropriate for error triage in production logs.- log.info("error sending transaction:", e) + log.error("error sending transaction:", e)🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@wallet/src/de/schildbach/wallet/ui/more/ConfirmInviteDialogFragment.kt` around lines 74 - 78, The catch block in ConfirmInviteDialogFragment that currently calls log.info("error sending transaction:", e) should use log.error (or log.warn) to reflect the severity; update the exception logging in the catch within the method that sends the invite transaction (the catch that updates binding.confirmMessage.text and binding.confirmMessage.isVisible) to call log.error and include the exception object so the stacktrace is recorded.
55-55: Minor:inviteAmountduplicatesviewModel.amount.
Coin.valueOf(args.amount)is computed on line 55 and assigned toviewModel.amount, then recomputed on line 59 asinviteAmount. You could reuseviewModel.amountdirectly to avoid the duplication.- val inviteAmount = Coin.valueOf(args.amount) + val inviteAmount = viewModel.amountAlso applies to: 59-59
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@wallet/src/de/schildbach/wallet/ui/more/ConfirmInviteDialogFragment.kt` at line 55, The code computes Coin.valueOf(args.amount) twice; instead of recomputing it for inviteAmount, reuse the previously assigned viewModel.amount: remove the second Coin.valueOf(args.amount) and set inviteAmount (or use inviteAmount variable) from viewModel.amount (or eliminate inviteAmount and reference viewModel.amount directly) so you don't duplicate the conversion (referencing viewModel.amount, args.amount, and inviteAmount to locate the spots).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wallet/src/de/schildbach/wallet/ui/InviteHandlerViewModel.kt (1)
58-59: Remove commented-out dead code.
_validationState/validationStatehave been fully commented out. They should either be deleted or tracked in an issue if the feature is deferred.♻️ Proposed cleanup
-// private val _validationState = MutableStateFlow(InvitationValidationState.NONE) -// val validationState: StateFlow<InvitationValidationState?> = _validationState.asStateFlow() -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/InviteHandlerViewModel.kt` around lines 58 - 59, Remove the dead commented-out state flow declarations in InviteHandlerViewModel: delete the two commented lines containing "_validationState" and "validationState" (and any leftover reference to InvitationValidationState if unused), or if this feature is intentionally deferred, create a tracking issue and replace the commented code with a single TODO comment that references that issue and InvitationValidationState; ensure no unused imports or references remain after removal.wallet/res/layout/fragment_request_username.xml (1)
103-116: LGTM — ScrollView integration is correct.The
height="0dp"+constraintTop_toBottomOf/constraintBottom_toTopOfpair correctly fills the space betweentop_stackand the button.fillViewport="true"is appropriate to prevent collapsed layout on short content, andrequirements_stackas the solewrap_contentchild follows the expected ScrollView pattern.One optional tidy-up: the Android ConstraintLayout docs recommend
android:layout_width="0dp"with explicitapp:layout_constraintStart_toStartOf="parent"andapp:layout_constraintEnd_toEndOf="parent"overmatch_parentfor direct children of ConstraintLayout, asmatch_parentcan silently conflict with padding applied to the root. All existing children in this file usematch_parent, so this is a pre-existing pattern and not a blocker.♻️ Align with ConstraintLayout recommended width idiom (optional)
<ScrollView android:id="@+id/scroll_view" - android:layout_width="match_parent" + android:layout_width="0dp" android:layout_height="0dp" android:fillViewport="true" android:layout_marginHorizontal="15dp" app:layout_constraintTop_toBottomOf="@+id/top_stack" - app:layout_constraintBottom_toTopOf="@+id/request_username_button"> + app:layout_constraintBottom_toTopOf="@+id/request_username_button" + app:layout_constraintStart_toStartOf="parent" + app:layout_constraintEnd_toEndOf="parent">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/res/layout/fragment_request_username.xml` around lines 103 - 116, The ScrollView currently uses android:layout_width="match_parent" while placed in a ConstraintLayout; change its width to "0dp" and add app:layout_constraintStart_toStartOf="parent" and app:layout_constraintEnd_toEndOf="parent" so it follows the ConstraintLayout recommended idiom (reference ids: ScrollView @+id/scroll_view and its child LinearLayout @+id/requirements_stack); keep android:layout_height="0dp" and the existing top/bottom constraints unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt`:
- Line 221: The observers and onLockScreenDeactivated call sites call
processInvitation without checking invitation.validationState, allowing re-entry
and re-showing dialogs; to fix, add a unified guard inside processInvitation
that returns early if invitation.validationState != null (and preserve existing
null/isSynced checks there), then revert the change at the LiveData observer
(the line that added validationState == null) back to only the null + isSynced
checks; update callers in the isBlockchainSynced observer and
onLockScreenDeactivated to call processInvitation as-is (no extra guards) so the
validationState check is enforced centrally within processInvitation.
---
Nitpick comments:
In `@wallet/res/layout/fragment_request_username.xml`:
- Around line 103-116: The ScrollView currently uses
android:layout_width="match_parent" while placed in a ConstraintLayout; change
its width to "0dp" and add app:layout_constraintStart_toStartOf="parent" and
app:layout_constraintEnd_toEndOf="parent" so it follows the ConstraintLayout
recommended idiom (reference ids: ScrollView @+id/scroll_view and its child
LinearLayout @+id/requirements_stack); keep android:layout_height="0dp" and the
existing top/bottom constraints unchanged.
In `@wallet/src/de/schildbach/wallet/ui/InviteHandlerViewModel.kt`:
- Around line 58-59: Remove the dead commented-out state flow declarations in
InviteHandlerViewModel: delete the two commented lines containing
"_validationState" and "validationState" (and any leftover reference to
InvitationValidationState if unused), or if this feature is intentionally
deferred, create a tracking issue and replace the commented code with a single
TODO comment that references that issue and InvitationValidationState; ensure no
unused imports or references remain after removal.
| inviteHandlerViewModel.invitation.observe(viewLifecycleOwner) { invitation -> | ||
| val isSynced = viewModel.isBlockchainSynced.value == true | ||
| if (invitation != null && isSynced) { | ||
| if (invitation != null && isSynced && invitation.validationState == null) { |
There was a problem hiding this comment.
Guard inconsistency: isBlockchainSynced and onLockScreenDeactivated paths bypass the validationState == null check
The new guard prevents re-entry from the invitation LiveData observer after validation. However, two other call sites are unguarded:
- Line 184 (
isBlockchainSyncedobserver): callsprocessInvitation(inviteHandlerViewModel.invitation.value!!, ...)unconditionally whenever sync state changes. If the invitation has already been validated (validationState != null) and a sync event fires afterwards,processInvitationwill callshowInviteValidationDialogwith the validated state — potentially re-showing a dismissal dialog. - Lines 247–255 (
onLockScreenDeactivated): same issue; novalidationState == nullguard before callingprocessInvitation.
Consider adding the same guard at those call sites, or moving the guard into processInvitation itself to enforce it uniformly:
♻️ Proposed fix: hoist the guard into processInvitation
private suspend fun processInvitation(invitation: InvitationLinkData, isSynced: Boolean, isLockScreenActive: Boolean) {
+ if (invitation.validationState != null) return
if (isSynced) {Then revert line 221 back to only the null + isSynced checks:
- if (invitation != null && isSynced && invitation.validationState == null) {
+ if (invitation != null && isSynced) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt` at
line 221, The observers and onLockScreenDeactivated call sites call
processInvitation without checking invitation.validationState, allowing re-entry
and re-showing dialogs; to fix, add a unified guard inside processInvitation
that returns early if invitation.validationState != null (and preserve existing
null/isSynced checks there), then revert the change at the LiveData observer
(the line that added validationState == null) back to only the null + isSynced
checks; update callers in the isBlockchainSynced observer and
onLockScreenDeactivated to call processInvitation as-is (no extra guards) so the
validationState check is enforced centrally within processInvitation.
1 participant
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Style