Upgrade to new notifications endpoint #6190

jinchung · 2024-10-10T20:08:02Z

Fixes APP-887

What changed (plus any additional context for devs)

Adds support for the new notifications endpoint (REST PUT) which adds support for select L2 notifications
Removes the old explorer redux (which used websockets) which is no longer needed
Moves support for global notification topics (e.g, points) to the new endpoint as well
Migrates existing users to the new endpoint

Please note: APP-985 is still a known issue

Screen recordings / screenshots

RPReplay_Final1728594734.MP4

What to test

https://www.notion.so/rainbowdotme/Notifications-upgrade-642eae49b0eb45b2a65cd89c367fed00#27a7325fd6894d9d9de645300fcdae2f

linear · 2024-10-10T20:15:22Z

APP-887 Migrate to new notifications subscription system

jinchung · 2024-10-10T20:20:58Z

src/hooks/index.ts

@@ -48,7 +48,6 @@ export { default as useGas } from './useGas';
 export { default as useHeight } from './useHeight';
 export { default as useHideSplashScreen } from './useHideSplashScreen';
 export { default as useImageMetadata } from './useImageMetadata';
-export { default as useInitializeAccountData } from './useInitializeAccountData';


related to removal of redux explorer

jinchung · 2024-10-10T20:21:59Z

src/migrations/migrations/migrateNotificationSettingsToV3.ts

+      if (walletSettings.length) {
+        const newSettings = walletSettings.map(wallet => ({
+          ...wallet,
+          successfullyFinishedInitialSubscription: false,


main change: reset the successfullyFinishedInitialSubscription flag so we know to resubscribe using the new endpoint (after it successfully resubscribes, it'll get set back to true)

jinchung · 2024-10-10T20:22:45Z

src/notifications/NotificationsHandler.tsx

@@ -255,8 +252,7 @@ export const NotificationsHandler = ({ walletReady }: Props) => {
            })
        )
      );
-      initializeGlobalNotificationSettings();
-      initializeNotificationSettingsForAllAddressesAndCleanupSettingsForRemovedWallets(addresses);
+      initializeNotificationSettingsForAllAddresses(addresses);


initializeNotificationSettingsForAllAddresses will now handle the global notification initialization as well since it's all put together in one payload to the backend

jinchung · 2024-10-10T20:50:44Z

src/notifications/analytics.ts

-) => {
-  analytics.track('Changed Notification Settings', {
-    chainId,
+export const trackChangedGlobalNotificationSettings = (topic: GlobalNotificationTopicType, enableTopic: boolean) => {


renamed this since this was only used for the global notif changes

jinchung · 2024-10-10T20:52:57Z

src/notifications/settings/hooks.ts

-  useEffect(() => () => {
-    listener.remove();
-  });
+  useEffect(() => {


needed to add this into the useEffect because of the async nature of listener changes, which was causing the returned walletNotificationSettings to not necessarily be in sync with the results after a setWalletNotificationSettings

jinchung · 2024-10-10T20:55:03Z

src/notifications/settings/initialization.ts


-  const queue = _prepareSubscriptionQueueAndCreateInitialSettings(addresses, initializationState);
+const createInitialSettingsForAllAddresses = (addresses: AddressWithRelationship[]) => {


this function uses needsUpdate flag so that we only resubscribe when we need to (e.g, if successfullyFinishedInitialSubscription is set to false or if a wallet address that we have is missing from the existing notification settings)

brunobar79 · 2024-10-10T22:04:11Z

Launch in simulator or device for d8ae563

walmat

Mostly looks good. Main concerns:

comparisons between addresses should be toLowerCase() just in case they differ in case values only
JSON.parse erratic behavior when not what we expect
creating a shared util for checking status === RESULTS.GRANTED || status === RESULTS.LIMITED since it's used in many places
shortcutting the need for getFCMToken after we know we have just saved it by returning the token value from saveFCMToken

src/hooks/useResetAccountState.ts

src/migrations/migrations/migrateNotificationSettingsToV3.ts

src/notifications/settings/firebase.ts

walmat · 2024-10-11T23:33:53Z

src/notifications/settings/hooks.ts

-  useEffect(() => () => {
-    listener.remove();
-  });
+  useEffect(() => {


src/notifications/permissions.ts

src/notifications/settings/settings.ts

src/screens/NotificationsPromoSheet/index.tsx

src/screens/SettingsSheet/components/NotificationsSection.tsx

derHowie

Great stuff

I followed your testing instructions here: https://www.notion.so/rainbowdotme/Notifications-upgrade-642eae49b0eb45b2a65cd89c367fed00#27a7325fd6894d9d9de645300fcdae2f

Everything behaved as expected except for one step. In the 'Toggling / changing the notifications settings in the app' section I saw an issue when toggling my group of watched wallets the first time. I saw a spinner for a while, then eventually received an error message indicating that the request had failed. However, it appeared later that the watched wallets had had their notification setting enabled.

My guess is that your PUT timed out but eventually we picked up the change. Would that make sense?

src/notifications/settings/settings.ts

jinchung · 2024-10-16T16:02:04Z

src/hooks/useInitializeWallet.ts

@@ -61,8 +59,8 @@ export default function useInitializeWallet() {
      try {
        PerformanceTracking.startMeasuring(PerformanceMetrics.useInitializeWallet);
        logger.debug('[useInitializeWallet]: Start wallet setup');
-        await resetAccountState();
-        logger.debug('[useInitializeWallet]: resetAccountState ran ok');
+        dispatch(requestsResetState());


the resetAccountState function only has the WC v1 requests resetting logic left - so using this function directly instead

jinchung · 2024-10-16T16:03:47Z

src/screens/SettingsSheet/components/NetworkSection.tsx

  const dispatch = useDispatch();

  const onNetworkChange = useCallback(
    async (chainId: ChainId) => {
-      await resetAccountState();


resetAccountState only has the resetting of WC v1 requests now - given that this doesn't change based on accountSetting's network anymore since we don't support testnets at the moment, and given that we will be removing WC v1, I am preemptively removing this here

brunobar79 · 2024-10-16T17:41:32Z

Launch in simulator or device for fa8a228

brunobar79 · 2024-10-16T19:15:16Z

Launch in simulator or device for bc2d20e

derHowie · 2024-10-16T21:29:11Z

not rerequesting, but the newest changes lgtm ✅

Includes retries if the current FCM token is out of date.

initialization Previously, we had to do firebase subscriptions one at a time. With the new API, we need to provide the full state of enabled notifications.

…fication settings for a wallet

…match NotificationsSection in its usage

… simpler unsubscribe logic

…d handle marketing topics

…change listeners

…setting in it; use this reset function directly instead

…ssion is GRANTED or LIMITED

…ingsToBeUpdated

…ions settings functions

jinchung · 2024-10-17T16:09:04Z

merged early as all tests had passed except for lint, and now lint passed in the final run.

brunobar79 · 2024-10-17T16:44:38Z

Launch in simulator or device for 703607d

sentry-io · 2024-10-18T10:23:30Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ Error: [notifications]: Error while getting and saving FCM token Promise$argument_0(src/notifications/permissions) View Issue
‼️ Error: [wallet]: Error generating account for keychain consoleTransport(src/logger/index) View Issue
‼️ Error: [wallet]: Error in getPrivateKey consoleTransport(src/logger/index) View Issue
‼️ Error: KC unavailable for PKEY lookup consoleTransport(src/logger/index) View Issue
‼️ Error: [wallet]: Error in getPrivateKey consoleTransport(src/logger/index) View Issue

_{Did you find this useful? React with a 👍 or 👎}

jinchung changed the title ~~@jin/notifs 5 global~~ Upgrade to new notifications endpoint Oct 10, 2024

jinchung requested review from walmat and derHowie October 10, 2024 20:20

jinchung marked this pull request as ready for review October 10, 2024 20:20

jinchung commented Oct 10, 2024

View reviewed changes

walmat requested changes Oct 15, 2024

View reviewed changes

derHowie approved these changes Oct 15, 2024

View reviewed changes

src/notifications/settings/settings.ts Outdated Show resolved Hide resolved

jinchung commented Oct 16, 2024

View reviewed changes

jinchung requested a review from walmat October 16, 2024 17:42

walmat approved these changes Oct 16, 2024

View reviewed changes

jinchung added 9 commits October 17, 2024 11:22

Add support for notifications param in backend network settings

84e6ca4

Add support for new notification subscription endpoint

857ad13

Includes retries if the current FCM token is out of date.

Add new publishAndSaveWalletSettings function in notif settings

9adfdae

Remove the concept of a subscription queue in notifications

94ecd09

initialization Previously, we had to do firebase subscriptions one at a time. With the new API, we need to provide the full state of enabled notifications.

Remove unused subscribeWalletToAllEnabledTopic

51bd370

Update function to use publishAndSaveWalletSettings for removing noti…

d9cbbe1

…fication settings for a wallet

Use publishAndSaveWalletSettings function in toggle group notifications

a7915df

Remove unused unsubscribeWalletFromAllNotificationTopics function

5706bac

Remove unused single topic subscribe and unsubscribe functions

6eb929f

jinchung added 23 commits October 17, 2024 11:22

Update topic state after toggling for entire wallet

97421f2

Remove use of oldType in notification settings

1a198cd

Remove websocket subscription for notifications scope in redux explorer

8983846

Remove redux explorer

f977bb3

Remove unnecessary useInitializeAccountData hook

8833e98

Create migration to reset initially subscribed state for notifications

700a819

Update notifs initialization handling

2d86a7d

Add function to subscribe existing notification settings

b31562f

Update request permissions function and have NotificationsPromoSheet …

87cd1ca

…match NotificationsSection in its usage

Remove global notif removal from DevSection - will be replaced with a…

9f61857

… simpler unsubscribe logic

Update publish wallet notifications function params to use objects an…

a1de67d

…d handle marketing topics

Remove redundant initialization of global notification settings

6312b5c

Subscribe existing notifications only if not empty

d1d98bd

Update analytics tracking for global notifs toggle

fc98658

Cleanup: streamline unsubscribe of all notifs from DevSection

e472328

Fix: update topics view when enabling watched wallet notifs

f734d53

Fix: wrap mmkv setting state in useEffect bc of async nature of mmkv …

244b35d

…change listeners

Remove useResetAccountState hook which now only has WC v1 requests re…

83cf842

…setting in it; use this reset function directly instead

Update saveFCMToken to return FCM token

fd9c087

Create a separate utility function for checking if notification permi…

3560fc3

…ssion is GRANTED or LIMITED

Rename toggleGroupNotifications arg wallets to walletNotificationSett…

320ae80

…ingsToBeUpdated

Include toLowerCase check for wallet address comparisons in notificat…

8ef7756

…ions settings functions

Prettier formatting

a61a1a5

jinchung force-pushed the @jin/notifs-5-global branch from 604262a to a61a1a5 Compare October 17, 2024 15:22

Fix type rename

2c3f7de

jinchung merged commit 2518dd8 into develop Oct 17, 2024
8 checks passed

jinchung deleted the @jin/notifs-5-global branch October 17, 2024 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to new notifications endpoint #6190

Upgrade to new notifications endpoint #6190

jinchung commented Oct 10, 2024 •

edited

Loading

linear bot commented Oct 10, 2024

jinchung Oct 10, 2024

jinchung Oct 10, 2024

jinchung Oct 10, 2024

jinchung Oct 10, 2024

jinchung Oct 10, 2024

walmat Oct 11, 2024

jinchung Oct 10, 2024

brunobar79 commented Oct 10, 2024

walmat left a comment

walmat Oct 11, 2024

derHowie left a comment •

edited

Loading

jinchung Oct 16, 2024

jinchung Oct 16, 2024

brunobar79 commented Oct 16, 2024

brunobar79 commented Oct 16, 2024

derHowie commented Oct 16, 2024

jinchung commented Oct 17, 2024

brunobar79 commented Oct 17, 2024

sentry-io bot commented Oct 18, 2024 •

edited

Loading


		const queue = _prepareSubscriptionQueueAndCreateInitialSettings(addresses, initializationState);
		const createInitialSettingsForAllAddresses = (addresses: AddressWithRelationship[]) => {

Upgrade to new notifications endpoint #6190

Upgrade to new notifications endpoint #6190

Conversation

jinchung commented Oct 10, 2024 • edited Loading

What changed (plus any additional context for devs)

Screen recordings / screenshots

What to test

linear bot commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunobar79 commented Oct 10, 2024

walmat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derHowie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunobar79 commented Oct 16, 2024

brunobar79 commented Oct 16, 2024

derHowie commented Oct 16, 2024

jinchung commented Oct 17, 2024

brunobar79 commented Oct 17, 2024

sentry-io bot commented Oct 18, 2024 • edited Loading

Suspect Issues

jinchung commented Oct 10, 2024 •

edited

Loading

derHowie left a comment •

edited

Loading

sentry-io bot commented Oct 18, 2024 •

edited

Loading