-
Notifications
You must be signed in to change notification settings - Fork 54
Use modified paraswap package version #2493
Use modified paraswap package version #2493
Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated PR, but i see also a lot of ethersjs code that doesn't get tree shaken.
Maybe this help:
https://twitter.com/DawsonBotsford/status/1394048094338129922?s=20&t=8MXUOAJOdaijh_uX0cohog
The proposed NPM package, is nice (if it works), although i wonder if u could make a version that Paraswap would be willing to merge.
Maybe one solution is to:
- Not delete the web3 dependency, mark it as peer or optional (not sure)
- import only the specific things that are required
- I think they probably shouldn't instanciate the web3 object there, make it optional
Other approach, if the previous is not easily done, is to see what do we need from them. Cause it feels their library adds too many dependencies.
I'll raise also this to them to see if they know how to get around it, but loading all lodash, bignumber and axios seems like too much for just doing a simple request
@@ -150,6 +150,9 @@ | |||
"use-count-up": "^2.2.5", | |||
"wcag-contrast": "^3.0.0", | |||
"web-vitals": "^2.1.0", | |||
"web3": "^1.7.0", | |||
"web3-providers-http": "^1.7.0", | |||
"web3-providers-ws": "^1.7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to add web3?
I think the whole point is that we don't need it, because we use ethersjs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the dex-js needs it, otherwise the build doesn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, because is peer dependency... okok
maybe should be optional in dex-js (since we don't use it), but fine
@nenadV91 I've open this discussion, i think Paraswap library should have less dependencies: https://gnosisinc.slack.com/archives/C0203Q9R4JJ/p1645723122926249 |
Hey @nenadV91 , I always get 403 error response on each paraswap request in this PR However, I get 403 error on Prod as well. Weird. Nevertheless, I have not noticed any issues in price fetching in this PR. |
@elena-zh For me Paraswap requests work fine both in this PR and on prod. |
@nenadV91 , seems that an access to paraswap.io is totally blocked for me |
@@ -225,7 +228,7 @@ | |||
"fast-safe-stringify": "^2.0.8", | |||
"firebase": "^9.1.3", | |||
"ipfs-http-client": "^52.0.3", | |||
"paraswap": "^5.0.1", | |||
"paraswap": "npm:@nenad91/paraswap#5.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using VPN, I was able to test the changes. LGTM!
Summary
Fixes #2449
Here are the changes I made in Paraswap package https://github.com/paraswap/paraswap-sdk/pull/109/files
I published this as a npm module with my account
The result is reduction of total bundle from All (6.28 MB) to All (4.93 MB)
Before

After

To test