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

zkwasm rollups transfer milestone 2 #1092

Conversation

ashWhiteHat
Copy link
Contributor

Milestone Delivery Checklist

  • The milestone-delivery-template.md has been copied and updated.
  • This pull request is being made by the same account as the accepted application.
  • I have disclosed any and all sources of reused code in the submitted repositories and have done my due diligence to meet its license requirements.
  • In case of acceptance, an invoice must be submitted and the payment will be transferred to the Polkadot/fiat account provided in the application.
  • The delivery is according to the Guidelines for Milestone Deliverables.

Link to the application pull request: w3f/Grants-Program#1487

@takahser
Copy link
Contributor

@ashWhiteHat sorry for the delay here, we currently have a bit of a backlog. I've started the evaluation but haven't completed it yet, so far I didn't find any issues. I'll be back with the full report soon.

@ashWhiteHat
Copy link
Contributor Author

Hi @takahser
Thank you for the review.
Please take time 👍

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.

@ashWhiteHat thanks for your patience and sorry for the delay. Last week the Polkadot Blockchain Academy kept me busy, but today I've finally concluded the first round of review, see here.
Essentially, the Tutorial could need some more details and I found some inconsistencies as well. LMK if you have any questions

@ashWhiteHat
Copy link
Contributor Author

Hi @takahser

Thank you for the review!

  • I know it's just a simple Dockerfile that runs tests, but please add instructions on how to test it. Also, it'd have been nice to have a docker setup that allows to spin up a node which uses your nova pallet.

We assumed that developers setup the container through docker-compose.
According to your review, we added how to setup with docker.
https://github.com/KogarashiNetwork/Kogarashi?tab=readme-ov-file#test
I think we don't need node which uses your nova pallet because developers use our pallet by importing it to their node.
The way to import is the same with other pallet.

  • in step 3 you mention that the tutorial is based on the sum-storage pallet; I think it'd be better to mention it at the very beginning

This is good point!
We added description about dependent libraries and abstraction.
https://kogarashinetwork.github.io/tutorial/nova_pallet/

  • in step 4 you mention TestRuntime and show the code found in recipes/pallets/sum-storage/src/tests.rs in the 1st code snippet, however, you print a different path: runtime/src/main.rs
  • still in step 4, in the 2nd code snippet you print the same path again
  • in step 5 you mention the /src/main.rs which doesn't exist neither within the sum-storage pallet nor the recipes repo
  • similarly, in step 2 you mention the /src/main.rs which doesn't exist neither within the sum-storage pallet nor the recipes repo

Regarding pallet import, we aligned our notation with Substrate official documentation because most developer familiar with it.
https://docs.substrate.io/reference/how-to-guides/basics/import-a-pallet/
We added description that our notation is aligned with Substrate pallet import tutorial and put its reference.

We also added full example of tutorial and put its link on the top.
https://github.com/KogarashiNetwork/Kogarashi/tree/master/sample

I would appreciate it if you could confirm.
Thank you!

@takahser takahser self-requested a review January 24, 2024 16:11
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.

@ashWhiteHat thanks for the updates, that's helpful.

However, the docker compose setup didn't work for me:

Kogarashi % docker compose up
[+] Running 1/0
 ✔ Container nova  Created                                                                                                       0.0s 
Attaching to nova
nova  |     Finished release [optimized + debuginfo] target(s) in 0.59s
nova  |      Running unittests src/lib.rs (/app/target/release/deps/zk_storage-e36e0c6c09af8122)
nova  | 
nova  | running 3 tests
nova  | test tests::__construct_runtime_integrity_test::runtime_integrity_tests ... ok
nova  | test tests::default_sum_zero ... ok
nova  | error: test failed, to rerun pass `--lib`
nova  | 
nova  | Caused by:
nova  |   process didn't exit successfully: `/app/target/release/deps/zk_storage-e36e0c6c09af8122` (signal: 9, SIGKILL: kill)
nova exited with code 101

Also, thanks for adding the sample project and improving the Tutorial. Could you add instructions on how to run it as well as explain, whether steps 4-6 of the tutorial are covered by the sample project?

@ashWhiteHat
Copy link
Contributor Author

Hi @takahser
Thank you for the polite review.
We modified according to your review.

However, the docker compose setup didn't work for me:

It was the problem of allocated memory for docker.
We reduced memory for container and fixed this.

Could you add instructions on how to run it as well as explain, whether steps 4-6 of the tutorial are covered by the sample project?

Sorry for inconvenient.
We updated tutorial as follows.

  • Put corresponding sample code link on file path
  • Separate client implementation part (4. Define the function for IVC verification)
  • Put how to import pallet tutorial link on footer

Now, tutorial file path corresponds sample implementation and description follows development process.

I would appreciate it if you could confirm.
Thank you!

@ashWhiteHat ashWhiteHat requested a review from takahser January 30, 2024 03:31
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.

@ashWhiteHat sorry for the delay here. I've pulled the latest changes, but I'm still getting the same error as before.

% git rev-parse HEAD
dc1450971ccfd35b9d337a8d9f7bc997cbe02d89

% docker compose up
[+] Running 1/0
 ✔ Container nova  Created                                                                                                                                                                             0.0s 
Attaching to nova
nova  |     Finished release [optimized + debuginfo] target(s) in 0.63s
nova  |      Running unittests src/lib.rs (/app/target/release/deps/zk_storage-e36e0c6c09af8122)
nova  | 
nova  | running 3 tests
nova  | test tests::__construct_runtime_integrity_test::runtime_integrity_tests ... ok
nova  | test tests::default_sum_zero ... ok
nova  | error: test failed, to rerun pass `--lib`
nova  | 
nova  | Caused by:
nova  |   process didn't exit successfully: `/app/target/release/deps/zk_storage-e36e0c6c09af8122` (signal: 9, SIGKILL: kill)
nova exited with code 101

Regarding my memory (RAM), at the time of running I had 15GB of free space and docker used around an additional 8GB of space.

Thx for updating and improving the tutorial along with the sample app, I'm fine with that part now. I've updated the evaluation accordingly.

@ashWhiteHat
Copy link
Contributor Author

Hi @takahser
Thank you for the review.

I optimized hash memory performance.
KogarashiNetwork/Kogarashi#191

In Github Actions and my local environment, it passed the tests.
Could you run cargo test --release on sample directory if it fail to pass tests on docker?

I would appreciate it if you could confirm.
Thank you.

@ashWhiteHat ashWhiteHat requested a review from takahser February 9, 2024 07:30
@takahser
Copy link
Contributor

takahser commented Feb 9, 2024

@ashWhiteHat
Copy link
Contributor Author

@takahser
Thank you for the check.

Doesn't docker test pass?

@ashWhiteHat
Copy link
Contributor Author

Hi @takahser

Is everything okay?
I would like to know the current status.

@takahser
Copy link
Contributor

takahser commented Feb 19, 2024

@ashWhiteHat sorry for the delay. Correct, the docker issue hasn't been resolved, yet. Once this is fixed, I'll approve the delivery.

If you're not sure how to fix it on my machine, we can also schedule a call and run it on your machine instead.

@ashWhiteHat
Copy link
Contributor Author

Thank you for the providing the schedule.
I booked your schedule!
I am looking forward to seeing you.

@ashWhiteHat
Copy link
Contributor Author

Thank you for the call.
This is the output of command.

➜  Kogarashi git:(master) dcom up
Starting nova ... done
Attaching to nova
nova    |     Finished release [optimized] target(s) in 0.82s
nova    |      Running unittests src/lib.rs (target/release/deps/zk_storage-e18c6ff412cf73a5)
nova    | 
nova    | running 3 tests
nova    | test tests::__construct_runtime_integrity_test::runtime_integrity_tests ... ok
nova    | test tests::default_sum_zero ... ok
nova    | test tests::sums_thing_one has been running for over 60 seconds
nova    | test tests::sums_thing_one ... ok
nova    | 
nova    | test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 107.26s
nova    | 
nova    |    Doc-tests zk-storage
nova    | 
nova    | running 0 tests
nova    | 
nova    | test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
nova    | 
nova exited with code 0

I would appreciate it if you could confirm.
Thank you!

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.

@ashWhiteHat thanks for your time and efforts, your delivery has been accepted. You can find the final evaluation here.

@takahser takahser merged commit ae7f5fc into w3f:master Feb 20, 2024
6 checks passed
Copy link

We noticed that this is the last milestone of your project. Congratulations on completing your grant! 🎊

So, where to from here? First of all, you should join our Grants Community chat, if you haven't already, so we can stay in touch.
If you are looking for continuative support for your project, there are quite a few options. The main goal of the W3F grants program is to support research as well as early-stage technical projects. If your project still falls under one of those categories, you might want to apply for a follow-up grant. However, depending on your goals and project status, there are other support programs in our ecosystem that might be better suited as the next step. For example, projects with a Business Case/Token should look into the Substrate Builders Program or VC Funding and Common Good projects have a good chance of receiving Treasury Funding. If you are looking for guidance, the team at https://substrate.io/ecosystem/square-one/ has compiled a list of ecosystem support sources and are happy to help you navigate it.

For a more comprehensive list, see our Alternative Funding page. Let us know if you have any questions regarding the above. We are more than happy to point you to additional resources and help you determine the best course of action.
Lastly, we hope your W3F grant was a success and we want to thank you for being part of the journey!

Copy link

🪙 Please fill out the invoice form in order to initiate the payment process. Thank you!

@semuelle
Copy link
Member

semuelle commented Mar 1, 2024

@ashWhiteHat, have you submitted an invoice for this milestone?

@ashWhiteHat
Copy link
Contributor Author

@semuelle
Yes, I have submitted last week.

@semuelle
Copy link
Member

semuelle commented Mar 1, 2024

Found it. My bad.

@RouvenP
Copy link

RouvenP commented Mar 15, 2024

hi @ashWhiteHat we sent the payment today

@ashWhiteHat
Copy link
Contributor Author

Hi @RouvenP
Thank you for the notification.
I confirmed it!

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.

5 participants