Uniswap / sdk-core

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

feat: native bigint support, modernization of sdk #69

Open koraykoska opened 1 year ago

koraykoska commented 1 year ago

This is part of the sdk upgrade grant.

Updates

Important backwards compatibility notes

The updates in this PR are fully source-code compatible and non-breaking. But explicit < es2020 projects will not be able to compile it.
So, if people use a modern setup, they won't have to change anything in their existing code but can start using native BigInt (and even if not will feel a performance difference).
Because of the above, I propose making a major version bump anyways. That way, users will need to explicitly opt-in to the new core-sdk, but if they do they don't have to update any source code.

BigInt reduces verbosity-based bugs and helps code to be concise and easier to read. So people will be able to read through the source code going forward more easily and possibly make better contributions because of that.

stale[bot] commented 1 year ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

koraykoska commented 1 year ago

@cbachmeier @just-toby

koraykoska commented 1 year ago

@zzmp

1) Breaking up the PR into multiple defeats the purpose of the fork in the Uniswap foundation Github. We decided it's better to have the scratchpad there and submit PRs for every milestone. We understand that it can take time to get proper reviews and hence wanted to combine it as much as possible instead of sitting on multiple open PRs before being able to continue. Splitting up the PR again after having done proper project management on the foundation repo would be a waste of everyone's time.

2) When we dropped the deprecated build tools library, we had to choose between commonjs and es modules. As far as I could see, commonjs compiles are usable with both commonjs and es modules. Whereas the other way round is only compatible with es modules (not relevant for typescript but still relevant for plain JS).

If you have reasons to switch to es modules only and are ok with support for older JS projects being dropped I can go ahead.

just-toby commented 1 year ago

@koraykoska for the future: i think it can take less than 2-3 months to get a review, if you explicitly request reviews, tag us, or get our attention via slack or through the uniswap foundation. i think the team just wasn't aware of this PR until recently, so sorry for the delay!

the current reviewers also aren't aware of the previous PRs on the fork, so at this point we're re-reviewing the whole change at once. this will contribute to slower reviews on our end too.

zzmp commented 1 year ago

2. When we dropped the deprecated build tools library, we had to choose between commonjs and es modules. As far as I could see, commonjs compiles are usable with both commonjs and es modules. Whereas the other way round is only compatible with es modules (not relevant for typescript but still relevant for plain JS).

cjs builds are not as easily tree-shakeable by build tools like webpack, and so this may contribute to larger bundle sizes for websites which use this library, amongst them app.uniswap.org. For this reason we have a strong preference to continue to have a default esm export available.

koraykoska commented 1 year ago

@just-toby @zzmp Switched back to esm as it's ultimately your decision. Tests and build step pass.

elee1766 commented 1 year ago

could you remove the depdnency on @ethersproject/address

Florian-S-A-W commented 1 year ago

could you remove the depdnency on @ethersproject/address

I'll look into it, it's only used once in the project I think.

krzkaczor commented 1 year ago

@koraykoska, any ETA on this? sdk-core is quite a widespread package in the ecosystem, and no native bigint support is a problem.

koraykoska commented 1 year ago

@krzkaczor Waiting for the final reviews.

koraykoska commented 1 year ago

@just-toby Please re-run the linter workflow. I had to enable push permissions for github actions

koraykoska commented 11 months ago

@zzmp @JFrankfurt Can someone please re-run the linter workflow please?

koraykoska commented 11 months ago

@just-toby @zzmp @JFrankfurt Please fix the permission issue with the linter and re-run it :)

stale[bot] commented 9 months ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

Florian-S-A-W commented 9 months ago

Yes this is still relevant. This can be merged after rerunning the actions.

shuhuiluo commented 9 months ago

Yes this is still relevant. This can be merged after rerunning the actions.

How hard is it for them to run the actions? Lol.

maxandron commented 7 months ago

Yes this is still relevant

koraykoska commented 7 months ago

@just-toby @zzmp @cbachmeier @JFrankfurt @grabbou

koraykoska commented 7 months ago

Please don't let this go over more than 1 year