Very nice! Let me know when you want a review :)

Any idea why the CI chokes on the CLI tests?

Thanks! Will do.

Not sure as to why the CI isn't passing - was working earlier when I ran tests locally. I'll take a look tonight and see what's up.

Btw, do you think this would be easy to do at the same time? #254

Btw, do you think this would be easy to do at the same time? #254

Long story short:
https://stackoverflow.com/questions/71814845/how-to-calculate-uniswap-v3-pools-total-value-locked-tvl-on-chain
Due to personal reasons, I can't implement that myself at the moment. However, it seems to be not too complicated to add.

Btw, do you think this would be easy to do at the same time? #254

Long story short: https://stackoverflow.com/questions/71814845/how-to-calculate-uniswap-v3-pools-total-value-locked-tvl-on-chain Due to personal reasons, I can't implement that myself at the moment. However, it seems to be not too complicated to add.

Sure thing, seems pretty straightforward to implement

Very nice! Let me know when you want a review :)

Any idea why the CI chokes on the CLI tests?

Took a peek at the logs for the CI, seems like provider env variable may not have set

Ah yeah, I just now remember that secrets (like our PROVIDER credentials) aren't set on PRs.

Might get a second key, obfuscate it slightly, and use it for PRs.

Should have fixed it in #266, feel free to rebase so the tests get green :)

Codecov Report

Merging #264 (0175265) into master (208843c) will increase coverage by 2.16%.
The diff coverage is 92.67%.

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
+ Coverage   82.42%   84.59%   +2.16%     
==========================================
  Files          10       10              
  Lines         808     1032     +224     
==========================================
+ Hits          666      873     +207     
- Misses        142      159      +17     

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Now that we have all green :) Going to write tests for the methods that address #254 and should be good for a review

Btw, do you think this would be easy to do at the same time? #254

Long story short: https://stackoverflow.com/questions/71814845/how-to-calculate-uniswap-v3-pools-total-value-locked-tvl-on-chain Due to personal reasons, I can't implement that myself at the moment. However, it seems to be not too complicated to add.

Sure thing, seems pretty straightforward to implement

After implementing a version of this - seems this takes quite a while to run as it iterates across the entire tick range of a pool and calls the .tick() function of the contract each iteration. So, I added a method to fetch the TVL data from the uniswap subgraph using the built-in request library.

Do you think I should remove the subgraph query function? suppose it's easy enough for users to make similar requests on their own (this is also failing some tests at the moment).

@ErikBjare this is now ready for review :)

wrt to #254 I've added a function get_tvl_in_pool() and have commented the logic (some weird hacks with the tickBitmap to get it working), but let me know if any questions.

Edit: I've removed my prior functions for querying subgraph, feel that's out of scope here and there are known issues w/ its calculation of TVL

Amazing job, that's impressive. The only thing I just can't get why
def get_liquidity_positions(self) -> List[int]:
instead of
def get_liquidity_positions(address: AddressLike) -> List[int]:

IMO if we have a scenario when this function has the reason to be implemented for our address, then we also have a similar/same scenario for others.

Fair point, I agree. I'll make that change quick.

@ErikBjare just take a look at this grandeur!

Merged a squashed version in 9edfbaa (which was messy), now trying to address some issues during rebase.

Alright, I can't manage to get this PR to get marked as "Merged" for some reason (keeps complaining about some conflict?).

Changes are merged though, fixing some lingering issues on master.

Thanks again for contributing @KeremP! ❤️

Sounds good! Thanks for the review :)

What is the difference between mint_liquidity and mint_position here? I tried to create positions using both. mint_liquidity was successful but mint_position didn't seem to create a position though the function execution was successful.