safe-global / safe-client-gateway

Serves as a bridge for the Safe{Wallet} clients (Android, iOS, Web)
https://docs.safe.global
MIT License
26 stars 64 forks source link

Evaluate integration with safe-react-gateway-sdk #69

Closed fmrsabino closed 2 months ago

fmrsabino commented 2 years ago

We should evaluate the integration with the types used in the safe-react-gateway-sdkhttps://github.com/safe-global/safe-react-gateway-sdk

Ideally it should only contain the types/interfaces that should be consumed by the clients. Potential external dependencies around these types should also be reviewed.

Link: https://github.com/safe-global/safe-react-gateway-sdk

fmrsabino commented 2 years ago

The SDK relies on cross-fetch. For including the SDK we should strive to include just the types that serve as the interface between the client applications and the Safe Client Gateway.

├─┬ @gnosis.pm/safe-react-gateway-sdk@3.4.0
│ └─┬ cross-fetch@3.1.5
│   └─┬ node-fetch@2.6.7
│     ├── encoding@0.1.13 deduped
│     └─┬ whatwg-url@5.0.0
│       ├── tr46@0.0.3
│       └── webidl-conversions@3.0.1

Additionally, we include a JSON schema validation (with AJV) for both requests and responses in the Safe Client Gateway. Since we don't want to push this dependency to the SDK, we would keep the validation of these payloads in the Safe Client Gateway while the types themselves would still serve as the common interface between service and clients.

Moving forward I see 3 different solutions:

  1. We'd have a single package just with the types (extracted from the SDK).
  2. We would not share the types (keep as is).
  3. We'd move the types over to the Safe Client Gateway repo while creating a node package with just the types.

In my opinion, option 3 seems to be the one that would work best for our needs – the advantage is that the publishing of the the types could follow the release schedule of the Safe Client Gateway.

mmv08 commented 2 years ago

Related stackoverflow question: https://stackoverflow.com/questions/54941329/share-interfaces-between-api-and-frontend

It pretty much sums up my previous experience with NestJS. They're especially bang on with decorators

fmrsabino commented 2 years ago

Related stackoverflow question: https://stackoverflow.com/questions/54941329/share-interfaces-between-api-and-frontend

It pretty much sums up my previous experience with NestJS. They're especially bang on with decorators

I'm not sure if I follow the decorators part. It seems that the issue is around typeorm which we don't use. However the folder structure of backend/frontend seems to be similar to point 3 that I mentioned above.

mmv08 commented 2 years ago

I'm not sure if I follow the decorators part. It seems that the issue is around typeorm which we don't use. However the folder structure of backend/frontend seems to be similar to point 3 that I mentioned above.

When I used NestJS, I remember having swagger and serialization-related decorators in DTO/Entities. I don't think the frontend can understand them

fmrsabino commented 2 years ago

When I used NestJS, I remember having swagger and serialization-related decorators in DTO/Entities. I don't think the frontend can understand them

I se what you mean. I will explore alternative solutions to it! 👍 Seems that they offer the following decorators:

https://docs.nestjs.com/openapi/decorators

fmrsabino commented 2 years ago

I'm not sure if I follow the decorators part. It seems that the issue is around typeorm which we don't use. However the folder structure of backend/frontend seems to be similar to point 3 that I mentioned above.

When I used NestJS, I remember having swagger and serialization-related decorators in DTO/Entities. I don't think the frontend can understand them

I will suggest a workaround this issue in https://github.com/5afe/safe-client-gateway-nest/pull/84

I'd like to keep OpenApi annotations separate from the domain entities that we work with. So I've created classes that just implement the domain spec but are annotated with Swagger decorators. See: https://github.com/5afe/safe-client-gateway-nest/blob/4fe838e57db6ac0b64b1ab335c12c1104b1f86c8/src/routes/chains/openapi/api-chain.ts

This can be a bit cumbersome to setup in the beginning but has the following advantages:

mmv08 commented 2 years ago

I'm not sure if I follow the decorators part. It seems that the issue is around typeorm which we don't use. However the folder structure of backend/frontend seems to be similar to point 3 that I mentioned above.

When I used NestJS, I remember having swagger and serialization-related decorators in DTO/Entities. I don't think the frontend can understand them

I will suggest a workaround this issue in #84

I'd like to keep OpenApi annotations separate from the domain entities that we work with. So I've created classes that just implement the domain spec but are annotated with Swagger decorators. See: https://github.com/5afe/safe-client-gateway-nest/blob/4fe838e57db6ac0b64b1ab335c12c1104b1f86c8/src/routes/chains/openapi/api-chain.ts

This can be a bit cumbersome to setup in the beginning but has the following advantages:

  • We keep an API spec separate from the domain (which can be shared with Safe React).
  • While we avoid solutions that generate these for us we actually get more flexibility in how we document the API – eg.: if a generator cannot work around with generics, unions or other complex types then we can manually register the appropriate spec for it.

I like the solution, nice one 👍

iamacook commented 2 months ago

I'm going to close this as we have now have https://github.com/safe-global/safe-client-gateway-sdk.