ZK Rollups Pallet Milestone1 #93

SotaWatanabe · 2021-02-02T06:40:19Z

Milestone Delivery Checklist

The milestone-delivery-template.md has been copied and updated.
The invoice form 📝 has been filled out for this milestone.
This pull request is done by the same account, which is responsible for the pull request of the accepted application.
In the case of acceptance, the payment will be transferred to the initial BTC payment address.
The delivery is according to the milestone delivery guidelines.

Zk rollup milestone 1

mmagician · 2021-02-12T14:17:07Z

@SotaWatanabe At the moment the test container is failing to resolve some dependencies, see the output:

test         | Prover is working
yarn run v1.22.5
$ mocha -r ts-node/register 'tests/**/test-integration.ts'
test         |
test         | TSError: ⨯ Unable to compile TypeScript:
test         | tests/zksync/src/transport.ts:1:19 - error TS2307: Cannot find module 'axios' or its corresponding type declarations.
test         |
test         | 1 import Axios from 'axios';
test         |                     ~~~~~~~
test         | tests/zksync/src/transport.ts:2:38 - error TS2307: Cannot find module 'websocket-as-promised' or its corresponding type declarations.
test         |
test         | 2 import WebSocketAsPromised = require('websocket-as-promised');

It seems that both axios and websocket-as-promised are part of the package.json dependency list, so I'm not sure why it's not working. Since I'm not an expert on mocha and how it handles packages, I kindly ask for support on this issue.

mmagician · 2021-02-12T15:31:16Z

Below you'll find further comments from the codebase evaluation:

ts-tests sub-directory is largely copied & restructured from matter-labs/zksync & no references nor acknowledgements are provided as far as I could see.

Additionally, the since the zksync repo (a fork thereof) is already a submodule and contains the exact same files (or is there a difference?), why duplicate it into a separate top level ts-tests?

Regarding the concrete milestone 1 deliverables:

Integration Test On Substrate | Test for all contracts and sidechain network actors

other than setup-wallet/contract-test.ts & the tests from matter-labs/zksync, are there any other original tests provided?

ashWhiteHat · 2021-02-14T01:23:28Z

@SotaWatanabe At the moment the test container is failing to resolve some dependencies, see the output:
test         | Prover is working
yarn run v1.22.5
$ mocha -r ts-node/register 'tests/**/test-integration.ts'
test         |
test         | TSError: ⨯ Unable to compile TypeScript:
test         | tests/zksync/src/transport.ts:1:19 - error TS2307: Cannot find module 'axios' or its corresponding type declarations.
test         |
test         | 1 import Axios from 'axios';
test         |                     ~~~~~~~
test         | tests/zksync/src/transport.ts:2:38 - error TS2307: Cannot find module 'websocket-as-promised' or its corresponding type declarations.
test         |
test         | 2 import WebSocketAsPromised = require('websocket-as-promised');
It seems that both axios and websocket-as-promised are part of the package.json dependency list, so I'm not sure why it's not working. Since I'm not an expert on mocha and how it handles packages, I kindly ask for support on this issue.

It didn't happen in my environment.
Let me check this on Github Actions🚀

ts-tests sub-directory is largely copied & restructured from matter-labs/zksync & no references nor acknowledgements are provided as far as I could see.

Thank you for the detailed review.
I copied a lot of parts from matter-labs/zksync/sdk/zksync.js and zksync/core/tests/ts-tests/tests, not whole matter-labs/zksync.
The reason why I copied was to import and customize some modules needed for the integration test because these didn't work by just importing as a reference.

Mainly, I modified the following modules.

getDefaultProvider
I modified it in order to use the operator network.
Before and After
etherTransaction
I modified the way to get etherTransaction because frontier can't use socket to get transaction hash so I changed the code getting etherTransaction after finalizing block and getting transaction using transaction hash.
Before and After
fundedWallet
I modified the way to transfer ether because etherjs socket can't use on frontier.
I changed ethersjs handle wait to web3 transaction and finalize manually.
Before and After

other than setup-wallet/contract-test.ts & the tests from matter-labs/zksync, are there any other original tests provided?

I did an integration test as same as matter-labs do.
The matter-labs tests all components with zk test i server command in their document.
The command link workspace ts-tests and executes zksync/core/tests/ts-tests/tests/main.test.ts.
They provide only zksync/core/tests/ts-tests/tests/main.test.ts for integration test.

Sorry for writing such a long message😘
Please confirm.
Thank you!

ashWhiteHat · 2021-02-16T10:29:26Z

Hi @mmagician .

At the moment the test container is failing to resolve some dependencies, see the output:

I did an integration test on Github Actions and the same error happened.
Thank you for letting me know.
And I fixed that issue and Github Actions worked correctly at the latest change.
https://github.com/PlasmNetwork/ZKRollups/runs/1909749575

Please confirm.
Thank you!

mmagician · 2021-02-16T15:39:48Z

I can see your integration test passing. Just after a quick look I see that you've changed the startup process slightly - could you please also update the docs / READMEs accordingly?

ashWhiteHat · 2021-02-17T02:15:33Z

I can see your integration test passing. Just after a quick look I see that you've changed the startup process slightly - could you please also update the docs / READMEs accordingly?

Thank you for your check.
I changed some code but it's mainly for Github Actions so we can check locally with the same command and doc.

ashWhiteHat · 2021-02-25T00:27:41Z

Hi @mmagician
Is there any point I need to fix?

mmagician · 2021-02-25T02:31:50Z

@noctrlz apologies for the delay. So far so good, but I would still like to test a couple more things. Please allow a couple of extra days for the review. Thanks!

ashWhiteHat · 2021-02-28T22:17:07Z

Hi @mmagician
Is everything going well?

SotaWatanabe · 2021-03-04T01:29:58Z

Thank you @noctrlz for the update.

Hello Web3 Foundation team, @mmagician, if you need it, we are more than happy to set up a call and explain how it works and show you a demo so that you can understand the status and technical architecture. ZK Rollups is obviously new and not easy to understand. I hope this may help you understand what we have done on Substrate.

semuelle · 2021-03-06T18:04:40Z

Hi @SotaWatanabe & @noctrlz,
thanks for the message and sorry for the slow response. The W3F grants program is very popular these days and we are working hard to keep up with evaluations. I'm sure we will be able to dig further into your delivery this coming week.

If there is further need for a demo or setup instructions, we will communicate that here.

ashWhiteHat · 2021-03-07T00:46:28Z

Thank you for the answer!
Please take your time.

mmagician · 2021-03-11T18:43:16Z

Apologies for the delay in reviewing your code.

The only problem I see with accepting this milestone is code attribution, namely what I mentioned earlier about largely copying the ts-tests directory.
I wonder, since you are already forking the zksync repo, why not modify the ts-tests there, and use these for your integration tests? This way:

It is clear that you didn't write all the tests yourself and everyone can easily do a diff between your fork and the original
You avoid code duplication, as right now you have a zksync submodule + an extra ts-tests directory in root.

Lastly, adding the defaultProvider operator is very docker-specific, as this is bound to work for your setup only, where the zkSync server container is called operator. I wonder if you could re-work the network setup of your containers? I would suggest exploring the concept of host networking in docker - I think this way you can avoid setup-specific changes to the codebase and instead limit the changes to your docker files.

ashWhiteHat · 2021-03-15T09:21:44Z

Hi @mmagician.
Thank you for the review!
I changed the structure according to your review.

It is clear that you didn't write all the tests yourself and everyone can easily do a diff between your fork and the original
You avoid code duplication, as right now you have a zksync submodule + an extra ts-tests directory in root.

I merged zksync to one to avoid duplication and it's referred by the operator and test containers.

Lastly, adding the defaultProvider operator is very docker-specific, as this is bound to work for your setup only, where the zkSync server container is called operator

I fixed this problem and there are no docker-specific defaultProvider.

I modified the documentation accordingly and made the github actions test pass.
https://github.com/PlasmNetwork/ZKRollups/runs/2111077264

Please confirm🚀

mmagician · 2021-03-15T09:38:29Z

@noctrlz Thanks for the updates. I'll let you know once I've reviewed your changes

mmagician · 2021-03-17T10:57:28Z

The structure looks much better now, thank you!
Now sure whether it's because of the changes you introduced, but now I'm getting the following error when executing the tests:

test         |        "before all" hook: create tester and test wallets for "should execute an auto-approved deposit":
test         |      Error: connect ECONNREFUSED 172.21.0.5:3030

Let me know if you are able to reproduce that?

ashWhiteHat · 2021-03-18T10:04:48Z

Hi @mmagician.
Thank you for the review.
It seemed like I forgot to remove debug operation.
I fixed and it works now.
Please confirm!

mmagician · 2021-03-18T12:24:54Z

@noctrlz I just pulled & updated the submodules, but I'm still seeing the same error.

ashWhiteHat · 2021-03-18T14:31:13Z

@mmagician
I think it will work after you execute $ docker-compose rm && sh scripts/integration.sh.
If it doesn't work, please give me the full log.
In my environment, it worked without any cache.
Thank you!

mmagician · 2021-03-18T19:05:29Z

It worked! I can already tell you that the first milestone is accepted, congrats! I'll submit an official evaluation PR soon.

ashWhiteHat · 2021-03-18T23:22:17Z

@mmagician
Thank you so much!

mmagician · 2021-03-22T08:07:12Z

@SotaWatanabe @noctrlz Please find the evaluation notes here

mmagician · 2021-03-22T14:06:00Z

@SotaWatanabe @noctrlz We noticed that the invoice you submitted contains a different payment address than the original contract.
As you'll find in our application template:
The combination of your GitHub account submitting the application and the payment address above will be your unique identifier during the program. Please keep them safe.
If you want to change your payment address, we would require you to amend the original contract via a PR.

SotaWatanabe · 2021-03-22T16:58:40Z

Hello Marcin, Thank you very much for notifying us. I will review it as soon as possible and get it back to you. Thank you. 2021年3月22日(月) 22:06 Marcin ***@***.***>:

…

@SotaWatanabe <https://github.com/SotaWatanabe> @noctrlz <https://github.com/NoCtrlZ> We noticed that the invoice you submitted contains a different payment address than the original contract. As you'll find in our application template: The combination of your GitHub account submitting the application and the payment address above will be your unique identifier during the program. Please keep them safe. If you want to change your payment address, we would require you to amend the original contract via a PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#93 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG77XSHOSFGBGAZVLBAH5HTTE5FFZANCNFSM4W6CMEEQ> .

SotaWatanabe · 2021-03-23T11:49:26Z

@mmagician Sorry for the delay. Recently, we moved our HQ from Japan to Singapore and now this grant is operated by the Singapore entity. This is why we have 2 different bitcoin addresses. I will create the PR and send it to you shortly. Sorry for the inconvenience.

SotaWatanabe · 2021-03-23T15:03:33Z

I just amend our bitcoin address via a PR. Please check it when you have time. Thank you.
w3f/Grants-Program#330 (comment)

* copy template * describe for each deliverable * remove unnecessary file Co-authored-by: NoCtrlZ <[email protected]> Co-authored-by: Shinsaku Ashizawa <[email protected]>

ashWhiteHat and others added 4 commits February 1, 2021 18:00

copy template

7dffbab

describe for each deliverable

7f409f5

remove unnecessary file

27d1cc7

Merge pull request #1 from PlasmNetwork/zk-rollup-milestone-1

318da95

Zk rollup milestone 1

SotaWatanabe mentioned this pull request Feb 2, 2021

ZK Rollups Pallet Application w3f/Grants-Program#92

Merged

4 tasks

semuelle assigned mmagician Feb 2, 2021

mmagician added the changes requested label Feb 12, 2021

mmagician merged commit 50e9fb4 into w3f:master Mar 22, 2021

SotaWatanabe mentioned this pull request Mar 23, 2021

ZK Rollups Pallet Milestone1 Amend our bitcoin address w3f/Grants-Program#330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZK Rollups Pallet Milestone1 #93

ZK Rollups Pallet Milestone1 #93

SotaWatanabe commented Feb 2, 2021 •

edited

Loading

mmagician commented Feb 12, 2021

mmagician commented Feb 12, 2021

ashWhiteHat commented Feb 14, 2021 •

edited

Loading

ashWhiteHat commented Feb 16, 2021 •

edited

Loading

mmagician commented Feb 16, 2021

ashWhiteHat commented Feb 17, 2021 •

edited

Loading

ashWhiteHat commented Feb 25, 2021

mmagician commented Feb 25, 2021

ashWhiteHat commented Feb 28, 2021

SotaWatanabe commented Mar 4, 2021

semuelle commented Mar 6, 2021

ashWhiteHat commented Mar 7, 2021

mmagician commented Mar 11, 2021

ashWhiteHat commented Mar 15, 2021

mmagician commented Mar 15, 2021

mmagician commented Mar 17, 2021

ashWhiteHat commented Mar 18, 2021

mmagician commented Mar 18, 2021 •

edited

Loading

ashWhiteHat commented Mar 18, 2021 •

edited

Loading

mmagician commented Mar 18, 2021

ashWhiteHat commented Mar 18, 2021

mmagician commented Mar 22, 2021

mmagician commented Mar 22, 2021

SotaWatanabe commented Mar 22, 2021 via email

SotaWatanabe commented Mar 23, 2021 •

edited

Loading

SotaWatanabe commented Mar 23, 2021

ZK Rollups Pallet Milestone1 #93

ZK Rollups Pallet Milestone1 #93

Conversation

SotaWatanabe commented Feb 2, 2021 • edited Loading

Milestone Delivery Checklist

mmagician commented Feb 12, 2021

mmagician commented Feb 12, 2021

ashWhiteHat commented Feb 14, 2021 • edited Loading

ashWhiteHat commented Feb 16, 2021 • edited Loading

mmagician commented Feb 16, 2021

ashWhiteHat commented Feb 17, 2021 • edited Loading

ashWhiteHat commented Feb 25, 2021

mmagician commented Feb 25, 2021

ashWhiteHat commented Feb 28, 2021

SotaWatanabe commented Mar 4, 2021

semuelle commented Mar 6, 2021

ashWhiteHat commented Mar 7, 2021

mmagician commented Mar 11, 2021

ashWhiteHat commented Mar 15, 2021

mmagician commented Mar 15, 2021

mmagician commented Mar 17, 2021

ashWhiteHat commented Mar 18, 2021

mmagician commented Mar 18, 2021 • edited Loading

ashWhiteHat commented Mar 18, 2021 • edited Loading

mmagician commented Mar 18, 2021

ashWhiteHat commented Mar 18, 2021

mmagician commented Mar 22, 2021

mmagician commented Mar 22, 2021

SotaWatanabe commented Mar 22, 2021 via email

SotaWatanabe commented Mar 23, 2021 • edited Loading

SotaWatanabe commented Mar 23, 2021

SotaWatanabe commented Feb 2, 2021 •

edited

Loading

ashWhiteHat commented Feb 14, 2021 •

edited

Loading

ashWhiteHat commented Feb 16, 2021 •

edited

Loading

ashWhiteHat commented Feb 17, 2021 •

edited

Loading

mmagician commented Mar 18, 2021 •

edited

Loading

ashWhiteHat commented Mar 18, 2021 •

edited

Loading

SotaWatanabe commented Mar 23, 2021 •

edited

Loading