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.

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

router-sdk: Breaking changes introduced to Trade object constructor with version 1.10.0 #80

Closed
beder-bourahmah-gluwa opened this issue Aug 26, 2024 · 3 comments · Fixed by #82

Comments

@beder-bourahmah-gluwa
Copy link

Version 1.10.0 introduced the v4Routes array as a required property on the Trade object's constructor here.

Previously, you could use the constructor as follows:

new Trade({ v2Routes: [], v3Routes: v3RoutesGoHere, mixedRoutes: [], tradeType: tradeTypeGoesHere })

However, this does not work with version 1.10.0 and can cause runtime errors. In our case, the package.json dependency was set as follows:

"@uniswap/router-sdk": "^1.9.2"

This allows for 1.10.0 to be used instead of 1.9.2 because it assumes compliance with semantic versioning, i.e. no breaking changes for a minor revision. However, if it does use 1.10.0 and the constructor is used as shown above to work with 1.9.2, you will see the following error:

TypeError: undefined is not iterable (cannot read property Symbol(Symbol.iterator))
    at _createForOfIteratorHelperLoose4 (chunk-ANO3WF4I.js?v=e35eaaa1:7757:50)
    at new Trade5 (chunk-ANO3WF4I.js?v=e35eaaa1:8363:31)

Which points at this line.

It is easy enough to fix this issue as a consumer of this library by updating package.json to lock on 1.10.0 and pass an empty array for the new property, but this is not ideal.

@ewilz
Copy link
Member

ewilz commented Aug 28, 2024

Thanks @beder-bourahmah-gluwa. Thinking of patching this in this PR. Of course, if you've already fixed your code to match, you'll have to do it once again 😓 #82

@beder-bourahmah-gluwa
Copy link
Author

Thank you for the quick turnaround @ewilz . Since your PR makes those route arrays optional on the constructor and we were explicitly setting them to empty arrays when not utilizing them in our code, we won't need to change our code after that change is released. Thank you!

@ewilz
Copy link
Member

ewilz commented Aug 29, 2024

great, sure then you should be good,. yah for some of the other functions like fromRoute it's a little more opinionated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@ewilz @beder-bourahmah-gluwa and others