Skip to content
This repository was archived by the owner on Jun 24, 2022. It is now read-only.

[Paraswap] migration to v5 #1433

Merged
merged 6 commits into from
Oct 5, 2021
Merged

[Paraswap] migration to v5 #1433

merged 6 commits into from
Oct 5, 2021

Conversation

nenadV91
Copy link
Contributor

@nenadV91 nenadV91 commented Sep 13, 2021

Summary

Fixes #1378

  • migration from Paraswap v4 to v5

    To Test

  1. Go to Cowswap
  2. Test different swap combinations to check if there is anything unusual in the prices related to the current production version or any other issues related to prices

Note

  • update solver address in the code

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@nenadV91 nenadV91 marked this pull request as ready for review September 17, 2021 11:06
@nenadV91 nenadV91 requested review from a team September 17, 2021 11:06
@fairlighteth
Copy link
Contributor

Did some brief testing and for those few tests, it seems to work great. Some points to note:

  • I'm getting a bit lower Fees on this PR, which is great.
  • On this swap the decimal seemed truncated on the fiat display for the input amount:
  • (LEFT is this PR <---> RIGHT is production)

Screen Shot 2021-09-20 at 10 35 18

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.

LGTM

Only thing that I noticed is that sometimes theres a 533 response from paraswap
screenshot_2021-09-20_10-12-31

@alfetopito
Copy link
Contributor

On this swap the decimal seemed truncated on the fiat display for the input amount:

That's probably just rounding as I believe the same display logic is on prod and dev

@fairlighteth
Copy link
Contributor

That's probably just rounding as I believe the same display logic is on prod and dev

Hmm I think the screenshots I shared aren't the right ones 🤔 . I was referring to seeing a truncated fiat value for the input field, looking something like '$342324.'

I guess it looked weird to me seeing a number truncated ending with a dot/decimal. But can't reproduce for now...

@elena-zh
Copy link

was referring to seeing a truncated fiat value for the input field, looking something like '$342324.'

this might be reported in #1316

@MareenG
Copy link

MareenG commented Sep 21, 2021

LGTM!
This affects only mainnet right, as paraswap doesn't operate on xDai or Rinkeby?

@MareenG
Copy link

MareenG commented Sep 21, 2021

Only the issues reported before( #1426 #1427 ) are present in this PR

@alfetopito
Copy link
Contributor

was referring to seeing a truncated fiat value for the input field, looking something like '$342324.'

this might be reported in #1316

Yes @elena-zh that's it.

@biocom I think the issue is that the formating fn reduces the precision for higher values. Need to work on that eventually.

Copy link
Contributor

@W3stside W3stside left a comment

Choose a reason for hiding this comment

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

nice!

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.

👍x2

@anxolin anxolin self-requested a review October 4, 2021 10:47
@anxolin
Copy link
Contributor

anxolin commented Oct 4, 2021

Are you guys getting sometimes:
image

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Code looks great, works as expected. Only concern is their API failing some times...

@nenadV91
Copy link
Contributor Author

nenadV91 commented Oct 4, 2021

Yes, it was mentioned before https://gnosisinc.slack.com/archives/C025G521XQD/p1632209701016600. Could be that our code sometimes produces more then one request to Paraswap almost instantly or in the time span of few seconds and they now limit some of those but I'm not sure about this.

@alfetopito alfetopito added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Oct 4, 2021
@mergify mergify bot merged commit 15fc3dd into develop Oct 5, 2021
@alfetopito alfetopito deleted the 1378/migrate-to-paraswap-v5 branch October 5, 2021 19:20
anxolin added a commit that referenced this pull request Oct 11, 2021
anxolin added a commit that referenced this pull request Oct 11, 2021
This was referenced Oct 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to Paraswap API v5
8 participants