feat: add signerAddress prop to DELETE subscriptions endpoint #2651

compojoom · 2025-07-16T07:08:07Z

Summary

Resolves https://linear.app/safe-global/issue/COR-336/expose-signeraddress-to-the-delete-subscriptions-endpoint

adds a signerAddress prop to the delete subscriptions endpoint. This allows us to delete just a single signerAddress.

With this change the endpoint behaves as follows

no signer address provided -> drop everything row related to the safeAddress and deviceId
signerAddress provided -> drop only the rows that match the signerAddress and safeAddress and deviceId
signerAddress set to null -> drop only the rows where signerAddress is NULL and SafeAddress and DeviceId match

LucieFaire

looks legit, but left some small comments :)

LucieFaire · 2025-07-16T07:54:31Z

src/domain/notifications/v2/entities/__tests__/delete-all-subscriptions.dto.entity.spec.ts

@@ -203,4 +203,146 @@ describe('DeleteAllSubscriptionsDtoSchema', () => {
      },
    ]);
  });
+
+  it('should validate signerAddress when provided', () => {
+    const deleteAllSubscriptionsDto = {


nitpick: you could use the builder and only provide the signerAddress on demand

LucieFaire · 2025-07-16T07:55:21Z

src/domain/notifications/v2/entities/__tests__/delete-all-subscriptions.dto.entity.spec.ts

+    expect(result.success).toBe(true);
+  });
+
+  it('should validate when signerAddress is not provided', () => {


i think the very first test checks the same, so maybe we dont need it, or should be more explicit in a check

LucieFaire · 2025-07-16T07:56:06Z

src/domain/notifications/v2/entities/__tests__/delete-all-subscriptions.dto.entity.spec.ts

+
+  it('should checksum signerAddress when provided', () => {
+    const nonChecksummedAddress = faker.finance.ethereumAddress().toLowerCase();
+    const deleteAllSubscriptionsDto = {


nitpick: maybe we could use builder everywhere instead of specifying the whole obj?

LucieFaire · 2025-07-16T08:03:48Z

src/domain/notifications/v2/entities/delete-all-subscriptions.dto.entity.ts

@@ -10,6 +10,7 @@ export const DeleteAllSubscriptionsDtoSchema = z.object({
        chainId: NumericStringSchema,
        deviceUuid: UuidSchema,
        safeAddress: AddressSchema,
+        signerAddress: AddressSchema.nullable().optional(),


can be replaced with nullish() - it is an equivalent

LucieFaire · 2025-07-16T08:06:58Z

src/domain/notifications/v2/notifications.repository.integration.spec.ts

+        signer_address: signerAddress,
+      });
+
+      const deleteAllSubscriptionsDto = [


could we please also use the builder in these tests

LucieFaire · 2025-07-16T08:07:58Z

src/domain/notifications/v2/notifications.repository.spec.ts

+
+    it('Should include signerAddress in where conditions when provided', async () => {
+      const signerAddress = getAddress(faker.finance.ethereumAddress());
+      const deleteAllSubscriptionsDto = [


builder ? :)

LucieFaire · 2025-07-16T08:11:55Z

src/domain/notifications/v2/notifications.repository.ts

+        },
+      };
+
+      // Handle signerAddress: undefined (omitted) vs null (explicit) vs string (specific address)


could be simplified:

return subscription.signerAddress === undefined ? baseCondition : { ...baseCondition, signer_address: subscription.signerAddress === null ? IsNull() : subscription.signerAddress, };

And maybe we can leave one comment before describing all cases in one place

Hey @LucieFaire, Thanks for the suggestion, but I'm going to resist it for as long as I can :D
I don't think that nested ternaries make the code simpler. They make it more compact, but sacrifice readibility.

PooyaRaki · 2025-07-16T14:29:33Z

@compojoom I marked it as a draft since the tests are failing. Please feel free to mark it as ready for review once the tests are fixed. thanks

src/domain/notifications/v2/notifications.repository.interface.ts

src/domain/notifications/v2/notifications.repository.integration.spec.ts

PooyaRaki · 2025-07-17T13:28:22Z

src/domain/notifications/v2/notifications.repository.integration.spec.ts

+      );
+
+      // The subscription with signerAddress1 should still exist
+      const remainingSubscriptions =


Nit: We can rename it to otherSubscriptions for more readability, Specially that the previous test also has const remainingSubscriptions and expect it to be zero.

src/domain/notifications/v2/entities/__tests__/delete-all-subscriptions.dto.builder.ts

PooyaRaki · 2025-07-17T13:38:10Z

src/routes/notifications/v2/entities/delete-all-subscriptions.dto.entity.ts

+    type: 'string',
+    required: false,
+    nullable: true,
+    description:


Thanks for the thorough explanation. 👏

feat: add signerAddress prop to DELETE subscriptions endpoint

9da78f1

compojoom requested a review from a team as a code owner July 16, 2025 07:08

LucieFaire reviewed Jul 16, 2025

View reviewed changes

PooyaRaki marked this pull request as draft July 16, 2025 14:28

compojoom added 3 commits July 16, 2025 18:08

fix: address PR feedback

9cc59b5

fix: lint

cf5d7b4

fix: failing tests

9721e37

compojoom marked this pull request as ready for review July 17, 2025 08:00