Uniswap / sdk-core

⚙️ Code shared across TypeScript Uniswap SDK versions
MIT License
121 stars 355 forks source link

Could not parse fraction #20

Open cl510804194 opened 2 years ago

cl510804194 commented 2 years ago

when i copy the code

image

and throw a error such as

image

this is my params i find source code params new Percent(50, 10_000),also throw the same error

cl510804194 commented 2 years ago

this is my params image or ![Uploading image.png…]()

amanmehra262001 commented 1 year ago

Is this issue resolved yet.

amanmehra262001 commented 1 year ago

Switching JSBI version to "jsbi": "^3.1.4", worked for me

Florian-S-A-W commented 1 year ago

Hi @cl510804194 @amanmehra262001

I could not reproduce your issue with neither the latest version of the smart-order-router package nor the core-sdk package. I can see that the core-sdkhas been using "jsbi": "^3.1.4" for quite some time now, what version of what package are you using where you are experiencing this issue?

ai-blib commented 1 year ago
image

Through debugging, I found that the type of ZERO in the code has changed。 Cause the following judgment to fail image。 I tried to change the packaging tool of the project to webpack to solve the problem. I used vite before

ai-blib commented 1 year ago

image It is here that the type judgment fails. You can also modify this code to solve

Florian-S-A-W commented 1 year ago

Hi @ai-blib

This code works for me without any issues. What packages/versions are you using? Where is this ZERO from?

ai-blib commented 1 year ago
image image

From this package。 The type of ZARO should be JSBI,In unknown circumstances, the type of ZERO becomes array. Therefore, it is judged that the verification did not pass

image
ai-blib commented 1 year ago
image
ai-blib commented 1 year ago

My ultimate solution is to modify @ uniswap/sdk core. image. Until now, I haven't figured out why the type of SERO is sometimes JSBI, and sometimes Array I found through the debugger that when an error is reported, the ZAERO type changes to array. I feel like it's a waste of time, find a reason. So, @ uniswap/sdk core was modified

Florian-S-A-W commented 1 year ago

Thanks @ai-blib

Unfortunately, JSBI is an extension of Array, so instanceof Array will always be true for any JSBI instance as the instanceof operator checks if the prototype property of a constructor appears anywhere in the prototype chain of the object. Adding Array as a supported type to construct a Fraction is a hack and would lead to undefined behaviour for any Array that is not a JSBI instance.

So unfortunately we have to find out why ZERO is not a JSBI instance (but still an Array?) in your case. Are you using the latest version of the router-sdk? Do you have different versions of any of the other sdks installed somehow? Could it be that you are using a different version of JSBI somewhere else in your project?

Ultimately I think it would make more sense to drop JSBI than try to find a solution for this.

ai-blib commented 1 year ago

I also thought it was a version issue at first. router-sdk I will always be the latest version. still have problem。 I also did experiments. create-react-app creates a react project,It is possible to use the latest version,Its packaging tool is webpack。Then, based on vite's new packaging tool, And then my new packaging tool based on vite will report this,Could not parse fraction。I do not know why it has to be like this。 I have tried using the latest version

Florian-S-A-W commented 1 year ago

Could you send me your OS, your node version and a code snippet that reproduces this issue? I'll try to reproduce it in Docker.

ai-blib commented 1 year ago

But I've been a bit busy lately, wait for me to finish the work at hand. I will send out the project of my experiment, and it took me a lot of time to find the reason for this problem

Florian-S-A-W commented 1 year ago

Sure, thanks for your help.

torhector2 commented 1 year ago

Hi @Florian-S-A-W

We also find this issue on our project when integrating the Uniswap widget (that uses the sdk-core). We're using Vite, I tried integrating it with Nextjs and found no issues. My guess is that Vite has some kind of issue with JSBI.

torhector2 commented 1 year ago

We opened an issue in the widget repo, but the source error seems to be the same as here, the tryParseFraction https://github.com/Uniswap/widgets/issues/586

Florian-S-A-W commented 1 year ago

Thank you @torhector2 that helps a lot. I will look into it.

rafinskipg commented 1 year ago

did you see any reason why it fails @Florian-S-A-W ?

rafinskipg commented 1 year ago

image

ai-blib commented 1 year ago

Hi @Florian-S-A-W

We also find this issue on our project when integrating the Uniswap widget (that uses the sdk-core). We're using Vite, I tried integrating it with Nextjs and found no issues. My guess is that Vite has some kind of issue with JSBI.

Regarding vite, you can try configuring it like this to see if it is feasible

image
b-pmcg commented 1 year ago

Hi @Florian-S-A-W We also find this issue on our project when integrating the Uniswap widget (that uses the sdk-core). We're using Vite, I tried integrating it with Nextjs and found no issues. My guess is that Vite has some kind of issue with JSBI.

Regarding vite, you can try configuring it like this to see if it is feasible

image

This worked for me, but I it uncovered a new error require is not defined. Adding this config option into to the vite config fixes it, but I'm not sure if there's any drawbacks to adding it.

build: {
    commonjsOptions: {
      transformMixedEsModules: true
    },
  },

Also I had to pin jsbi to version 3.x, as there were some breaking changes in v4

Florian-S-A-W commented 1 year ago

This issue seems to come from Vite rebasing direct node module imports which leads to double instantiating the same dependency and instanceof failing. See https://github.com/vitejs/vite/issues/9828 and https://github.com/vitejs/vite/issues/9528 It seems that configuring vite like @ai-blib and @b-pmcg works but I don't see an acceptable way of fixing this issue directly in the sdk-core. Not using instanceof would just be incorrect.

ashutosh887 commented 1 year ago

I would like to work on this @cl510804194 @amanmehra262001 @moodysalem

mrosendin commented 11 months ago

Hi @Florian-S-A-W, I had the same issue as @ai-blib using the latest versions of sdk-core and v3-sdk. Changing JSBI version to 3.2.5 did not solve for me. I independently came up with the same solution as @ai-blib. However, I agree with you this is a hacky solution.

I encountered the Could not parse fraction error when running v3-sdk test suite, occurring from Fraction.tryParseFunction in sdk-core. For example, the transpiled sdk-core package didn't seem to register the JSBI type signature for other when invoking Fraction.lessThan(other).

Florian-S-A-W commented 11 months ago

Hi @mrosendin , We created a major update for the v3-sdk and sdk-core for the Uniswap Foundation that removes internal usage of JSBI. It should (hopefully) be merged soon. Just waiting on Uniswap Labs for some reviews.