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

[iamacook]

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.

[PooyaRaki]

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]

@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]

@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]

@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]

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]

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]

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]

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]

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

[katspaugh]

Thanks!

[iamacook]

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

[PooyaRaki]

Thanks for patiently addressing all the remarks 💯

[PooyaRaki]

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

This looks like a solid refactor. We should create a task for each part and plan to tackle them.