Uniswap / v3-sdk

🛠 An SDK for building applications on top of Uniswap V3
MIT License
547 stars 425 forks source link

Remove async await from tickDataProvider #120

Closed a-straus closed 2 years ago

a-straus commented 2 years ago

Hi there,

I have been going through the UniswapV3 sdk code for some time, and I was confused that the pool was sometimes requiring async methods. Digging further, it appears that the pieces of code that are async/await are in the tickDataProvider. In fact, the tickDataProvider is executing synchronous code, so IMO the async/await is confusing and might lead developers to think that the SDK is sometimes talking with the node provider or doing some weird math that needs to be offloaded to a special piece of hardware, similar to how encryption in a library like bCrypt works.

I am putting in this small PR to remove the async/await from just the tickDataProvider before I do more work and remove the async requirement from things like the pool.swap function, which will require more work to clean the tests etc. Maybe you have an explanation for why it was created as async/await?

One reason I can understand is that maybe in the future you are planning to add the option to connect a node provider to the sdk methods which will then actually perform network async functions. In that case, I would argue that it is better to leave the functions as synchronous right now and make them async if and when they actually are so. The code will need to change significantly if they are changed to be actually async.

dalechyn commented 2 years ago

async/await is there because the implementation of the Tick Provider may be different from case to case. If you choose to bulk read the TickLens i.e. and store the ticks, the asynchronous action takes place in the creation of the pool. But you also may make it work as a stream, to fetch only those ticks, which are being used by the pool.swap function. You may also pull the data with GraphQL specifically for some of the ticks. Any logic is possible. There is a need for this, as without it everyone would need to strictly follow a fetch-first pattern, not fetch-on-use.

stale[bot] commented 2 years 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.