vercel[bot]
commented
2 weeks ago @kfastov is attempting to deploy a commit to the LFG Labs Team on Vercel.
A member of the Team first needs to authorize it.
Closed kfastov closed 1 week ago
vercel[bot]
commented
2 weeks ago @kfastov is attempting to deploy a commit to the LFG Labs Team on Vercel.
A member of the Team first needs to authorize it.
coderabbitai[bot]
commented
2 weeks ago The changes in the pull request significantly modify the Page component in app/[addressOrDomain]/page.tsx. The fetchPortfolioAssets function is replaced with calculateAssetPercentages, which computes asset values while filtering out tokens from excluded protocols. The fetchPortfolioProtocols function is updated to accept a data object for better management, and a new function, fetchPortfolioData, is introduced to orchestrate data fetching. Error handling is improved, and the use of useEffect hooks is refined to ensure correct data fetching and rendering.
| File Path | Change Summary |
|---|---|
| app/[addressOrDomain]/page.tsx | - Replaced fetchPortfolioAssets with calculateAssetPercentages - Updated function signatures for fetchPortfolioAssets and fetchPortfolioProtocols to accept a data object - Added new function: fetchPortfolioData - Enhanced error handling in fetchPortfolioAssets - Refined useEffect hooks and updated rendering logic |
| services/argentPortfolioService.ts | - Implemented fetchDataWithRetry for enhanced data fetching with retries - Updated fetchDapps, fetchTokens, fetchUserTokens, and fetchUserDapps to use the new retry mechanism - Modified calculateTokenPrice to include retry logic |
| Objective | Addressed | Explanation |
|---|---|---|
| Add "Portfolio by asset type" section with a donut chart (#879) | ❌ | No implementation for the donut chart section is present in the changes. |
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
kfastov
commented
2 weeks ago I'll do optimizations and some tests (on different accounts) today and then mark as ready
joeperpetua
commented
2 weeks ago Hey @kfastov , thanks for the mention!
If I may say, looking at the API behavior, all the protocol pair tokens for a given protocol share the same symbol.
This are all the count of tokens that use the same symbol filtering by name containing "pair" (ci):
One approach would be displaying a sum of all the tokens for a given protocol under the same symbol (as they are listed in the API). This would make the chart more aligned to the actual portfolio data, as ignoring them altogether would kinda defeat the purpose of the chart.
This should be achievable using a map with the symbol as a key and incrementing the balance on each encounter of a userToken with the same symbol.
What do you think? @Marchand-Nicolas
PD: you can also use my account to test the chart (0x06F17c9e19D91900ac039f60344cECC6494297CBd37BbB9B3618B048a2Ed3633)
kfastov
commented
2 weeks ago What I aim now is just matching the Argent logic. It seems that Argent portfolio just shows the "free" undeposited tokens, but I agree that it may be counter-intuitive. @Marchand-Nicolas Do you think we should show ETH and STRK here that are locked in the protocol like zkLend or Ekubo?
Marchand-Nicolas
commented
2 weeks ago @kfastov Hey, if that is doable, I think that indeed it would be better to take in account tokens that are locked in protocols such as zklend or ekubo
Marchand-Nicolas
commented
2 weeks ago Hey @kfastov , thanks for the mention!
If I may say, looking at the API behavior, all the protocol pair tokens for a given protocol share the same symbol.
This are all the count of tokens that use the same symbol filtering by name containing "pair" (ci):
- CairoFi-P: 11
- JEDI-P: 795
- LPT: 31 ( this would be the only exception, as it is used by 10kSwap, ProtossSwap and StarkEx )
- LYN: 1
- SSS-P: 8
- sSTARKD-P: 30
- vSFN-P: 1
- vSTARKD-P: 189
- ven-P: 50
One approach would be displaying a sum of all the tokens for a given protocol under the same symbol (as they are listed in the API). This would make the chart more aligned to the actual portfolio data, as ignoring them altogether would kinda defeat the purpose of the chart.
This should be achievable using a map with the symbol as a key and incrementing the balance on each encounter of a userToken with the same symbol.
What do you think? @Marchand-Nicolas
PD: you can also use my account to test the chart (0x06F17c9e19D91900ac039f60344cECC6494297CBd37BbB9B3618B048a2Ed3633)
Thanks for the help, it seems perfectly appropriate to me
vercel[bot]
commented
2 weeks ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| starknet-quest | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Nov 5, 2024 9:15am |
kfastov
commented
2 weeks ago I am working on asset exclusion logic now. Hope to finish it tomorrow. If I realize I can't, I'll just pick a simpler strategy. Motivation for now:
One approach would be displaying a sum of all the tokens for a given protocol under the same symbol
If I understood correctly (please correct me if I'm wrong), it would make asset chart partially resemble the protocol chart, which also kinda defeats its purpose. But I would instead display a sum of all positions across all protocols for a given token, as it's already implemented in this draft PR. The difficulty of this approach is that position values can be larger than total asset value (because of debt), so, to prevent percentage values higher than 100%, I ignored debt positions in the total value calculation, and define total value as sum of all non-debt positions + sum of all non-protocol tokens. To finish the implementation of this approach, I just need a decent exclusion logic (which I am working on), as hardcoding multiple token names seems not very future-proof
This should be achievable using a map with the symbol as a key and incrementing the balance on each encounter of a userToken with the same symbol.
Yes, that's how it's currently implemented.
joeperpetua
commented
2 weeks ago I like the idea of the of displaying the base asset instead of the protocol one, but I think it could not be applicable to all cases. The last comment was regarding Pair Tokens, and those tokens are a mix of other two one (ETH/USDC) for example.
The issue arises mainly on how the protocol manage pair tokens, as some create a new token for that pair, and others display them individually, some examples are:
I guess as the protocols don't have a standardized way to manage them, we could either chose one of the approaches or tey to handle both.
For the pair tokens, we could either display them by protocol pair symbol (last comment) or, what I think may be better, is to display the pair symbols as indovidual entries in the chart, e.g. STRK/ETH, USDT/LORDS, etc
You can check the response of fetchUserDapps, there if there is a pair you should get the two token addresses that compose the pair, there you also get the debt if present so you can handle it as when using fetchUserTokens the balance would be zero even if there is indeed a debt.
Marchand-Nicolas
commented
1 week ago Also there are some conflicts now, but it should be quick to fix
kfastov
commented
1 week ago Omg I messed up with branches and didn't pushed last 2 commits, sorry for that
kfastov
commented
1 week ago Sorry, haven't pushed last commit, just a few minutes, local merge problems Done
kfastov
commented
1 week ago Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/argentPortfolioService.ts (1)>
78-80: Clarify error message to accurately reflect the number of attemptsThe error message thrown after retries may be misleading if the number of attempts differs from
maxRetries. If the function attemptsmaxRetriestimes and then fails, the message "Failed after ${maxRetries} retries" is accurate. However, if it attempts an extra time due to the loop logic, this message could be confusing. Consider adjusting the error message to reflect the actual number of attempts. Apply this diff to adjust the error message:- `Failed after ${maxRetries} retries. Endpoint: ${endpoint}. Error: ${lastError.message}` + `Failed after ${retries} attempts. Endpoint: ${endpoint}. Error: ${lastError.message}`📜 Review details
It's max retries because you may not need so many retries: any subsequent request may become successful
kfastov
commented
1 week ago @Marchand-Nicolas the problem is that the page is being rendered twice and the first time it aborts the requests. I tried to debug this issue but failed so far. It was there even before my commits. I'll try to investigate it later, but for now I can just remove the error notification if it was caused by requests being aborted by re-render.
Marchand-Nicolas
commented
1 week ago @Marchand-Nicolas the problem is that the page is being rendered twice and the first time it aborts the requests. I tried to debug this issue but failed so far. It was there even before my commits. I'll try to investigate it later, but for now I can just remove the error notification if it was caused by requests being aborted by re-render.
Yes that's not a big deal, just make sur the skeleton keeps being displayed during the whole loading process
Marchand-Nicolas
commented
1 week ago My bad it seems to be working properly @kfastov
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
Resolves: #879
Other information
I picked the approach to hide protocol assets and instead show the base ones. Motivation behind this:
Example for Nico.strk (outdated, will update it soon):
Huge thanks to @joeperpetua who already did all the heavy work in #912 creating the charts and the Argent API service
Summary by CodeRabbit
New Features
Bug Fixes
Documentation