Skip to content
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

Shivarthu Milestone 1 #535

Merged
merged 3 commits into from
Oct 26, 2022
Merged

Shivarthu Milestone 1 #535

merged 3 commits into from
Oct 26, 2022

Conversation

amiyatulu
Copy link
Contributor

@amiyatulu amiyatulu commented Aug 10, 2022

Milestone Delivery Checklist

Link to the application pull request: w3f/Grants-Program#525 < please fill this in with the PR number of your application.

@takahser takahser self-assigned this Aug 17, 2022
@takahser takahser self-requested a review August 17, 2022 12:24
deliveries/Shivarthu Milestone 1.md Outdated Show resolved Hide resolved
@takahser
Copy link
Contributor

@amiyatulu I took a look at your delivery, but I wasn't able to find the instructions to run and test your code.
I added more detailed comments to my evaluation, please have a look and let me know, if you have any questions.

@amiyatulu
Copy link
Contributor Author

@takahser Thank you for reviewing. Docker compose is failing because non creation of .local folder. I have updated the docker-compose.yml file, please use the recent commit. I will soon give you the tutorial and instructions to test the app.

@takahser
Copy link
Contributor

I will soon give you the tutorial and instructions to test the app.

@amiyatulu did you update the tutorial/instructions yet, so they reflects the latest changes? Please let me know when it's ready to test again.

@takahser takahser removed their assignment Aug 29, 2022
@amiyatulu
Copy link
Contributor Author

@takahser takahser self-assigned this Sep 5, 2022
@takahser
Copy link
Contributor

takahser commented Sep 5, 2022

@amiyatulu no problem, thanks for the update. I'm going to take another look soon.

@takahser
Copy link
Contributor

@amiyatulu thanks for your patience and sorry for the wait here - we have currently a rather large backlog and are a bit behind. I took another look, I added a few more comments regarding the deliveries 0b, 0c to my evaluation and I accepted 0d. Let me know if you have any questions.

@amiyatulu
Copy link
Contributor Author

@takahser Thank you for the update.

Documentation Feedback

The template pallet that seems to build on [parity's template pallet](https://paritytech.github.io/substrate/master/src/pallet_template/lib.rs.html#1-102) should be renamed to a more meaningful name.
I am updating the logic of template to into a loose coupling pallet in the next version (https://github.com/amiyatulu/shivarthu/tree/main/pallets/schelling-game-shared) so that logic can be shared to all pallets. I am mainly working on substrate (https://github.com/amiyatulu/shivarthu/tree/main/pallets/profile-validation), need some time for frontend.

I'm confused about the reason for why a [light version of the grant application doc](https://github.com/amiyatulu/shivarthu/blob/main/docs/Shivarthu.md) was added to the repo which was also mentioned in the delivery doc.
I have updated some technical details that is not mentioned in the delivery doc, so made a new version. Not sure if I need to have a pull request for new updates.

The inline comments are currently very sparse.
Functions names are self-explanatory, the technical details article contains all details needed. I will add more inline comments if you say, but in the schelling-game-shared and profile-validation, as template pallet will not be present in the future.

Testing Guide Feedback

There is (still) no test guide.
What test guide do you need, about how to run the test?

However, there are a lot of warnings. Mostly, unused imports and variables, that can easily be fixed:
Sorry for a lot of warnings of unused imports, as it active development phase, I have not removed it, because it may be needed in updates. I will clean it up, if it's required for milestone delivery.

Waiting for your feedback. Please let me know what's need to be done further.

Copy link
Contributor

@takahser takahser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amiyatulu thanks for your reply.

I am updating the logic of template to into a loose coupling pallet in the next version

Ok, that works for me. Please resolve it until the next delivery then.

I have updated some technical details that is not mentioned in the delivery doc, so made a new version.

The problem with this approach is, that it's difficult to track, which version is the current one. I'd suggest you integrate these changes into your original grant application doc by amending them. This should be easy to approve, especially if you just add features and/or technical details.

I will add more inline comments if you say, but in the schelling-game-shared and profile-validation

Okay. Yes, please add sufficient comments to help others understand your code, especially in case they want to reuse it for their own project(s).

What test guide do you need, about how to run the test?

Yes, for completeness it should contain test instructions (probably just cargo test).

@amiyatulu
Copy link
Contributor Author

@takahser Thank you for replying. I have updated the docs and removed most unused imports. Please have a look: https://github.com/amiyatulu/shivarthu

@takahser
Copy link
Contributor

@amiyatulu thanks for your patience and sorry for the delay. I checked it out again and tried to run the frontend. However, it's currently blank and showing a console error Uncaught TypeError: Cannot read properties of undefined (reading 'startsWith'):

image

The network requests look fine:

image

Also, the node is running and I didn't identify any issue when exploring it in neither of the two frontends:

Could you double-check if it works on your machine?

@amiyatulu
Copy link
Contributor Author

@takahser Hi, frontend is running fine on my computer. Have you tried the latest commit? I have uploaded the frontend on vercel at https://shivarthuapp.vercel.app/

Updated tutorial: https://github.com/amiyatulu/shivarthu_frontend/blob/main/docs/Tutorial.md

Copy link
Contributor

@takahser takahser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amiyatulu thanks for the update.
I pulled the latest changes and tried again - same result.

I'm on the following commits:

~/repos/shivarthu$ git log
commit 0535df8a374ea66b0411c4c23fe8a0f67937e8ea (HEAD -> main, origin/main, origin/HEAD)
Author: Amiya Behera <[email protected]>
Date:   Fri Sep 23 16:56:32 2022 +0530

    score game
~/repos/shivarthu_frontend$ git log
commit 371fe81f75267bfd64d186e1f9582a52e76dcddb (HEAD -> main, origin/main, origin/HEAD)
Author: Amiya Behera <[email protected]>
Date:   Thu Sep 29 13:20:57 2022 +0530

    docs

@amiyatulu
Copy link
Contributor Author

@takahser Sorry it happened due to .env file which was in the gitignore. I have commented out the section that requires .env file. Please use the new commit.

@takahser
Copy link
Contributor

@amiyatulu sorry for the delay here, I didn't find time to take a look earlier.
I'll take another round today and give you feedback after.

@takahser
Copy link
Contributor

@amiyatulu I tried again and it's still not working. I tested your code on 2 machines (macOS and Ubuntu).
For some reason, now the node doesn't even connect on the other two frontends anymore. Maybe we can schedule a google meet and you can walk me through, if you're interested, to accelerate the process, what do you think?

@amiyatulu
Copy link
Contributor Author

@takahser Ok. This is my email id: amiyatulu at gmail.com Please let me know at what time and day you like to schedule a Google meet.

@takahser
Copy link
Contributor

@amiyatulu I've sent you some suggestions via email, please check.

Copy link
Contributor

@takahser takahser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amiyatulu thanks for your time in yesterday's call and your explanations.
In the meanwhile, I was able to run and smoke-test your delivery and I decided to accept your delivery. However, there are a few things that you should improve for M2. I'm quoting directly from my evaluation:

As this is the first delivery, I'm now willing to accept it in the current state, but for M2 these issues should definitely be fixed. I'm referring to:

  • more useful inline comments on the pallet code that can be used to generate documentation using rustdoc
  • removal of the application doc that is out of sync with the original
  • renaming of the template pallet to a more meaningful name
  • fixing of warnings during the build & tests

I'm looking forward your M2 delivery that should incorporate these improvements.
Please let me know if you have any questions.

@takahser takahser merged commit 62374f4 into w3f:master Oct 26, 2022
@github-actions
Copy link

Congratulations on completing the first milestone of this grant! As part of the Grants Program, we want to help grant recipients acknowledge their grants publicly. To that end, we’ve created a badge for projects that successfully deliver their first milestone. Note that it must only be used within the context of the delivered work, so please do not display it on your team or project's homepage unless accompanied by a short description of the grant.

Furthermore, you're now welcome to announce the grant publicly. Please remember to observe the foundation’s guidelines in doing so. In case you haven't done so yet, you may also reach out to [email protected] for feedback on your announcement and cross-promotion.

Thank you for your contribution and good luck with the remaining milestones, if any! As usual, please let us know if you run into any delays by leaving a comment on the application PR, or directly submitting an amendment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants