Add push notifications service #4304
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add config
WalkthroughAdds a mobile notifications subsystem: device registration REST endpoints and DTOs, persistence (protobuf) for device profiles, a relay client with provider rotation/retries, mobile encryption and services, a NotificationService aggregator, config/build updates, and replaces SystemNotificationService usages with NotificationService across consumers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Mobile Client
participant API as DevicesRestApi
participant DevRegSvc as DeviceRegistrationService
participant Store as DeviceRegistrationStore
participant PersistSvc as PersistenceService
Client->>API: POST /mobile-devices/registrations (deviceId, token, pubKey, descriptor, platform)
API->>API: Validate fields (presence, iOS token hex)
alt Validation fails
API-->>Client: 400 Bad Request
else
API->>DevRegSvc: register(deviceId, token, pubKey, descriptor, platform)
DevRegSvc->>Store: add/replace MobileDeviceProfile
Store->>PersistSvc: persist store
PersistSvc-->>Store: persisted
DevRegSvc-->>API: success
API-->>Client: 200 OK
end
sequenceDiagram
participant ChatSvc as ChatNotificationService
participant NotifSvc as NotificationService
participant SysSvc as SystemNotificationService
participant MobileSvc as MobileNotificationService
participant DevRegSvc as DeviceRegistrationService
participant Encrypt as MobileNotificationEncryption
participant Relay as MobileNotificationRelayClient
ChatSvc->>NotifSvc: dispatchNotification(notification)
par System path
NotifSvc->>SysSvc: dispatchNotification(notification)
SysSvc-->>NotifSvc: OS popup
and Mobile path
NotifSvc->>MobileSvc: dispatchNotification(notification)
MobileSvc->>DevRegSvc: getMobileDeviceProfiles()
DevRegSvc-->>MobileSvc: [profiles]
loop per profile
MobileSvc->>MobileSvc: build payload JSON
MobileSvc->>Encrypt: encrypt(pubKeyBase64, payload)
Encrypt-->>MobileSvc: encrypted payload
MobileSvc->>Relay: sendToRelayServer(isAndroid, token, encrypted)
Relay-->>MobileSvc: success/failure
end
end
sequenceDiagram
participant Relay as MobileNotificationRelayClient
participant ProviderA as ProviderA
participant ProviderB as ProviderB
participant HTTP as BaseHttpClient
Relay->>Relay: selectNextProvider()
Relay->>ProviderA: POST /notification (token, payload)
alt 200 OK
ProviderA-->>Relay: 200 OK
Relay->>Relay: mark provider healthy
else 408/429/5xx (transient)
ProviderA-->>Relay: transient error
Relay->>Relay: mark provider failed
Relay->>Relay: selectNextProvider()
Relay->>ProviderB: POST retry
ProviderB-->>Relay: OK/Error
else timeout / permanent error
ProviderA-->>Relay: failure
Relay-->>Relay: exhaust providers -> fail
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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
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: 8
🤖 Fix all issues with AI agents
In `@api/src/main/java/bisq/api/rest_api/endpoints/devices/DevicesRestApi.java`:
- Around line 107-123: The parameter validation in
DeviceRegistrationService.register() and DeviceRegistrationService.unregister()
is inverted because checkArgument is given StringUtils.isEmpty(...) instead of
its negation; update the four checks in register() and the check in unregister()
to use checkArgument(!StringUtils.isEmpty(...), "...") so that non-empty inputs
pass and empty/null inputs throw; locate the checks by searching for
checkArgument and StringUtils.isEmpty within DeviceRegistrationService.register
and DeviceRegistrationService.unregister and apply the negation to each
condition.
In `@api/src/main/java/bisq/api/rest_api/endpoints/RestApiBase.java`:
- Around line 39-46: The buildNoContentResponse method currently returns a 204
with an empty string body which can cause a payload to be serialized; update
RestApiBase.buildNoContentResponse to return a true no-content response (no
entity) — e.g. call Response.status(Response.Status.NO_CONTENT).build() or
invoke buildResponse with a null/absent entity instead of "" so that
buildResponse (and any underlying response creation) does not attach an entity
for HTTP 204 responses.
In
`@bonded-roles/src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java`:
- Line 217: Update the erroneous exception message in
MobileNotificationRelayClient where it returns
CompletableFuture.failedFuture(new RuntimeException(...)); the current message
mentions "No block explorer provider available" by mistake—replace it with a
correct message referencing the mobile notification relay (e.g., "No mobile
notification relay provider available" or similar) so the RuntimeException and
any logs accurately reflect MobileNotificationRelayClient's failure to find a
relay provider.
- Line 58: The static ExecutorService POOL created via
ExecutorFactory.newCachedThreadPool in MobileNotificationRelayClient is never
shut down; update the class to properly terminate it by either (A) adding a
public static shutdown method (e.g., shutdownPool()) that calls POOL.shutdown(),
awaits termination with a timeout and calls POOL.shutdownNow() if needed, and
ensure callers invoke it during application termination, or (B) convert POOL to
an instance field and add logic in the existing shutdown() method to call
shutdown()/awaitTermination()/shutdownNow() on that instance; reference POOL,
shutdown(), httpClient, MobileNotificationRelayClient and
ExecutorFactory.newCachedThreadPool when making the change.
- Around line 138-142: The shared mutable Set fields (candidates,
providersFromConfig, fallbackProviders, failedProviders) are plain HashSet
instances and are modified from within asynchronous tasks (e.g., inside
sendToRelayServer's supplyAsync blocks), causing race conditions; replace these
sets with concurrent sets (e.g., ConcurrentHashMap.newKeySet()) or guard all
reads/writes with a single lock to ensure thread-safety, and update any code
that assumes HashSet semantics accordingly (look for uses in
MobileNotificationRelayClient methods such as sendToRelayServer and any lambda
passed to supplyAsync).
In
`@notifications/src/main/java/bisq/notifications/mobile/registration/DeviceRegistrationService.java`:
- Around line 71-72: The unregister method has the same inverted validation: the
checkArgument currently passes StringUtils.isEmpty(deviceId) which allows
null/empty values; change the validation in DeviceRegistrationService.unregister
to assert the deviceId is not empty (e.g., use !StringUtils.isEmpty(deviceId) or
a non-empty helper like StringUtils.isNotEmpty(deviceId)) so checkArgument
throws when deviceId is null/empty and keep the existing error message "deviceId
must not be null or empty".
- Around line 50-54: The validation in DeviceRegistrationService is inverted:
the checkArgument calls use StringUtils.isEmpty(...) which is true for
null/empty and thus throws for valid input; update each call (deviceId,
deviceToken, publicKeyBase64, deviceDescriptor) to assert non-emptiness by using
the negation (e.g., checkArgument(!StringUtils.isEmpty(deviceId), "...")) or
StringUtils.isNotEmpty(...) so valid non-empty strings pass and empty/null
values fail; leave the checkNotNull(platform, ...) as-is and keep the existing
error messages.
In
`@security/src/main/java/bisq/security/mobile_notifications/MobileNotificationEncryption.java`:
- Around line 43-72: The comment and usage in encrypt(String publicKeyBase64,
String message) incorrectly claim Cipher.getInstance("ECIES") yields
AES-128-CBC; change the code to explicitly request the AES-CBC ECIES variant by
using Cipher.getInstance("ECIESwithAES-CBC", BouncyCastleProvider.PROVIDER_NAME)
and construct IESParameterSpec with the additional cipherKeySize argument (e.g.,
new IESParameterSpec(derivation, encoding, macKeySize, cipherKeySize)) to
specify the AES key length, and update the surrounding comment to remove the
misleading statement about the default ECIES streaming mode and clearly document
that AES-CBC is selected explicitly via the transformation and cipherKeySize
parameter.
🧹 Nitpick comments (12)
apps/desktop/desktop-app/src/main/resources/desktop.conf (1)
157-169: Consider adding a fallback relay to avoid a single point of failure.With
enabled = trueand an emptyfallbackProviders, a relay outage would disable push notifications. Adding at least one fallback (or documenting the operational expectation) would improve resiliency.api/src/main/java/bisq/api/ApiService.java (1)
168-168: Minor style issue: missing space before=.- DevicesRestApi devicesRestApi= new DevicesRestApi(deviceRegistrationService); + DevicesRestApi devicesRestApi = new DevicesRestApi(deviceRegistrationService);apps/seed-node-app/src/main/resources/seed_node.conf (1)
155-155: Minor formatting: inconsistent indentation on closing brace.The closing brace for
blockchainExplorerappears to have reduced indentation compared to the pattern used elsewhere. Consider aligning it with the other block closures.- } + }notifications/src/main/java/bisq/notifications/mobile/MobileNotificationService.java (2)
52-56: Consider reversing shutdown order for proper cleanup.The shutdown sequence should typically reverse the initialization order to ensure proper cleanup of dependencies. Currently both methods chain in the same order (
deviceRegistrationService→mobileNotificationRelayClient).♻️ Suggested fix
public CompletableFuture<Boolean> shutdown() { log.info("shutdown"); - return deviceRegistrationService.shutdown() - .thenCompose(e -> mobileNotificationRelayClient.shutdown()); + return mobileNotificationRelayClient.shutdown() + .thenCompose(e -> deviceRegistrationService.shutdown()); }
58-75: Consider handlingUNSPECIFIEDplatform explicitly.When
mobileDeviceProfile.getPlatform()returnsUNSPECIFIED, the current logic treats it as iOS (non-Android). Consider logging a warning or skipping devices with an unspecified platform to avoid sending notifications with incorrect parameters.♻️ Suggested improvement
public void dispatchNotification(Notification notification) { deviceRegistrationService.getMobileDeviceProfiles() .forEach(mobileDeviceProfile -> { + MobileDevicePlatform platform = mobileDeviceProfile.getPlatform(); + if (platform == MobileDevicePlatform.UNSPECIFIED) { + log.warn("Skipping device {} with UNSPECIFIED platform", mobileDeviceProfile.getDeviceId()); + return; + } - boolean isAndroid = mobileDeviceProfile.getPlatform() == MobileDevicePlatform.ANDROID; + boolean isAndroid = platform == MobileDevicePlatform.ANDROID; String deviceTokenHex = mobileDeviceProfile.getDeviceToken();notifications/src/main/proto/notifications.proto (1)
24-28: Consider aligning enum value names with the enum type.The enum values use a
DEVICEREGISTRATIONPLATFORM_prefix, but the enum is namedMobileDevicePlatform. Proto3 convention typically prefixes values with the enum name to avoid collisions:enum MobileDevicePlatform { MOBILE_DEVICE_PLATFORM_UNSPECIFIED = 0; MOBILE_DEVICE_PLATFORM_IOS = 1; MOBILE_DEVICE_PLATFORM_ANDROID = 2; }This is a minor naming consistency improvement and can be deferred if there are existing consumers relying on the current names.
apps/api-app/src/main/java/bisq/api_app/ApiApplicationService.java (1)
184-185: Consider null safety for the chained service access.The accessor chain
notificationService.getMobileNotificationService().getDeviceRegistrationService()assumes all intermediate services are non-null. While this should be the case given the initialization order, consider whether defensive handling or documentation is warranted.notifications/src/main/java/bisq/notifications/NotificationService.java (1)
59-62: Consider resilience if one dispatch fails.If
systemNotificationService.dispatchNotification()throws an exception,mobileNotificationService.dispatchNotification()will not be called. Consider wrapping each call in try-catch to ensure both services receive the notification attempt.♻️ Suggested improvement
public void dispatchNotification(Notification notification) { - systemNotificationService.dispatchNotification(notification); - mobileNotificationService.dispatchNotification(notification); + try { + systemNotificationService.dispatchNotification(notification); + } catch (Exception e) { + log.error("Failed to dispatch system notification", e); + } + try { + mobileNotificationService.dispatchNotification(notification); + } catch (Exception e) { + log.error("Failed to dispatch mobile notification", e); + } }bonded-roles/src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java (4)
75-91: Duplicate provider parsing logic.The mapping logic for
providersandfallbackProvidersis identical. Consider extracting a helper method to reduce duplication.Proposed refactor
+ private static Set<Provider> parseProviders(java.util.List<? extends com.typesafe.config.Config> configList) { + return configList.stream() + .map(config -> { + String url = config.getString("url"); + String operator = config.getString("operator"); + TransportType transportType = getTransportTypeFromUrl(url); + return new Provider(url, operator, transportType); + }) + .collect(Collectors.toUnmodifiableSet()); + } + public static Config from(com.typesafe.config.Config typesafeConfig) { long timeoutInSeconds = typesafeConfig.getLong("timeoutInSeconds"); - Set<Provider> providers = typesafeConfig.getConfigList("providers").stream() - .map(config -> { - String url = config.getString("url"); - String operator = config.getString("operator"); - TransportType transportType = getTransportTypeFromUrl(url); - return new Provider(url, operator, transportType); - }) - .collect(Collectors.toUnmodifiableSet()); - - Set<Provider> fallbackProviders = typesafeConfig.getConfigList("fallbackProviders").stream() - .map(config -> { - String url = config.getString("url"); - String operator = config.getString("operator"); - TransportType transportType = getTransportTypeFromUrl(url); - return new Provider(url, operator, transportType); - }) - .collect(Collectors.toUnmodifiableSet()); + Set<Provider> providers = parseProviders(typesafeConfig.getConfigList("providers")); + Set<Provider> fallbackProviders = parseProviders(typesafeConfig.getConfigList("fallbackProviders")); return new Config(timeoutInSeconds, providers, fallbackProviders); }
273-279: Usingnullas a sentinel for timeout is fragile.
completeOnTimeout(null, ...)usesnullto signal timeout, which is then checked inthenCompose. While this works, it's easy to accidentally break if future changes don't account for the null case.Consider using
completeOnTimeoutwith a dedicated sentinel value ororTimeout()which throwsTimeoutExceptiondirectly.
316-321: Silent exception swallowing in shutdown helper.Exceptions during HTTP client shutdown are silently ignored. Consider logging at debug level to aid troubleshooting.
Proposed fix
private void shutdownHttpClient(BaseHttpClient client) { try { client.shutdown(); - } catch (Exception ignore) { + } catch (Exception e) { + log.debug("Exception during HTTP client shutdown", e); } }
50-51: TODO comments indicate work in progress.These TODO comments suggest the implementation is incomplete. Should these be addressed before merging, or tracked as separate issues?
Would you like me to open issues to track these TODOs?
api/src/main/java/bisq/api/rest_api/endpoints/devices/DevicesRestApi.java
Show resolved
Hide resolved
...src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java
Show resolved
Hide resolved
...src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java
Show resolved
Hide resolved
...src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java
Outdated
Show resolved
Hide resolved
...ications/src/main/java/bisq/notifications/mobile/registration/DeviceRegistrationService.java
Outdated
Show resolved
Hide resolved
...ications/src/main/java/bisq/notifications/mobile/registration/DeviceRegistrationService.java
Outdated
Show resolved
Hide resolved
security/src/main/java/bisq/security/mobile_notifications/MobileNotificationEncryption.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@notifications/src/main/java/bisq/notifications/mobile/registration/DeviceRegistrationService.java`:
- Around line 62-69: The registration currently uses
persistableStore.getDeviceByDeviceId().putIfAbsent(deviceId,
mobileDeviceProfile) which prevents updating an existing device token and still
calls persist() even when nothing changed; change this to use put(deviceId,
mobileDeviceProfile) so re-registrations overwrite stale tokens, and only call
persist() when an actual put/replace occurred (or log when an existing
registration is detected) — update the DeviceRegistrationService method that
constructs MobileDeviceProfile and interacts with
persistableStore.getDeviceByDeviceId() accordingly.
...ications/src/main/java/bisq/notifications/mobile/registration/DeviceRegistrationService.java
Show resolved
Hide resolved
HenrikJannsen
force-pushed
the
add-push-notifications-service
branch
from
ed6adaa to
23aa116
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@api/src/main/java/bisq/api/rest_api/endpoints/devices/DevicesRestApi.java`:
- Around line 53-101: The deviceToken validation currently uses
ALPHANUMERIC_REGEX and the hardcoded APNS_TOKEN_LENGTH in registerDevice, which
incorrectly rejects valid FCM tokens and mis-validates APNS tokens; change this
to perform platform-specific validation using MobileDevicePlatform: remove the
generic ALPHANUMERIC_REGEX check and instead, when platform ==
MobileDevicePlatform.APNS validate deviceToken against a hex-only regex like
"[0-9a-fA-F]+" (no fixed length check), and for MobileDevicePlatform.FCM skip
character-set validation entirely (accept opaque tokens) — update or remove
APNS_TOKEN_LENGTH and any references to it and ensure the log and error response
reflect the platform-specific validation rule.
In
`@bonded-roles/src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java`:
- Around line 286-293: selectNextProvider can still encounter a race where
candidates becomes empty between the isEmpty() check and
CollectionUtil.getRandomElement(), causing a null Provider; update
selectNextProvider to be thread-safe by synchronizing the method or the block
that accesses candidates, and after calling
CollectionUtil.getRandomElement(candidates) immediately check for null and
either call fillCandidates(0) and retry (loop until a non-null Provider) or
throw a clear IllegalStateException; reference the selectNextProvider method,
the candidates field, fillCandidates(int) and
CollectionUtil.getRandomElement(...) when making the change.
- Around line 159-165: The current calculation of numTotalCandidates
(providersFromConfig.size() + fallbackProviders.size()) can double-count
overlapping providers; instead compute the unique provider count by taking the
union of providersFromConfig and fallbackProviders (or simply use candidates
after it's built) and assign numTotalCandidates to that unique size so retries
reflect actual distinct providers (refer to MobileNotificationRelayClient,
variables providersFromConfig, fallbackProviders, candidates, and
numTotalCandidates).
- Around line 95-103: The current getTransportTypeFromUrl(String url) uses
url.endsWith(...) which fails for URLs with ports or paths; update it to parse
the URL and inspect the host component (e.g., new URI/URL(url).getHost()) and
then check host != null && (host.endsWith(".onion") -> TransportType.TOR,
host.endsWith(".i2p") -> TransportType.I2P), falling back to
TransportType.CLEAR; also handle malformed URLs by catching
URISyntaxException/MalformedURLException and falling back to a safer check
(e.g., host == null ? check url for domain tokens with boundary/delimiter checks
or default to CLEAR) so TransportType detection is robust for ports and paths.
🧹 Nitpick comments (2)
bonded-roles/src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java (2)
183-205: Redundant try-catch block:RejectedExecutionExceptioncannot be thrown here.The
try-catchwrapping the call to the privatesendToRelayServermethod cannot catchRejectedExecutionExceptionbecause the private method already handles this exception internally (lines 280-283) and returns a failed future instead of throwing. This outer try-catch is dead code.♻️ Proposed simplification
public CompletableFuture<Boolean> sendToRelayServer(boolean isAndroid, String deviceTokenHex, String encryptedMessageHex) { - try { - return sendToRelayServer(isAndroid, - deviceTokenHex, - encryptedMessageHex, - new AtomicInteger(0)) - .exceptionallyCompose(throwable -> { - if (throwable instanceof RetryException retryException) { - return sendToRelayServer(isAndroid, - deviceTokenHex, - encryptedMessageHex, - retryException.getRecursionDepth()); - } else if (ExceptionUtil.getRootCause(throwable) instanceof RetryException retryException) { - return sendToRelayServer(isAndroid, - deviceTokenHex, - encryptedMessageHex, - retryException.getRecursionDepth()); - } else { - return CompletableFuture.failedFuture(throwable); - } - }); - } catch (RejectedExecutionException e) { - return CompletableFuture.failedFuture(new RejectedExecutionException("Too many requests. Try again later.")); - } + return sendToRelayServer(isAndroid, + deviceTokenHex, + encryptedMessageHex, + new AtomicInteger(0)) + .exceptionallyCompose(throwable -> { + if (throwable instanceof RetryException retryException) { + return sendToRelayServer(isAndroid, + deviceTokenHex, + encryptedMessageHex, + retryException.getRecursionDepth()); + } else if (ExceptionUtil.getRootCause(throwable) instanceof RetryException retryException) { + return sendToRelayServer(isAndroid, + deviceTokenHex, + encryptedMessageHex, + retryException.getRecursionDepth()); + } else { + return CompletableFuture.failedFuture(throwable); + } + }); }
273-279: Timeout handling usesnullas sentinel; consider explicit timeout exception.Using
completeOnTimeout(null, ...)and then checking fornullinthenComposeworks, but makes it impossible to distinguish between a legitimatenullreturn (if the supplier could return null) and a timeout. Since the supplier returnsBoolean, this is currently safe, but a more explicit approach usingorTimeout()with a customTimeoutExceptionwould be clearer and more robust.♻️ Alternative using orTimeout
- .completeOnTimeout(null, conf.getTimeoutInSeconds(), SECONDS) - .thenCompose(result -> { - if (result == null) { - return CompletableFuture.failedFuture(new RetryException("Timeout", recursionDepth)); - } - return CompletableFuture.completedFuture(result); - }); + .orTimeout(conf.getTimeoutInSeconds(), SECONDS) + .exceptionally(ex -> { + if (ex instanceof java.util.concurrent.TimeoutException) { + throw new CompletionException(new RetryException("Timeout", recursionDepth)); + } + throw ex instanceof CompletionException ? (CompletionException) ex : new CompletionException(ex); + });
api/src/main/java/bisq/api/rest_api/endpoints/devices/DevicesRestApi.java
Outdated
Show resolved
Hide resolved
...src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java
Show resolved
Hide resolved
...src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java
Show resolved
Hide resolved
...src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java
Show resolved
Hide resolved
HenrikJannsen
force-pushed
the
add-push-notifications-service
branch
from
23aa116 to
5eed858
Compare
Contributor
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@security/src/main/java/bisq/security/mobile_notifications/MobileNotificationEncryption.java`:
- Around line 36-42: The Javadoc for MobileNotificationEncryption.encrypt (in
class MobileNotificationEncryption) lists `@param` tags in the wrong order; update
the Javadoc so the `@param` entries match the method signature order by placing
the publicKeyBase64 description first and the message description second,
ensuring the parameter names exactly match the method parameters.
🧹 Nitpick comments (9)
api/src/main/java/bisq/api/rest_api/endpoints/RestApiBase.java (1)
39-46: Minor formatting nit: extra whitespace in method signature.The implementation correctly returns a 204 response without a body, which is the proper HTTP semantics. However, there's an extra space in the method signature.
🔧 Suggested fix
- protected Response buildNoContentResponse( ) { + protected Response buildNoContentResponse() {notifications/src/main/proto/notifications.proto (1)
29-35: Consider addingreserveddeclarations for future-proofing.For protobuf messages that will be persisted (like
MobileDeviceProfile), consider documenting field numbers as reserved if they're ever removed to prevent accidental reuse.bonded-roles/src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java (1)
180-185: Missinginitialize()method implementation.The class implements
Servicebut doesn't overrideinitialize(). While the default may return a completed future, this should be explicit for clarity, especially sinceshutdown()is explicitly implemented.Proposed addition
+ `@Override` + public CompletableFuture<Boolean> initialize() { + return CompletableFuture.completedFuture(true); + } + `@Override` public CompletableFuture<Boolean> shutdown() {apps/desktop/desktop-app/src/main/resources/desktop.conf (1)
157-169: Consider adding at least one fallback provider for resiliency.
Withenabled = trueand only one provider, delivery depends on a single relay; a fallback reduces outage risk.api/src/main/java/bisq/api/rest_api/endpoints/devices/RegisterDeviceRequest.java (1)
37-85: Add runtime validation for required fields (not just OpenAPI hints).
@Schema(required = true)doesn’t enforce at runtime, so null/blank values can slip in and later cause failures when buildingMobileDeviceProfile/protobufs. Consider adding Bean Validation annotations (if used) or explicit checks in the REST endpoint/service before constructing domain objects.api/src/main/java/bisq/api/ApiService.java (1)
168-168: Minor formatting: missing space before=.- DevicesRestApi devicesRestApi= new DevicesRestApi(deviceRegistrationService); + DevicesRestApi devicesRestApi = new DevicesRestApi(deviceRegistrationService);notifications/src/main/java/bisq/notifications/mobile/MobileNotificationService.java (1)
58-76: Consider async dispatch for multiple devices.
dispatchNotificationprocesses devices sequentially on the calling thread. If many devices are registered or relay calls are slow, this blocks the caller. Consider dispatching asynchronously or in parallel.Additionally,
deviceTokenHexis a misnomer since FCM tokens (Android) are not hex-encoded—consider renaming todeviceTokenfor clarity.♻️ Suggested async dispatch pattern
public void dispatchNotification(Notification notification) { deviceRegistrationService.getMobileDeviceProfiles() .forEach(mobileDeviceProfile -> { - boolean isAndroid = mobileDeviceProfile.getPlatform() == MobileDevicePlatform.ANDROID; - String deviceTokenHex = mobileDeviceProfile.getDeviceToken(); - MobileNotificationPayload payload = new MobileNotificationPayload(notification.getId(), - notification.getTitle(), - notification.getMessage()); - try { - String json = JsonMapperProvider.get().writeValueAsString(payload); - String encryptedMessageHex = MobileNotificationEncryption.encrypt(mobileDeviceProfile.getPublicKeyBase64(), json); - mobileNotificationRelayClient.sendToRelayServer(isAndroid, - deviceTokenHex, - encryptedMessageHex); - } catch (Exception e) { - log.error("Could not send notification to relay server", e); - } + CompletableFuture.runAsync(() -> { + boolean isAndroid = mobileDeviceProfile.getPlatform() == MobileDevicePlatform.ANDROID; + String deviceToken = mobileDeviceProfile.getDeviceToken(); + MobileNotificationPayload payload = new MobileNotificationPayload(notification.getId(), + notification.getTitle(), + notification.getMessage()); + try { + String json = JsonMapperProvider.get().writeValueAsString(payload); + String encryptedMessage = MobileNotificationEncryption.encrypt(mobileDeviceProfile.getPublicKeyBase64(), json); + mobileNotificationRelayClient.sendToRelayServer(isAndroid, + deviceToken, + encryptedMessage); + } catch (Exception e) { + log.error("Could not send notification to relay server for device {}", + mobileDeviceProfile.getDeviceId(), e); + } + }); }); }notifications/src/main/java/bisq/notifications/NotificationService.java (2)
53-57: Consider reversing shutdown order to match Bisq conventions.Throughout the codebase (e.g.,
ApiApplicationService,DesktopApplicationService), services are shut down in the reverse order of their initialization. Here,initialize()starts withsystemNotificationServicethenmobileNotificationService, butshutdown()uses the same order instead of reversing it.While not critical since these services are independent, reversing the order would maintain consistency with the established pattern.
♻️ Suggested fix
public CompletableFuture<Boolean> shutdown() { log.info("shutdown"); - return systemNotificationService.shutdown() - .thenCompose(e -> mobileNotificationService.shutdown()); + return mobileNotificationService.shutdown() + .thenCompose(e -> systemNotificationService.shutdown()); }
59-62: Consider independent error handling for each notification channel.If
systemNotificationService.dispatchNotification()throws an exception,mobileNotificationService.dispatchNotification()will not be called. For a more resilient approach, consider catching exceptions from each service independently so that a failure in one channel doesn't prevent the other from executing.♻️ Suggested resilient approach
public void dispatchNotification(Notification notification) { - systemNotificationService.dispatchNotification(notification); - mobileNotificationService.dispatchNotification(notification); + try { + systemNotificationService.dispatchNotification(notification); + } catch (Exception e) { + log.error("Failed to dispatch system notification", e); + } + try { + mobileNotificationService.dispatchNotification(notification); + } catch (Exception e) { + log.error("Failed to dispatch mobile notification", e); + } }
security/src/main/java/bisq/security/mobile_notifications/MobileNotificationEncryption.java
Show resolved
Hide resolved
HenrikJannsen
force-pushed
the
add-push-notifications-service
branch
from
5eed858 to
ca02093
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@bonded-roles/src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java`:
- Line 245: The log in MobileNotificationRelayClient (the log.info call that
logs client.getBaseUrl() and param) is leaking the full request parameter which
contains deviceTokenHex; change it to redact or preview the token like
DeviceRegistrationService's tokenPreview pattern by computing a safe preview
(e.g., first 6–8 chars + "..." or mask all but last 4) from the deviceTokenHex
before logging, and use that preview in the log message instead of the raw param
so the base URL and timing remain logged but the token is not exposed.
In
`@notifications/src/main/java/bisq/notifications/mobile/registration/DeviceRegistrationService.java`:
- Around line 56-60: The info log in DeviceRegistrationService exposes sensitive
device identifiers by logging tokenPreview and publicKeyPreview via log.info;
change this to avoid revealing any part of deviceToken/publicKeyBase64 at INFO
level — either remove the previews and log only non-sensitive metadata (e.g.,
token length and publicKeyBase64 length or a short hash), or keep the existing
preview variables but move the log call to DEBUG. Update the log invocation
(log.info(...)) to instead log safe values (e.g., deviceToken.length(),
publicKeyBase64.length(), or a hashed value) or use log.debug and retain the
original previews only under DEBUG so that tokenPreview/publicKeyPreview are
never emitted at INFO.
🧹 Nitpick comments (2)
bonded-roles/src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java (2)
187-213: Unreachable catch block forRejectedExecutionException.The
try-catchat lines 190-212 cannot catchRejectedExecutionExceptionfromsupplyAsyncbecauseCompletableFuture.supplyAsyncdoesn't throw synchronously—it returns a failed future instead. The exception would propagate through the future chain, not as a synchronous throw.The actual
RejectedExecutionExceptionhandling should be done viaexceptionallyorhandleon the future, or the try-catch should wrap the innersendToRelayServercall at line 230 (which does correctly catch it at line 287).Proposed fix - remove unreachable code
public CompletableFuture<Boolean> sendToRelayServer(boolean isAndroid, String deviceTokenHex, String encryptedMessageHex) { - try { - return sendToRelayServer(isAndroid, - deviceTokenHex, - encryptedMessageHex, - new AtomicInteger(0)) - .exceptionallyCompose(throwable -> { - if (throwable instanceof RetryException retryException) { - return sendToRelayServer(isAndroid, - deviceTokenHex, - encryptedMessageHex, - retryException.getRecursionDepth()); - } else if (ExceptionUtil.getRootCause(throwable) instanceof RetryException retryException) { - return sendToRelayServer(isAndroid, - deviceTokenHex, - encryptedMessageHex, - retryException.getRecursionDepth()); - } else { - return CompletableFuture.failedFuture(throwable); - } - }); - } catch (RejectedExecutionException e) { - return CompletableFuture.failedFuture(new RejectedExecutionException("Too many requests. Try again later.")); - } + return sendToRelayServer(isAndroid, + deviceTokenHex, + encryptedMessageHex, + new AtomicInteger(0)) + .exceptionallyCompose(throwable -> { + if (throwable instanceof RetryException retryException) { + return sendToRelayServer(isAndroid, + deviceTokenHex, + encryptedMessageHex, + retryException.getRecursionDepth()); + } else if (ExceptionUtil.getRootCause(throwable) instanceof RetryException retryException) { + return sendToRelayServer(isAndroid, + deviceTokenHex, + encryptedMessageHex, + retryException.getRecursionDepth()); + } else { + return CompletableFuture.failedFuture(throwable); + } + }); }
323-328: Consider logging suppressed exceptions at debug level.Silently swallowing exceptions makes debugging harder. Consider logging at
debugortracelevel to aid troubleshooting without cluttering logs.Proposed fix
private void shutdownHttpClient(BaseHttpClient client) { try { client.shutdown(); - } catch (Exception ignore) { + } catch (Exception e) { + log.debug("Exception during HTTP client shutdown", e); } }
...src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java
Outdated
Show resolved
Hide resolved
...ications/src/main/java/bisq/notifications/mobile/registration/DeviceRegistrationService.java
Outdated
Show resolved
Hide resolved
HenrikJannsen
force-pushed
the
add-push-notifications-service
branch
from
ca02093 to
25fed66
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@bonded-roles/src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java`:
- Around line 280-286: When the CompletableFuture returned by
completeOnTimeout(...) times out you must add the provider that was attempted to
failedProviders before returning the failed future so the retry logic
(failedProviders.size() < numTotalCandidates) reflects the timeout; modify the
thenCompose block that throws new RetryException("Timeout", recursionDepth) to
first add the current provider/candidate to failedProviders (using the same
provider variable used elsewhere when marking failures) and then return
CompletableFuture.failedFuture(new RetryException("Timeout", recursionDepth)),
ensuring you preserve thread-safety/duplication semantics used by
failedProviders.
- Around line 195-209: The instanceof check against RetryException inside the
exceptionallyCompose lambda is ineffective because async exceptions are wrapped
(e.g., CompletionException); change the handler to unwrap the root cause first
by using ExceptionUtil.getRootCause(throwable) (or otherwise unwrap
CompletionException) and then test that result for RetryException; if it is a
RetryException call sendToRelayServer(isAndroid, deviceTokenHex,
encryptedMessageHex, retryException.getRecursionDepth()), otherwise return
CompletableFuture.failedFuture(throwable). Ensure you only reference
RetryException, ExceptionUtil.getRootCause, and sendToRelayServer in the updated
lambda so the dead direct instanceof branch is removed.
- Around line 315-320: The fillCandidates method currently logs an error when
recursionDepth > 0 but leaves the candidates set empty, which lets
selectNextProvider call getRandomElement on an empty collection and cause an
NPE; change the else branch in fillCandidates to throw an IllegalStateException
(include a clear message referencing recursionDepth and that all providers are
exhausted) instead of only logging, so callers like selectNextProvider fail
fast; keep or log the original error message before throwing to preserve
diagnostics.
In
`@notifications/src/main/java/bisq/notifications/mobile/registration/DeviceRegistrationService.java`:
- Around line 56-57: The current INFO log in DeviceRegistrationService prints
sensitive user-provided identifiers (deviceId and deviceDescriptor); change it
to only log non‑sensitive metadata at INFO (e.g., platform and lengths like
deviceId.length() and deviceDescriptor.length()) and move the raw deviceId and
deviceDescriptor values to a debug log (use log.debug) so they are available
when debugging but not stored at INFO level; update the logging call(s) in the
method where log.info("Registering device - deviceId...") is used to reflect
this.
🧹 Nitpick comments (1)
bonded-roles/src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java (1)
60-68: Consider using primitive int instead of AtomicInteger for recursion depth.The
AtomicIntegeris passed by reference and mutated externally (line 268), which is unconventional for exception-carried state. Since the retry logic is sequential (one retry at a time per request), a simpleintvalue would be clearer and avoid the mutable-reference pattern.
...src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java
Show resolved
Hide resolved
...src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java
Show resolved
Hide resolved
...src/main/java/bisq/bonded_roles/mobile_notification_relay/MobileNotificationRelayClient.java
Show resolved
Hide resolved
...ications/src/main/java/bisq/notifications/mobile/registration/DeviceRegistrationService.java
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This implementation is based on
#4266
but diverges in several important ways.
Notification filtering, routing, and delivery state are already handled by the existing notifications framework (per chat channel and notification settings). Re-implementing that logic here would duplicate complex domain behavior and introduce a high risk of inconsistencies and subtle bugs.
Therefore, this design reuses the existing notification domain and only adds the minimal plumbing required to forward notifications to mobile devices.
Key Design Decisions
No user profile-level filtering
profileIdfor registration would break notifications in multi-profile mode, as devices would only receive notifications for a single profile.➡️ Result:
profileIdwas intentionally removed from device registration.Device identity
deviceIdis the primary identifier for registration and unregistration.It can be:
publicKeyBase64, orThis keeps the API stateless and avoids profile coupling.
REST API
Endpoints
RegisterDeviceRequest fields
deviceIddeviceTokenpublicKeyBase64deviceDescriptorplatformNotification Payload
All desktop system notifications are forwarded to mobile unchanged in structure, serialized as JSON:
Rationale
➡️ Keeping desktop and mobile notifications aligned is a deliberate design goal.
Configuration
Notes
urlmay be:fallbackProvidersare used only if all primary providers failSummary
Summary by CodeRabbit
New Features
Configuration
Refactor
Security
✏️ Tip: You can customize this high-level summary in your review settings.