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

[SDK dogfooding] Refactoring, split API's, new assembling, Swagger #97

Merged
merged 49 commits into from
Mar 9, 2023
Merged

[SDK dogfooding] Refactoring, split API's, new assembling, Swagger #97

merged 49 commits into from
Mar 9, 2023

Conversation

shoom3301
Copy link
Contributor

@shoom3301 shoom3301 commented Feb 14, 2023 •
edited
Loading

Docs: https://www.notion.so/cownation/FE-SDK-Dogfooding-our-own-SDK-in-CoW-Swap-ce502a2dfb3f423bb9c7998de4237575
Swagger: cowprotocol/services#1208
Explorer dogfooding: cowprotocol/explorer#352
CowSwap dogfooding: cowprotocol/cowswap#2071

Changes

  1. Separated Metadata, Subgraph and Order book APIs
  2. Order book API is based on autogenerated typescript code from Swagger
  3. Deleted Context, now each API constructor receives only environment: prod | staging
  4. Deleted ECDSA related methods, because we already have it in @cowprotocol/contracts package
  5. Deleted 0x and Paraswap APIs because they are irrelevant

Bundle

image

image

@socket-security
Copy link

socket-security bot commented Feb 14, 2023 •
edited
Loading

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Powered by socket.dev

@github-actions
Copy link

github-actions bot commented Feb 14, 2023 •
edited
Loading

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 1 N/A
  • Result: ✅ success
  • Annotations: 0 total
Report generated by eslint-plus-action

@shoom3301 shoom3301 changed the title [SDK refactoring] New architecture for split API's, webpack [SDK refactoring #0] New architecture for split API's, webpack Feb 14, 2023
@shoom3301 shoom3301 requested a review from a team February 14, 2023 11:36
alfetopito
alfetopito reviewed Feb 14, 2023
View reviewed changes
anxolin
anxolin approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@anxolin anxolin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

SOFT APPROVE! This looks like a great PR!

As small criticism, still mixes a bunch of unrelated things, but I review and I'm mostly good. And I see you did a lot of PR in an attempt to make them easier to review. I appreciated that! 😃

The only things I would ask is:

  • Diagrams: I really like we are doing documentation, but Im reluctant to use Plant UML. Let me do a counter-proposal. Also, maybe the diagram is not 100% valid UML. Lastly, it would be nice to be easily part of the documentation and this is generated automatically. I have a plan for this, let me get back at you later. (You can merge though, my counter-proposal could come on top)
  • Before Merge: Please, let's make sure we test the consumption of this library in different environments. Pure commonJS nodeJS app, CRA website using ES6 imports, etc (in the past we had issues, so i want to make sure we don't shoot ourselves on the foot from changing a library).

This was referenced Feb 28, 2023
Merged
Merged
Merged
Merged
alfetopito
alfetopito reviewed Feb 28, 2023
View reviewed changes
alfetopito
alfetopito approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Very nice, thanks for putting up with my nitpicks so far

Talking about that, a few more comments. Nothing major.

@alfetopito
Copy link
Contributor

Also, unittests are failing

Actually, it's the linter
image

shoom3301 added a commit to cowprotocol/services that referenced this pull request Mar 6, 2023
@shoom3301
Fix orderbook openapi scheme (#1208)
e864bf7 
Fixes cowprotocol/cowswap#2045

To avoid code duplication and have one source of truth we decided to
generate typescript code based on Swagger scheme:
cowprotocol/cow-sdk#97

This PR levels out contradictions between the openapi scheme and the
implementation.
@shoom3301 shoom3301 mentioned this pull request Mar 7, 2023
Merged
@shoom3301 shoom3301 merged commit 2e5cc9d into main Mar 9, 2023
@alfetopito alfetopito deleted the refactoring/dogfooding branch March 21, 2023 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alfetopito alfetopito alfetopito approved these changes

@anxolin anxolin anxolin approved these changes

Assignees

@shoom3301 shoom3301

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shoom3301 @coveralls @alfetopito @anxolin