-
Notifications
You must be signed in to change notification settings - Fork 108
Show the wallets market performance - don't merge yet #3410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Ah, and ignore the .gitignore changes. :) |
@kakulukia thank you for the detailed summary. I will need a bit of time to look through it in detail and then give feedback. For expectation management, external contributions aren't guaranteed to be merged as the review process can be quiet resource intensive and need to align with our internal priorities. |
Hi @jadzeidan, thanks for considering to merge it. If the likelihood turns out to be above 50% I can surely polish this a bit more and add some additional tests. I just need little guidance. |
Hi @jadzeidan congrats for your product launch. Since the BTC Prague i know why all of you had no time for anything else and especially feature requests like this one. But i hope all went as planned and the event also was a success for your team - it sure was for me and my wife who finally understood BTC. :) I had a longer conversation with one of your colleagues in Prague and he already knew about the PR - so i heard that also other customers are facing the same pain I am when looking at 27.484,85 % gains with a simple DCA plan. ![]() And while my main goal of rather viewing the actual 15% I'm up since the start of that DCA plan is already kinda completed, I'd like to help you to get that rolled out to everybody to have a smoother experience with the wallet app. Over the summer holidays I'd like to continue on that feature, but would love to hear your feedback first. Happy to hear your thoughts! |
Hi @kakulukia, thank you for the kind works. Yes, the Nova launch has kept us quite busy (and still does!). And thanks for visiting our booth, my collogues let me know that you were there and have also nudged me to take a look at this issue again :) From a UX perspective, I think makes sense to keep this as an opt-in feature triggered in the app settings. From a technical perspective, I will create an internal issue so our devs can take a look. Please keep in mind reviews can be quite resource intensive so could take some time to get you feedback. I will update this ticket once I learn more. |
Hi @kakulukia I have discussed this with the engineering team and we concluded that there are actually many ways the portfolio can be calculated depending on the use case and how it can be implemented, all of which will need more discussion from our side so we know we are putting our efforts in the right direction. In addition, anything that touches fiat rates adds extra maintenance. So for now we will put this on hold and if/when we decide to move forward with this I will update the issue. Thank you for your patience and sorry about taking a while to get back to you. |
Understood, thanks for the update and hope to hear from you soon. :D |
This PR is not quite done yet, but i rather wanted to ask for feedback before i continue.
If I have a savings plan rather than some BTC one time investment, the current percentage gain just makes no sense at all. Thats why I started the connected issue some months ago and now this PR is my attempt to fix it.
It's missing some stuff still. But as it already solves my main pain point and I'd like to get some feedback and as im quite unfamiliar with the code base. Is the way it's implemented heading into the right direction?
Here is what it does - it adds a little switch in the upper right corner:
The current default is showing the coin sum data as before, but now its possible to switch to the percent view.
Before continuing there are some questions: The account-summery call serves on hourly (week) and one daily time series to power all other Month, Year and All filters. As the month/year/all percentage gain will be different there are two options. Calculating it on the fly in the frontend and the (in my opinion cleaner) way of letting account-summery just return the currently shown version of the data when pressing one of the filter options.
Yes, that means more calls when switching the filters, but thats why im asking for some guidance.
Here is a quick roundup of what i have done so far to get it working:
Would there be a better UI option or place for the switch? I don't fully like that it's grey for one option and blue for the other. Any idea?
I have one additional question to the tests. I get this while running the backend tests via
go test ./backend/...
:The readme is not specifically telling me how to run those tests, so it was just a guess. There is this
ci
makefile command which is also running into the same 10m timeout. Im not aware that i changed code that might be related to TestServeShutdownServe so im wondering what might be wrong?Also a fresh checkout of the repo is also running into this problem.
I'm leaving all the checkboxes as a reminder of things to do before merging.
Before asking for reviews, here is a check list of the most common things you might need to consider:
Closes #3147