feat: improve zk init perfromance #1774

Raid5594 · 2024-04-23T15:07:26Z

What ❔

Avoiding unnecessary compilation of smart contracts
Fixes EVM-551

Why ❔

Hardhat cache is not working as expected or scripts are being use for compilation

Checklist

PR title corresponds to the body of PR (we generate changelog entries from PRs).
Tests for the changes have been added / updated.
Documentation comments have been added / updated.
Code has been formatted via zk fmt and zk lint.
Spellcheck has been run via zk spellcheck.
Linkcheck has been run via zk linkcheck.

kelemeno · 2024-04-23T15:54:46Z

core/tests/ts-integration/scripts/build.ts

+
+const CONTRACTS_DIR = 'contracts';
+const OUTPUT_DIR = 'artifacts-zk';
+const TIMESTAMP_FILE = 'last_compilation.timestamp'; // File to store the last compilation time


I think is used in multiple places, maybe you can have them in utils and import them?

I think it follows the same logic as the lines above, then we'd want to put CONTRACTS_DIR and OUTPUT_DIR in utils as well

Yes, very good idea

kelemeno · 2024-04-23T16:09:42Z

Also kl-factory has been merged, please rebase on and merge main

kelemeno · 2024-04-23T16:10:32Z

infrastructure/zk/src/server.ts

@@ -46,20 +45,7 @@ export async function externalNode(reinit: boolean = false, args: string[]) {

 async function create_genesis(cmd: string) {
    await utils.confirmAction();
-    await utils.spawn(`${cmd} | tee genesis.log`);
-
-    const date = new Date();


Should we not keep this?

There was a discussion where it seems that we're not using these logs anymore

@mm-zk is it fine if we remove these log from here?

AFAIK yes. @Deniallugo - FYI.

i don't think we need this function at all

…ld fails

mm-zk · 2024-04-24T13:38:43Z

infrastructure/zk/src/server.ts

@@ -46,20 +45,7 @@ export async function externalNode(reinit: boolean = false, args: string[]) {

 async function create_genesis(cmd: string) {
    await utils.confirmAction();
-    await utils.spawn(`${cmd} | tee genesis.log`);
-
-    const date = new Date();


AFAIK yes. @Deniallugo - FYI.

mm-zk · 2024-04-24T13:39:52Z

core/tests/ts-integration/scripts/build.ts

+    TIMESTAMP_FILE
+} from './utils';
+
+async function main() {


please add comment - describing what we're doing here (using some .timestamp file to record the last time that files were changed)

seems that it was not done.
I'm looking for a 2-line comment, where reader can quickly see what's special about this 'build' script, without having to read the details of the code.

I'm not sure it's the right move.
We could solve in two ways:

Move l2 contracts compilation to foundry zksync

Use the prebuild contracts from the repo. we do pulbish built contracts

mm-zk · 2024-04-24T13:41:14Z

core/tests/ts-integration/scripts/utils.ts

+// Determine if recompilation is needed
+export function needsRecompilation(folder: string, timestampFile: string): boolean {
+    const lastCompilationTime = getLastCompilationTime(timestampFile);
+    const latestModificationTime = getLatestModificationTime(folder);


do these sources have any dependencies on outside dirs?

mm-zk · 2024-04-25T07:30:03Z

core/tests/ts-integration/package.json

@@ -9,8 +9,8 @@
    "fee-test": "RUN_FEE_TEST=1 zk f jest -- fees.test.ts",
    "api-test": "zk f jest -- api/web3.test.ts api/debug.test.ts",
    "contract-verification-test": "zk f jest -- api/contract-verification.test.ts",
-    "build": "hardhat compile",
-    "build-yul": "hardhat run scripts/compile-yul.ts"
+    "build": "yarn ts-node scripts/build.ts",


nit: do you actually have to say 'yarn' here? I thought that "ts-node XX" should be enough.

both will work, however 'yarn ts-node' is a safer choice in monorepo environment as it ensure execution in the context of yarn workspace.

mm-zk · 2024-04-25T07:31:00Z

core/tests/ts-integration/scripts/build.ts

+    TIMESTAMP_FILE
+} from './utils';
+
+async function main() {


seems that it was not done.
I'm looking for a 2-line comment, where reader can quickly see what's special about this 'build' script, without having to read the details of the code.

mm-zk · 2024-04-25T07:32:34Z

etc/contracts-test-data/scripts/utils.ts

@@ -0,0 +1,159 @@
+import * as fs from 'fs';


you are repeating the same file twice, that's not good, as it will be harder to manage and update in the future.

Could it be some shared library instead?

switch to sync call, so it's 1 library

mm-zk · 2024-04-25T07:33:05Z

etc/contracts-test-data/scripts/build.ts

+import { exec } from 'child_process';
+
+async function main() {
+    const timestampFilePath = path.join(process.cwd(), TIMESTAMP_FILE); // File stores the timestamp of last compilation


you are repeating this twice (as it already exists in the other build.rs) - maybe move it to a shared util instead?

That's right, I was thinking of it, but as these are different workspaces, I was not sure if a shared util is a good idea

EmilLuta · 2024-05-03T12:32:05Z

Folks, what's the status of this PR?

Raid5594 changed the title ~~Kl factory optimise init~~ feat: improve zk init perfromance Apr 23, 2024

Raid5594 requested a review from kelemeno April 23, 2024 15:42

kelemeno reviewed Apr 23, 2024

View reviewed changes

Raid5594 changed the base branch from kl-factory to main April 23, 2024 16:57

Raid5594 requested a review from a team as a code owner April 23, 2024 16:57

Raid5594 requested review from yorik, alexandrst88, artmakh, hatemosphere, onyxet and otani88 April 23, 2024 16:57

Raid5594 changed the base branch from main to kl-factory April 23, 2024 17:08

Raid Ateir added 3 commits April 23, 2024 19:12

update scripts for optimised compilation & throw error if genesis bui…

82515a6

…ld fails

zk fmt

9e7d44c

zk lint

5ca72d9

Raid5594 force-pushed the kl-factory-optimise-init branch from 98bd13e to 5ca72d9 Compare April 23, 2024 17:12

Raid5594 changed the base branch from kl-factory to main April 23, 2024 17:12

update gitignore

7f0d1e3

Raid5594 force-pushed the kl-factory-optimise-init branch 2 times, most recently from 1aebd66 to 7f0d1e3 Compare April 24, 2024 11:35

refactor consts in scripts

fef0a60

Raid5594 requested review from mm-zk and kelemeno April 24, 2024 12:16

mm-zk reviewed Apr 24, 2024

View reviewed changes

refactor latest mod time function to account for imports

528e7c2

Raid5594 requested a review from mm-zk April 24, 2024 16:01

mm-zk requested changes Apr 25, 2024

View reviewed changes

update comments and fs imports

18da342

Raid5594 requested a review from mm-zk April 25, 2024 09:11

Raid Ateir and others added 3 commits April 25, 2024 12:52

fix ts error

1b324fa

Merge branch 'main' into kl-factory-optimise-init

6f1c469

Merge branch 'main' into kl-factory-optimise-init

fd096fa

RomanBrodetski requested a review from Deniallugo May 6, 2024 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve zk init perfromance #1774

feat: improve zk init perfromance #1774

Raid5594 commented Apr 23, 2024 •

edited

kelemeno Apr 23, 2024

Raid5594 Apr 23, 2024

kelemeno Apr 23, 2024

Raid5594 Apr 24, 2024

kelemeno commented Apr 23, 2024

kelemeno Apr 23, 2024

Raid5594 Apr 23, 2024

Raid5594 Apr 24, 2024

mm-zk Apr 24, 2024

Deniallugo May 6, 2024

mm-zk Apr 24, 2024

mm-zk Apr 24, 2024

Raid5594 Apr 24, 2024

mm-zk Apr 25, 2024

Raid5594 Apr 25, 2024

Deniallugo May 6, 2024

mm-zk Apr 24, 2024

Raid5594 Apr 24, 2024

mm-zk Apr 25, 2024

Raid5594 Apr 25, 2024

mm-zk Apr 25, 2024

mm-zk Apr 25, 2024

Raid5594 Apr 25, 2024

mm-zk Apr 25, 2024

Raid5594 Apr 25, 2024

EmilLuta commented May 3, 2024

feat: improve zk init perfromance #1774

Are you sure you want to change the base?

feat: improve zk init perfromance #1774

Conversation

Raid5594 commented Apr 23, 2024 • edited

What ❔

Why ❔

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelemeno commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmilLuta commented May 3, 2024

Raid5594 commented Apr 23, 2024 •

edited