Add `/v2/owners/:ownerAddress/safes` to gracefully handle errors #2266

iamacook · 2025-01-16T10:31:34Z

Summary

When getting all Safes by owner (/v1/owners/:ownerAddress/safes), every Transaction Service is called. Should one or more throw, the request will as well.

This adds a new endpoint (/v2/owners/:ownerAddress/safes) which returns null for every chain which the Transaction Service threw for.

Changes

Add new scaffolding for v2 endpoint, deprecating previous.
Wrap Transaction Service requests in Promise.allSettled.
Construct all Safe lists from successful requests, logging for errorneous ones.
Add appropriate test coverage.

katspaugh

Nice. It's good that it still logs when it fails for a given chain.

src/domain/safe/safe.repository.ts

PooyaRaki · 2025-01-20T06:02:45Z

I believe we should either return all of a user’s Safes or none at all. As a user, if some of my Safes are missing from a request, I would assume that all have been returned and the missing ones are lost.
The relevance of the above comment depends on the specific use case of the endpoint.

katspaugh · 2025-01-20T06:56:21Z

@PooyaRaki I think the proposed change is good for two reasons:

In case of an accidental misconfiguration of one of the chains, the other chains will still work
Isolating configs per chain will also help isolating issues – when the whole endpoint is down, you have to to triage from scratch

PooyaRaki · 2025-01-20T07:20:45Z

@katspaugh I agree with you, but I think we should inform users when we’re unable to fetch data from a specific chain due to ongoing issues. Imagine a power user with assets on three different chains—if they see assets on two chains but not the third, they’d likely be confused. If we give them a heads-up about the issue and suggest trying again later, that’s fine. Otherwise, it could be misleading and might hurt their trust in the platform.

iamacook · 2025-01-20T08:22:23Z

@PooyaRaki, I agree it's important to prioritise clarity for users. However, this approach aligns with the existing behaviour of endpoints that fetch data from every Transaction Service.

Imagine a power user with assets on three different chains—if they see assets on two chains but not the third, they’d likely be confused. If we give them a heads-up about the issue and suggest trying again later, that’s fine. Otherwise, it could be misleading and might hurt their trust in the platform.

As we're fetching data from every chain, if a particular Transaction Service throws, we lack clarity as to whether the user has Safes on that chain, so it's potentially confusing either way.

One alternative could be to return null for the affected chain(s) and explicitly display a warning that Safes couldn't be fetched there.

What are your thoughts? cc @katspaugh

PooyaRaki · 2025-01-20T08:32:09Z

One alternative could be to return null for the affected chain(s) and explicitly display a warning that Safes couldn't be fetched there.

@iamacook

Yes, that’s what I was saying—we should display a warning indicating that Safes couldn’t be loaded there. Regarding returning null, what do we return if the user doesn’t have a Safe on a chain? The client needs to differentiate between an error and the absence of a Safe on a chain to display the appropriate error message.
Additionally, we need to ensure that the client properly handles this case before deploying this PR to production.

iamacook · 2025-01-20T08:36:25Z

Regarding returning null, what do we return if the user doesn’t have a Safe on a chain?

An empty array indicates no Safes are owned on a given chain when a request is successful. This PR, however, excludes the chain in question if the request fails.

I'd like to emphasise that returning null would strictly indicate that a request failed, leaving us uncertain whether Safes are (or aren't) owned on the affected chain.

we should display a warning indicating that Safes couldn’t be loaded there. Regarding returning null, what do we return if the user doesn’t have a Safe on a chain

Let's see what @katspaugh thinks about this, as implementing such changes would require client updates, whereas this PR is intended as a quick fix.

katspaugh · 2025-01-20T11:28:25Z

An empty array if no safes or null if a chain threw sounds good. That'd be a breaking change so we'll have to adjust the frontend.

PooyaRaki · 2025-01-20T11:35:48Z

I’d be happy to approve it, provided we ensure that our users are properly notified about the error. As an end user, I wouldn’t want to see my assets disappear simply because certain chains are experiencing issues.

iamacook · 2025-01-20T12:07:14Z

src/routes/owners/owners.controller.ts

+  @ApiOkResponse({
+    schema: {
+      type: 'object',
+      additionalProperties: {
+        type: 'array',
+        items: { type: 'string' },
+      },
+    },
+  })


The types were previously incorrect.

This cannot be set in a class as the dynamic properties are top-level.

iamacook · 2025-01-20T12:07:35Z

src/routes/owners/owners.controller.spec.ts

@@ -453,4 +457,327 @@ describe('Owners Controller (Unit)', () => {
        .expect({ statusCode: 502, message: 'Bad gateway' });
    });
  });
+
+  describe('GET all safes by owner address', () => {


Apart from the graceful handling case, these duplicate the above for the v2 endpoint.

iamacook · 2025-01-20T12:09:24Z

An empty array if no safes or null if a chain threw sounds good. That'd be a breaking change so we'll have to adjust the frontend.

I've added a v2 endpoint implementing the above. Please lmk what you think, @PooyaRaki @katspaugh

src/domain/safe/safe.repository.ts

katspaugh

Thanks!

PooyaRaki · 2025-01-20T13:41:38Z

src/routes/owners/owners.controller.ts

+      },
+    },
+  })
+  @Get('/v2/owners/:ownerAddress/safes')


The v2 endpoint should be stored in another controller, ideally the folder structure would look like:
src/routes/owners/v1/owners.controller.ts
src/routes/owners/v2/owners.controller.ts

But for now, if you think restructuring the whole component is outside the scope of this PR we could just create a new v2 file in the same directory:
src/routes/owners/owners.controller.v1.ts
src/routes/owners/owners.controller.v2.ts

The ideal solution is the first option, but I’m also satisfied with the second.

As per discussion, this was adjusted in 9121170:

There are now versioned controllers: OwnersControllerV1 and OwnersControllerV2 (owners.controller.v*.ts). As the service/repository methods are controller agnostic, however, they retain the deprecated__ prefix, with JSDoc markers.

iamacook · 2025-01-20T14:47:12Z

Do you think we should copy this behaviour to other endpoints that query all Transaction Services?

Gracefully handle errors when getting all Safes by owner

2a0ed4b

iamacook self-assigned this Jan 16, 2025

iamacook requested a review from a team as a code owner January 16, 2025 10:31

katspaugh approved these changes Jan 16, 2025

View reviewed changes

PooyaRaki reviewed Jan 20, 2025

View reviewed changes

src/domain/safe/safe.repository.ts Outdated Show resolved Hide resolved

Remove comment

602a361

Add new endpoint that returns null

43c6424

iamacook commented Jan 20, 2025

View reviewed changes

iamacook changed the title ~~Gracefully handle errors when getting all Safes by owner~~ Add /v2/owners/:ownerAddress/safes to gracefully handle errors Jan 20, 2025

iamacook changed the title ~~Add /v2/owners/:ownerAddress/safes to gracefully handle errors~~ Add /v2/chains/:chainId/owners/:ownerAddress/safes to gracefully handle errors Jan 20, 2025

iamacook changed the title ~~Add /v2/chains/:chainId/owners/:ownerAddress/safes to gracefully handle errors~~ Add /v2/owners/:ownerAddress/safes to gracefully handle errors Jan 20, 2025

Fix E2E test

6055f72

katspaugh reviewed Jan 20, 2025

View reviewed changes

src/domain/safe/safe.repository.ts Outdated Show resolved Hide resolved

Only check success once

2e27da0

katspaugh approved these changes Jan 20, 2025

View reviewed changes

PooyaRaki reviewed Jan 20, 2025

View reviewed changes

Split controller

9121170

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `/v2/owners/:ownerAddress/safes` to gracefully handle errors #2266

Add `/v2/owners/:ownerAddress/safes` to gracefully handle errors #2266

iamacook commented Jan 16, 2025 •

edited

Loading

katspaugh left a comment

PooyaRaki commented Jan 20, 2025

katspaugh commented Jan 20, 2025

PooyaRaki commented Jan 20, 2025

iamacook commented Jan 20, 2025

PooyaRaki commented Jan 20, 2025

iamacook commented Jan 20, 2025

katspaugh commented Jan 20, 2025

PooyaRaki commented Jan 20, 2025

iamacook Jan 20, 2025

iamacook Jan 20, 2025

iamacook commented Jan 20, 2025

katspaugh left a comment

PooyaRaki Jan 20, 2025

iamacook Jan 20, 2025

iamacook commented Jan 20, 2025

Add /v2/owners/:ownerAddress/safes to gracefully handle errors #2266

Are you sure you want to change the base?

Add /v2/owners/:ownerAddress/safes to gracefully handle errors #2266

Conversation

iamacook commented Jan 16, 2025 • edited Loading

Summary

Changes

katspaugh left a comment

Choose a reason for hiding this comment

PooyaRaki commented Jan 20, 2025

katspaugh commented Jan 20, 2025

PooyaRaki commented Jan 20, 2025

iamacook commented Jan 20, 2025

PooyaRaki commented Jan 20, 2025

iamacook commented Jan 20, 2025

katspaugh commented Jan 20, 2025

PooyaRaki commented Jan 20, 2025

iamacook Jan 20, 2025

Choose a reason for hiding this comment

iamacook Jan 20, 2025

Choose a reason for hiding this comment

iamacook commented Jan 20, 2025

katspaugh left a comment

Choose a reason for hiding this comment

PooyaRaki Jan 20, 2025

Choose a reason for hiding this comment

iamacook Jan 20, 2025

Choose a reason for hiding this comment

iamacook commented Jan 20, 2025

Add `/v2/owners/:ownerAddress/safes` to gracefully handle errors #2266

Add `/v2/owners/:ownerAddress/safes` to gracefully handle errors #2266

iamacook commented Jan 16, 2025 •

edited

Loading