Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Notifications #2268

Merged
merged 24 commits into from
Jan 24, 2025
Merged

Notifications #2268

merged 24 commits into from
Jan 24, 2025

Conversation

PooyaRaki
Copy link
Contributor

@PooyaRaki PooyaRaki commented Jan 16, 2025 •
edited
Loading

Summary

This PR addresses and fixes multiple bugs related to the Notifications system, improving its reliability.

Changes

  • Adds a new field failed to the multisig event
  • Removes the device and subscriptions on the mobile clients and re inserts them
  • Removes unnecessary migrations
  • Fix signature recovery for mobile app
  • Fix a bug that notification was send to a device per owner keys
  • Fix a bug with delegate keys
  • Adjust unit tests

@PooyaRaki PooyaRaki force-pushed the notificationRecoverSignature branch from c2c24fb to c364d7e Compare January 16, 2025 16:10
@PooyaRaki PooyaRaki force-pushed the notificationRecoverSignature branch 2 times, most recently from 8d1e99c to cbd727d Compare January 20, 2025 11:12
@PooyaRaki
fix notification recover message address on mobile clients
f33cd28
@PooyaRaki
REMOVE ME: Run staging pipeline for branch
bc86234
@PooyaRaki
Register notification subscription for all the signatures in request
1bedfb2
@PooyaRaki
Remove: clean notification data
5a3a9b2
@PooyaRaki
Fix tests
68b23da
@PooyaRaki
Remove: run CI on branch
cdbf10a
@PooyaRaki
Prevent duplicate notifications sent to a device due to multiple sign…
6a96839 
…er keys
@PooyaRaki
Delete all notification subscriptions
a582a6c
@PooyaRaki
remove: enable debug logs
8d98f42
@PooyaRaki
Notification subscription can be registered without any signer address
0645f04
@PooyaRaki
Remove: Debug Logs
467d423
@PooyaRaki
Remove: more debug logs
7cec885
@PooyaRaki
Remove notification hook skip tests
72a1724
@PooyaRaki
Fix IOS push notification Firebase payload
492e7d3
@PooyaRaki
Fix notification deletegate owner
1b77ed9
@PooyaRaki
Validates the executed transaction failed status
c7ffde6
@PooyaRaki
Adjust types
a983e72
@PooyaRaki
Add to to multisig executed notification
1188e85
@PooyaRaki
check confirmation notification signers agaianst delegators and the a…
91178a6 
…ctual owners
@PooyaRaki
Remove debug logs
9f449c3
@PooyaRaki
Remove Me: change ci file
a6f5108
@PooyaRaki PooyaRaki force-pushed the notificationRecoverSignature branch from cbd727d to a6f5108 Compare January 20, 2025 11:42
iamacook
iamacook reviewed Jan 23, 2025
View reviewed changes
migrations/1733761866546-cleanNotificationData.ts Show resolved Hide resolved
src/datasources/push-notifications-api/entities/firebase-notification.entity.ts Show resolved Hide resolved
src/datasources/push-notifications-api/firebase-cloud-messaging-api.service.spec.ts Outdated Show resolved Hide resolved
src/datasources/push-notifications-api/firebase-cloud-messaging-api.service.spec.ts Outdated Show resolved Hide resolved
src/datasources/push-notifications-api/firebase-cloud-messaging-api.service.ts Outdated Show resolved Hide resolved
src/routes/hooks/entities/schemas/executed-transaction.schema.ts Outdated Show resolved Hide resolved
src/routes/notifications/v1/entities/__tests__/create-registration-v2.dto.builder.ts Outdated Show resolved Hide resolved
src/routes/notifications/v1/entities/__tests__/create-registration-v2.dto.builder.ts Outdated Show resolved Hide resolved
src/routes/notifications/v1/notifications.controller.ts Outdated
Comment on lines 146 to 176
const signatureArray = safeV1Registration.signatures.length
? safeV1Registration.signatures
: [undefined];
for (const safeV1Signature of signatureArray) {
if (safeV1Registration.safes.length) {
const safeV2: Parameters<
NotificationsServiceV2['upsertSubscriptions']
>[0] & {
upsertSubscriptionsDto: {
safes: Array<UpsertSubscriptionsSafesDto>;
signature: `0x${string}`;
};
} = {
upsertSubscriptionsDto: {
cloudMessagingToken: args.cloudMessagingToken,
deviceType: args.deviceType,
deviceUuid: (args.uuid as UUID) || undefined,
safes: [],
signature: (safeV1Signature as `0x${string}`) ?? undefined,
},
authPayload: new AuthPayload(),
};
} = {
upsertSubscriptionsDto: {
cloudMessagingToken: args.cloudMessagingToken,
deviceType: args.deviceType,
deviceUuid: (args.uuid as UUID) || undefined,
safes: [],
signature: safeV1Registration.signatures[0] as `0x${string}`,
},
authPayload: new AuthPayload(),
};
const uniqueSafeAddresses = new Set(safeV1Registration.safes);
for (const safeAddresses of uniqueSafeAddresses) {
safeV2.upsertSubscriptionsDto.safes.push({
address: safeAddresses as `0x${string}`,
chainId: safeV1Registration.chainId,
notificationTypes: Object.values(NotificationType),
});
const uniqueSafeAddresses = new Set(safeV1Registration.safes);
for (const safeAddresses of uniqueSafeAddresses) {
safeV2.upsertSubscriptionsDto.safes.push({
address: safeAddresses as `0x${string}`,
chainId: safeV1Registration.chainId,
notificationTypes: Object.values(NotificationType),
});
}
safeV2Array.push(safeV2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per discussion, this is quite hard to follow.

We're not sure why [undefined] is required for signatureArray. I suggest adding some comments to begin with, and reconsidering multiple for loops and/or making this more functional, e.g. using map. (The latter can be done later, and we add a TODO for now.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up: [undefined] is likely required as registrations may have no signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, because the signature can be empty when there is no signer key in the mobile app.

src/routes/notifications/v1/notifications.controller.ts Outdated
Comment on lines 237 to 247
`gnosis-safe${args.registerDeviceDto.timestamp}${args.registerDeviceDto.uuid}${args.registerDeviceDto.cloudMessagingToken}${args.safeAddresses.sort().join('')}`,
),
),
},
signature: args.safeV2Dto.upsertSubscriptionsDto.signature,
});
} else {
return await recoverAddress({
hash: keccak256(
toBytes(
`gnosis-safe${args.registerDeviceDto.timestamp}${args.registerDeviceDto.uuid}${args.registerDeviceDto.cloudMessagingToken}${args.safeAddresses.sort().join('')}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The messages are the same so let's move it to a variable.

As before: we should ensure clients use the same signing method when moving to v2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First part done 25045e4
About the second part: Please create a task for it so that we don't forget about it otherwise it's easy to miss it if we keep it as a review comment.

@PooyaRaki PooyaRaki force-pushed the notificationRecoverSignature branch 3 times, most recently from 2a99e3b to 51b896f Compare January 23, 2025 17:15
@PooyaRaki PooyaRaki requested a review from iamacook January 23, 2025 17:23
@PooyaRaki PooyaRaki marked this pull request as ready for review January 24, 2025 08:53
@PooyaRaki PooyaRaki requested a review from a team as a code owner January 24, 2025 08:53
@PooyaRaki PooyaRaki force-pushed the notificationRecoverSignature branch 3 times, most recently from 90c854c to c25e69a Compare January 24, 2025 10:02
@PooyaRaki PooyaRaki force-pushed the notificationRecoverSignature branch from c25e69a to 3dfb79d Compare January 24, 2025 11:00
@PooyaRaki PooyaRaki force-pushed the notificationRecoverSignature branch from 3dfb79d to 92675a4 Compare January 24, 2025 11:01
@PooyaRaki PooyaRaki enabled auto-merge (squash) January 24, 2025 11:04
@PooyaRaki PooyaRaki merged commit 34e618c into main Jan 24, 2025
18 checks passed
@PooyaRaki PooyaRaki deleted the notificationRecoverSignature branch January 24, 2025 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamacook iamacook iamacook approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PooyaRaki @iamacook