Staking Rewards Viewer milestone 1#232
Conversation
mmagician
left a comment
mmagician
left a comment
There was a problem hiding this comment.
@jackson-harris-iii Thanks for the delivery. May I kindly ask you to follow the instructions outlined in the template:
Ideally all links inside the below table should include a commit hash, which will be used for testing.
Right now there's not a single link to your code and no way for us to test your deliverables.
@mmagician I have added commit hashes to line up with the completion of each deliverable as best I could. I did read into the link/commit hashes and it mentioned you would use the main branch if none were provided. I believed that because there is a test suite, a live version of the application, and a dockerfile that those would be used to run the application and then test for the functionality. (Attempting to run the application from prior commits will result in undesired behavior across the application. So it would be best to verify all completed work from the final/most recent commit) |
|
@jackson-harris-iii Thanks for that. I asked for detailed information as the original PR contained no link to your repo at all, so using |
@mmagician 😅 That makes sense. I didn't realize there was nothing connecting back to the original repo at all...Thanks for the reply back. Do you have an idea of when you might get to check the submission or how often on average this portion of the process takes? ( No worries if not) |
|
We've got quite a number of deliveries in the pipeline (see # of PRs here), so it might take a bit longer than usual - hard to say exactly. |
alxs
changed the title
|
@jackson-harris-iii I had a quick look at your delivery table and realised that the links you included point to (somewhat random?) commits themselves (e.g. the commit that you reference for license has nothing to do with the license itself). Rather, what we are after is the link to the file(s)/module(s)/component(s) where the functionality is implemented. As mentioned earlier, we don't need the commit hashes if the repo's latest state should be used for testing (e.g. rather than pointing to specific commit hash of the license you can just use master branch. While it is obvious where the license is located, it is less so for other, functional parts of your logic. Also please include the link to your original application document as specified in the template (you might have skipped the template code there). I know these might seem like trivial technicalities, but the milestone deliveries are not always evaluated by the grants team who are familiar with the process, but rather are sometimes be done by other community members. |
|
@jackson-harris-iii I just added the link to the application document myself. Please fix the deliverable links (feel free to have a look at other deliveries to see what we mean) and we'll get back to you with more feedback. |
|
I was trying the front end with some random Polkadot addresses and there seem to be some issues here:
It would also be cool to add some tooltips to the user inputs. Especially the "start balance" requires some explanation. It is also important to tell the user that this is an optional input. We should make it possible to leave the field open and then indicate in the output that the APY could not be calculated. |
jonasW3F
left a comment
jonasW3F
left a comment
There was a problem hiding this comment.
Dear @jackson-harris-iii
Thank you very much for your implementation of the UI. Generally, it looks quite good but there are a few things that I noticed during testing (Brave Browser):
- The information button next to "Get Staking Data" is misaligned and still has a placeholder text.
- It is possible to enter an EndDate that is before the StartDate and the output simply says “No results found” but it should give the hint that the StartDate must be before the EndDate.
- Please add tooltips on the StartDate, EndDate and especially on the startBalance. Let me add another comment that I already posted in the PR: It is also important to tell the user that this is an optional input. We should make it possible to leave the field open and then indicate in the output that the APY could not be calculated.
- The output text is missing the token ticker “DOT” and “KSM” after the balances. Also, please add the note that the fiat value is weighted by daily prices.
- The “EndBalance” in the output is empty.
- The output table for the individual payouts does not work properly. For example, the first page for “rows per page =5” is empty, when you click on the right arrow it shows you 3 rows.
- The export for the .csv file produces a weird file name without the .csv extension.
Let me know if you have any questions.
Cheers
Jonas
|
Hey @jonasW3F thank you for the detailed feedback. Many of the items I can begin work on and will submit an update on those as they are completed, but there are a few requests that I have questions about.
|
The following screenshots show you the issue. I guess the second picture should have 5 rows and should be displayed from the beginning, before clicking on the right arrow.
|
|
@jackson-harris-iii Any updates yet? |
|
@jonasW3F I've gone ahead and addressed the following requests (should be all of the ones you've previously mentioned) These changes are merged to main and awaiting review.
|
|
Thank you very much for the adjustments. I did a quick test and found the following issues (for some random addresses, CaRYnYy5JVW248MhNXxLb9NDaX7BWNE32vJfJKtQYY2bnqc and G7PXpsAvSjaYFMjQ3oPTyKUV4kh7eD3oTnNd9eDSMBscSZH):
And why are input addresses restricted to three (btw. the tooltip says "two")? If you change the way the output boxes for the individual outputs are ordered, you might fix the display issue and also be able to increase the number of input addresses. I think a reasonable number would be 10-15 or so. Cheers |
|
Hey @jonasW3F, I have changed the total number of addresses to 5, it was set at 2 for both UI purposes as well as ensuring a quicker response time, I was unsure if having many addresses (especially over a long duration of time) would be a lot of load on a 100% client side application. I attempted with 4 addresses and had similar load times to one address so up to 5 shouldn't be a problem and the UI has been adjusted slightly as well to accommodate the additional input fields. (Beyond 5 addresses there are some performance issues that have to do with node code being ran in the browser. Not a problem, but in order to go beyond 5 would require an overhaul using web assembly and another language.)
|
Thank you very much for your delivery. The ordering of the output addresses now looks good. I only have 2 minor details:
Aside from that, I think things are fine. Thanks again. |
Hey @jonasW3F, I went ahead and modified the APY message as well as added rounding to the Fiat Values in the summary. If that is everything what are the next steps in the process? Thanks, |
|
@jonasW3F - thanks for your time to help out with the milestone review. |
|
As a final note before accepting your milestone: please review the points outlined in the issue I opened in your repo. Many thanks! |
@mmagician The links have been removed and the issue has been closed. Should be good to go! |
|
@jackson-harris-iii Congratulations, I've accepted your delivery. See my full evaluation here. Feel free to keep improving the tool further. |
@mmagician thank you for accepting the milestone and leaving such actionable feedback.
This will increase performance without sacrificing the ability for this application to work purely in the client side which was necessary to makes it 100% deployable and operational to ipfs without the need for a server. Jackson |
|
Hmm, as far as I'm aware it's not the staking collector code that's slow, but rather the fact that the response from the node is slow...I don't think you would solve the problem by re-writing the logic in Rust. If you want to optimise performance, it would rather be running another RPC archive node and querying that one instead. If you want to be fancy, you could even include some basic auth that only enables your application to access the node's ports, i.e. there's no extra load on the node, but that's probably an overkill. |
|
@jonasW3F Hey, I wanted to reach out to you and see if you're open to finding some time to chat about the future of this tool.
If so what's the best way to chat and connect? |
|
@jackson-harris-iii Thanks for connecting again. As far as I'm aware, he's off on holidays right now. I'll ping him once he's back! |
@mmagician awesome! Thanks for getting back to me, how long might he be gone for, just so I have an idea of when to be in the look out for something. Also, Quick question, what is the typical turn around time for the grant disbursement? |
|
@jackson-harris-iii I'm not sure about his time off, will let you know once he's back. |
|
Hi @jackson-harris-iii |
|
Btw, it seems the service has some issues, it does not find any rewards for addresses that I find rewards for with my script. Could you check, @jackson-harris-iii ? |
Interesting, can you provide the parameters that you're providing to the tool and I can take a look. |
Checking in on an update for this. |
|
There was an issue with Subscan's Kusama API which has been resolved now. Still, it seems the front end does not show any rewards for either Polkadot or Kusama addresses. |
|
hi @jackson-harris-iii: we transferred the payment today. Apologies for the delay! |
* Create pallet_maci.md * Update pallet_maci.md * Update pallet_maci.md * Update pallet_maci.md
5 participants
Milestone Delivery Checklist
Link to the application PR: w3f/Grants-Program#429