Skip to content

Ruby protocol: milestone 2#332

Merged
takahser merged 15 commits intow3f:masterfrom
Ruby-Protocol:master
May 9, 2022
Merged

Ruby protocol: milestone 2#332
takahser merged 15 commits intow3f:masterfrom
Ruby-Protocol:master

Conversation

@rubyprotocol
Copy link
Copy Markdown
Contributor

Milestone Delivery Checklist

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

@Noc2
Copy link
Copy Markdown
Contributor

Noc2 commented Jan 3, 2022

Thanks for the delivery. We will look into it as soon as possible.

@mmagician mmagician self-requested a review January 3, 2022 10:56
@mmagician mmagician self-assigned this Jan 3, 2022
@alxs
Copy link
Copy Markdown
Contributor

alxs commented Jan 4, 2022

@rubyprotocol could you merge master into your branch? I think that should remove the first delivery from the added files and get the checks to pass.

@rubyprotocol
Copy link
Copy Markdown
Contributor Author

@rubyprotocol could you merge master into your branch? I think that should remove the first delivery from the added files and get the checks to pass.

I just delete the milestone 1 delivery, but the check doesn't seem to pass?

@lornacrevelingwgo23
Copy link
Copy Markdown

Hey @Noc2 @alxs @mmagician, this is a gentle reminder that we are still waiting for your comments on our milestone 2 delivery. Thx.

@mmagician
Copy link
Copy Markdown
Contributor

@lornacrevelingwgo23 thanks. I've already started looking into it, will give you some feedback by the end of the week.


**The [invoice form :pencil:](https://forms.gle/LSRr7PCjBpEbKGh89) has been filled out correctly for this milestone and the delivery is according to the official [milestone delivery guidelines](https://github.com/w3f/General-Grants-Program/blob/master/grants/milestone-deliverables-guidelines.md).**

* **PR Link:** https://github.com/w3f/Grants-Program/pull/311
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This also needs to be updated.

Suggested change
* **PR Link:** https://github.com/w3f/Grants-Program/pull/311
* **Application Document:** https://github.com/w3f/Grants-Program/blob/master/applications/RubyProtocol.md

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, already update it.

@mmagician
Copy link
Copy Markdown
Contributor

@rubyprotocol Before we proceed any further, I've noticed that you have duplicated the entire ZeroPool repo again into your code. This was already pointed out in the previous evaluation, please see here I understand from your tutorial that now it is used to demonstrate the milestone deliverables, but your code should ideally be a library - right now it's really looking just like a "project delivery", where the node, the client and the UI is bundled into one. Please bear in mind that one of the objectives of the Grants Program is to enable other community members to easily import your code and re-use it for their own project.

I would suggest you refactor your code and instead either simply point to the correct repository, giving instructions on how to build and run it (you don't need to duplicate whatever instructions ZeroPool has), or if you have objections to the first method, you can include that as a submodule (while it doesn't really fit an idea of submodule, in my opinion that's still better than including the entirety of the source code). Please re-think your approach to making the client and UI into stand-alone applications with clear, pluggable APIs.

Thanks for understanding.

@lornacrevelingwgo23
Copy link
Copy Markdown

@rubyprotocol Before we proceed any further, I've noticed that you have duplicated the entire ZeroPool repo again into your code. This was already pointed out in the previous evaluation, please see here I understand from your tutorial that now it is used to demonstrate the milestone deliverables, but your code should ideally be a library - right now it's really looking just like a "project delivery", where the node, the client and the UI is bundled into one. Please bear in mind that one of the objectives of the Grants Program is to enable other community members to easily import your code and re-use it for their own project.

I would suggest you refactor your code and instead either simply point to the correct repository, giving instructions on how to build and run it (you don't need to duplicate whatever instructions ZeroPool has), or if you have objections to the first method, you can include that as a submodule (while it doesn't really fit an idea of submodule, in my opinion that's still better than including the entirety of the source code). Please re-think your approach to making the client and UI into stand-alone applications with clear, pluggable APIs.

Thanks for understanding.

Sure, thanks for your comments. Will get it done soon.

@lornacrevelingwgo23
Copy link
Copy Markdown

@rubyprotocol Before we proceed any further, I've noticed that you have duplicated the entire ZeroPool repo again into your code. This was already pointed out in the previous evaluation, please see here I understand from your tutorial that now it is used to demonstrate the milestone deliverables, but your code should ideally be a library - right now it's really looking just like a "project delivery", where the node, the client and the UI is bundled into one. Please bear in mind that one of the objectives of the Grants Program is to enable other community members to easily import your code and re-use it for their own project.

I would suggest you refactor your code and instead either simply point to the correct repository, giving instructions on how to build and run it (you don't need to duplicate whatever instructions ZeroPool has), or if you have objections to the first method, you can include that as a submodule (while it doesn't really fit an idea of submodule, in my opinion that's still better than including the entirety of the source code). Please re-think your approach to making the client and UI into stand-alone applications with clear, pluggable APIs.

Thanks for understanding.

Hey @mmagician, already refactor the code according to your instruction. Please check our repo for the update.

@mmagician
Copy link
Copy Markdown
Contributor

Why are you re-defining the types that you import from fawkes_crypto? This seems overly confusing, especially that sometimes you're re-importing these and you then have both crate-local type definition and original definitions from fawkes_crypto.

@lornacrevelingwgo23
Copy link
Copy Markdown

Why are you re-defining the types that you import from fawkes_crypto? This seems overly confusing, especially that sometimes you're re-importing these and you then have both crate-local type definition and original definitions from fawkes_crypto.

Thanks for your comments. This is actually an optimization of the original private_ml code for milestone 1 delivery. It aims to ensure the consistency of curves used by our functional encryption scheme and zero-knowledge proof scheme of zeropool so that the verification can always be successful.

@mmagician
Copy link
Copy Markdown
Contributor

Hmm okay, I think it's confusing but if you say it's a conscious design decision - that's fine.
Lastly, there are multiple formatting and clippy issues, this was already reported in Milestone 1 evaluation. Please address them all (clippy as well as cargo fmt).

Also, it would be beneficial to see more elaborate tests: e.g. the inner product only takes an array of 1 elements, which is too trivial IMO to constitute a proper test.

@lornacrevelingwgo23
Copy link
Copy Markdown

Reference in

Hmm okay, I think it's confusing but if you say it's a conscious design decision - that's fine. Lastly, there are multiple formatting and clippy issues, this was already reported in Milestone 1 evaluation. Please address them all (clippy as well as cargo fmt).

Also, it would be beneficial to see more elaborate tests: e.g. the inner product only takes an array of 1 elements, which is too trivial IMO to constitute a proper test.

Thanks for your comments. Already fix the clippy and fmt issues. Please check the repo for update. Regarding tests, although our tutorial only gives an example of an array of two elements, our apps work fine with arrays of multiple elements.

@mmagician
Copy link
Copy Markdown
Contributor

Thanks for a quick response. I'm still seeing warnings from both:
cargo clippy and cargo fmt --all --check. Just by skimming through clippy issues, it seems like you can still fix a few of these. Format should definitely be fixed though.
Regarding tests: if it already works with larger inputs, then I guess it shouldn't be a problem to make the tests more realistic?

Thanks for your cooperation.

@takahser
Copy link
Copy Markdown
Contributor

@lornacrevelingwgo23 thanks for fixing, I confirm that there are no clippy issues anymore. Next, I'm planning to take a closer look at your delivery and be back with more detailed feedback soon.

@lornacrevelingwgo23
Copy link
Copy Markdown

@lornacrevelingwgo23 thanks for fixing, I confirm that there are no clippy issues anymore. Next, I'm planning to take a closer look at your delivery and be back with more detailed feedback soon.

Great, looking forward to hearing from you again.

@lornacrevelingwgo23
Copy link
Copy Markdown

@lornacrevelingwgo23 thanks for fixing, I confirm that there are no clippy issues anymore. Next, I'm planning to take a closer look at your delivery and be back with more detailed feedback soon.

Hey @takahser , any update from your side on the feedback? Thanks.

@takahser
Copy link
Copy Markdown
Contributor

takahser commented Mar 22, 2022

Hi @lornacrevelingwgo23
Sorry for the delay here. I checked out your project and followed the instructions in your README.

I was able to run the following services:

  • Authority API Service
  • Data Owner API Service

But they'd both return a 405 Method Not Allowed:
image

I also tried to run:

  • Data Purchaser API Service

But that service doesn't even respond in the first place.
I'm not sure, why the services are not working. Do I miss something?

Also, I tried to set up the zero-pool-node according to your M1 Readme but apparently, the folder mentioned isn't available in the most recent revision of the repo. Could you guide me through the process on how to run the node and ideally also update these instructions?

@lornacrevelingwgo23
Copy link
Copy Markdown

lornacrevelingwgo23 commented Mar 23, 2022

Hi @lornacrevelingwgo23 Sorry for the delay here. I checked out your project and followed the instructions in your README.

I was able to run the following services:

  • Authority API Service
  • Data Owner API Service

But they'd both return a 405 Method Not Allowed: image

I also tried to run:

  • Data Purchaser API Service

But that service doesn't even respond in the first place. I'm not sure, why the services are not working. Do I miss something?

Also, I tried to set up the zero-pool-node according to your M1 Readme but apparently, the folder mentioned isn't available in the most recent revision of the repo. Could you guide me through the process on how to run the node and ideally also update these instructions?

Thanks for the comments. I think the main issue is that the zeropool node is not set up correctly. The readme you referred to belongs to a fork branch of milestone2 repo, which does not include our latest update on the setup instruction. Please follow the readme in the main branch to set up the zeropool node.

After building the API service, run the authority, data owner, and data purchaser's API and then keep all the three terminals open, then set up the zeropool node and run the Ruby frontend. When the dev nodes are running correctly, you can start to test the API on the frontend according to the instruction in tutorial. Please do not try to test the APIs from the terminal. After running the API service commands, you only need to leave all three terminals open and then you can immediately move on to set up the zeropool node.

Please let me know if you have any other questions.

@takahser
Copy link
Copy Markdown
Contributor

@lornacrevelingwgo23 thanks for clarifying. I tried setting up the zeropool node using the updated instructions, however it fails:

% make init
./scripts/init.sh
*** Initializing WASM build environment
info: syncing channel updates for 'nightly-aarch64-apple-darwin'

  nightly-aarch64-apple-darwin unchanged - rustc 1.61.0-nightly (5f3700105 2022-03-22)

info: checking for self-updates
info: syncing channel updates for 'nightly-2020-10-05-aarch64-apple-darwin'
info: latest update on 2020-10-05, rust version 1.49.0-nightly (beb5ae474 2020-10-04)
error: target 'aarch64-apple-darwin' not found in channel.  Perhaps check https://doc.rust-lang.org/nightly/rustc/platform-support.html for available targets
make: *** [init] Error 1

I'm using a MacBook Air (M1, 2020) running on MacOs 12.2.1

@lornacrevelingwgo23
Copy link
Copy Markdown

@lornacrevelingwgo23 thanks for clarifying. I tried setting up the zeropool node using the updated instructions, however it fails:

% make init
./scripts/init.sh
*** Initializing WASM build environment
info: syncing channel updates for 'nightly-aarch64-apple-darwin'

  nightly-aarch64-apple-darwin unchanged - rustc 1.61.0-nightly (5f3700105 2022-03-22)

info: checking for self-updates
info: syncing channel updates for 'nightly-2020-10-05-aarch64-apple-darwin'
info: latest update on 2020-10-05, rust version 1.49.0-nightly (beb5ae474 2020-10-04)
error: target 'aarch64-apple-darwin' not found in channel.  Perhaps check https://doc.rust-lang.org/nightly/rustc/platform-support.html for available targets
make: *** [init] Error 1

I'm using a MacBook Air (M1, 2020) running on MacOs 12.2.1

It's possible that your problem is caused by the M1 chip of your MacBook. Would you please try to set up the zeropool node using either a MacBook with an intel chip or maybe a Ubuntu system?

@lornacrevelingwgo23
Copy link
Copy Markdown

lornacrevelingwgo23 commented Mar 27, 2022

@lornacrevelingwgo23 thanks for clarifying. I tried setting up the zeropool node using the updated instructions, however it fails:

% make init
./scripts/init.sh
*** Initializing WASM build environment
info: syncing channel updates for 'nightly-aarch64-apple-darwin'

  nightly-aarch64-apple-darwin unchanged - rustc 1.61.0-nightly (5f3700105 2022-03-22)

info: checking for self-updates
info: syncing channel updates for 'nightly-2020-10-05-aarch64-apple-darwin'
info: latest update on 2020-10-05, rust version 1.49.0-nightly (beb5ae474 2020-10-04)
error: target 'aarch64-apple-darwin' not found in channel.  Perhaps check https://doc.rust-lang.org/nightly/rustc/platform-support.html for available targets
make: *** [init] Error 1

I'm using a MacBook Air (M1, 2020) running on MacOs 12.2.1

@takahser, Please check this post. Only the rust version released after Nov. 27th, 2020 supports M1. We have updated the init file to ensure it is compatible with M1. Please follow the instruction in the readme and see if it works this time.

@takahser
Copy link
Copy Markdown
Contributor

@lornacrevelingwgo23 thanks for the update.
make init now succeeds with the new instructions, but make run fails with:

% make run

cargo +nightly-2020-10-05 build --release 
error: toolchain 'nightly-2020-10-05-aarch64-apple-darwin' is not installed
make: *** [run] Error 1

I tried installing the missing toolchain but that also failed:

% rustup toolchain add nightly-2020-10-05-aarch64-apple-darwin
info: syncing channel updates for 'nightly-2020-10-05-aarch64-apple-darwin'
info: latest update on 2020-10-05, rust version 1.49.0-nightly (beb5ae474 2020-10-04)
error: target 'aarch64-apple-darwin' not found in channel.  Perhaps check https://doc.rust-lang.org/nightly/rustc/platform-support.html for available targets

@lornacrevelingwgo23
Copy link
Copy Markdown

lornacrevelingwgo23 commented Mar 29, 2022

@lornacrevelingwgo23 thanks for the update. make init now succeeds with the new instructions, but make run fails with:

% make run

cargo +nightly-2020-10-05 build --release 
error: toolchain 'nightly-2020-10-05-aarch64-apple-darwin' is not installed
make: *** [run] Error 1

I tried installing the missing toolchain but that also failed:

% rustup toolchain add nightly-2020-10-05-aarch64-apple-darwin
info: syncing channel updates for 'nightly-2020-10-05-aarch64-apple-darwin'
info: latest update on 2020-10-05, rust version 1.49.0-nightly (beb5ae474 2020-10-04)
error: target 'aarch64-apple-darwin' not found in channel.  Perhaps check https://doc.rust-lang.org/nightly/rustc/platform-support.html for available targets

Hey @takahser , we have tried to update the Rust version to the one released after November, but the zeropool pallet cannot pass the compilation with this rust version. It seems the zeropool pallet only supports the rust version before October. On the other hand, the earliest possible Rust version that supports M1 is the one released after November. You can see the kind of dilemma we are currently in.

Would you please switch to a Macbook with an intel core or maybe a server with a ubuntu system if it's not too much trouble? My own Macbook has an intel core and it works perfectly well with the zeropool pallet, and my other teammate's computer has a ubuntu system and it also works OK. Please consider this option. Thanks a lot.

@lornacrevelingwgo23
Copy link
Copy Markdown

Hey @takahser

Hey @takahser, I don't think there is any way we could make Ruby compatible with M1 Macbook at the moment unless the zeropool folks somehow work things out. That's why I've set up a server with Linux system and I've test the zeropool code myself on it, and it works perfectly well. Would you please send me your email address so that I can send you the password and IP of the Linux server so that you could test Ruby protocol on the server? Thanks.

@takahser
Copy link
Copy Markdown
Contributor

takahser commented Apr 4, 2022

@lornacrevelingwgo23 thanks for your efforts. Yes, under this circumstances it might be easier to solve this via chat/email. I've reached out to you via email.

@lornacrevelingwgo23
Copy link
Copy Markdown

@lornacrevelingwgo23 thanks for your efforts. Yes, under this circumstances it might be easier to solve this via chat/email. I've reached out to you via email.

Hey @takahser , I've already left you a message via Matrix with the info regarding the IP and password of Linux server. Feel free to leave a message there if you have any problem.

@takahser
Copy link
Copy Markdown
Contributor

@lornacrevelingwgo23 thanks for the call yesterday, it was really helpful and the infrastructure now works for me. 👍
However, I just realized that in your delivery some deliverables are missing. The 0b., 0c. seem to be missing and also 1. and 2. do not match the deliverables from your contract. Could you consolidate the deliverables such that they match with these you declared in that grant application? Let me know if anything is unclear.

@rubyprotocol
Copy link
Copy Markdown
Contributor Author

rubyprotocol commented Apr 23, 2022

@lornacrevelingwgo23 thanks for the call yesterday, it was really helpful and the infrastructure now works for me. 👍 However, I just realized that in your delivery some deliverables are missing. The 0b., 0c. seem to be missing and also 1. and 2. do not match the deliverables from your contract. Could you consolidate the deliverables such that they match with these you declared in that grant application? Let me know if anything is unclear.

@takahser , thanks for your comments. Already updated the delivery accordingly. Please check it out.

@takahser
Copy link
Copy Markdown
Contributor

@rubyprotocol thanks for the update, 0a.-0d. are fine now. 👍
However, 1-3 are still missing and the 1 in the delivery seems to be a different deliverable?! Instead of "Client modules" it's called "Personal data monetization framework based on functional encryption and substrate pallet". Also, "2. | Benchmark" and "3. | Docker" are missing, correct?

@lornacrevelingwgo23
Copy link
Copy Markdown

lornacrevelingwgo23 commented Apr 29, 2022

your contract

@rubyprotocol thanks for the update, 0a.-0d. are fine now. 👍

Thanks for your comments.

However, 1-3 are still missing and the 1 in the delivery seems to be a different deliverable?! Instead of "Client modules" it's called "Personal data monetization framework based on functional encryption and substrate pallet".

Hey, I should've called it "Client modules" in the first place. Already change it to "Client modules" in the updated delivery.

Also, "2. | Benchmark" and "3. | Docker" are missing, correct?

The updated delivery also includes the Benchmark result and the dockerfile. Please check it out.

Copy link
Copy Markdown
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.

@lornacrevelingwgo23 Thx for the update, it looks much better indeed. However, I just realized that the LICENSE files are missing in your repos. Keep in mind that you need to include it in case of an APACHE 2.0 license.

@lornacrevelingwgo23
Copy link
Copy Markdown

@lornacrevelingwgo23 Thx for the update, it looks much better indeed. However, I just realized that the LICENSE files are missing in your repos. Keep in mind that you need to include it in case of an APACHE 2.0 license.

Hey @takahser , thanks for your reminder. Already include the APACHE 2.0 license and notice in our delivery.

Copy link
Copy Markdown
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.

@lornacrevelingwgo23 thanks for adding the LICENSE file. Could you also add it to the other repo you referenced in your delivery?

@lornacrevelingwgo23
Copy link
Copy Markdown

@lornacrevelingwgo23 thanks for adding the LICENSE file. Could you also add it to the other repo you referenced in your delivery?

Sure, already added the LICENSE files to all the repo mentioned in the delivery. Please check it out.

Copy link
Copy Markdown
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.

@lornacrevelingwgo23 thanks for adding the LICENSE file to the zeropool repo as well. I've accepted the delivery, you can find my evaluation here: https://github.com/w3f/Grant-Milestone-Delivery/blob/master/evaluations/ruby_protocol_2_takahser.md

@lornacrevelingwgo23
Copy link
Copy Markdown

@lornacrevelingwgo23 thanks for adding the LICENSE file to the zeropool repo as well. I've accepted the delivery, you can find my evaluation here: https://github.com/w3f/Grant-Milestone-Delivery/blob/master/evaluations/ruby_protocol_2_takahser.md

Cool, thanks a lot.

@RouvenP
Copy link
Copy Markdown

RouvenP commented May 16, 2022

hi @lornacrevelingwgo23 we transferred the payment today. Thanks!

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.

7 participants