Closed kitanovicd closed 3 weeks ago
vercel[bot]
commented
1 month ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| seamless-interface | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Aug 22, 2024 6:45pm |
WingsDevelopment
commented
1 month ago general comment, I would really try to avoid passing config as argument of each function and i would really like to try to find a way to create wrapper around readContractQueryOptions or some other solution that will allow us to never pass any config anywhere
kitanovicd
commented
1 month ago general comment, I would really try to avoid passing config as argument of each function and i would really like to try to find a way to create wrapper around readContractQueryOptions or some other solution that will allow us to never pass any config anywhere
This is great point. I think in a long run how we are currently doing it is not good. It will be so much of repeating code. Based on your suggestion I created queryContract and queryOptions wrappers. Now everything should be much easier. We don't need to get query client in every hook and we don't need to pass config everywhere. Please do one more round of detail review and tell me what do you think
kitanovicd
commented
3 weeks ago @fredwes @WingsDevelopment I changed folder structure as @WingsDevelopment suggested in next PR by mistake (should be commented on this one). I did test it and it looks good. I have a feeling that unit testing is much easier when everything is separated. I implemented unit tests here so I think it is still easy to review. That folder structure is implemented only in complex hooks such as user strategy profit. In simple hooks, such as equity fetching or price fetching I would keep everything in one file. If we go with separated files always it will create 10 files on PR even for very simple contract reads. I think current structure is fine. If some of the hooks become more complex that we will divide it into multiple files. The only thing I am not sure about is should we have also one file for types in sub-folders. For now I would keep it as it is in order to make PR smaller.
WingsDevelopment
commented
3 weeks ago @fredwes @WingsDevelopment I changed folder structure as @WingsDevelopment suggested in next PR by mistake (should be commented on this one). I did test it and it looks good. I have a feeling that unit testing is much easier when everything is separated. I implemented unit tests here so I think it is still easy to review. That folder structure is implemented only in complex hooks such as user strategy profit. In simple hooks, such as equity fetching or price fetching I would keep everything in one file. If we go with separated files always it will create 10 files on PR even for very simple contract reads. I think current structure is fine. If some of the hooks become more complex that we will divide it into multiple files. The only thing I am not sure about is should we have also one file for types in sub-folders. For now I would keep it as it is in order to make PR smaller.
i would keep it consistent even for small things, even one liners will still be easy to review and come back later to it..
Implemented calculation for unrealised profit/loss and calculation for realised profit/loss. Unrealised profit is calculated based on weighted average. Probably we will have unrealised gains shown at first plan but realised profit shown in second plan in some tooltip. I placed hooks into statev3 folder. Hooks are not coded based on useSeamlessContractRead hook because of loops. This hooks will be used for current holding table so they should be properly reviewed and checked if they are in proper format.
Regarding claimable rewards hook we already have that done in current repository. I didn't touch files at all and I am not sure should I copy them to new holder or not so I would leave that discussion open.
When calculating profit/loss I am parsing and handling only transfer events from strategy. I fetch equity in that block and that is how I calculate PnL. I was doing it by going through Deposit and Withdraw events also but I realised that code is much better when handling only Transfer ones. This means that we are not calculating for deposit cost inside this calculation. If this is deal breaker I can switch back in couple of minutes it is not a problem.
Total profit percentage is calculated based on weighted average like it is calculated on Aave dashboard. I think that is default way of calculating it. Also I implemented unit tests for all complex calculations.