zksync driver deposit (`enter` CLI command) #1208

pnowosie · 2021-04-06T12:23:37Z

Fixes PAY-64

Steps to test the command:

prepare .env file
provide valid eth node address for ZKSYNC_RINKEBY_GETH_ADDR key (e.g. infura) in the .env
run the service cargo run service run
open new terminal window and there run the following commands
cargo run payment fund --driver erc20
cargo run payment init --sender --driver zksync (will fail but unlocks the wallet of the zksync account)
cargo run payment enter --amount 5 --driver=zksync starts a deposit transaction. When successful will provide a link to track the transaction on the block explorer, e.g. example1 example2
when the transaction completes, funds can be seen with cargo run payment status --driver zksync

On the block explorer for Ethereum testnet you should see also 2 new transactions:

allowing ERC20 token deposit on zksync contract such this one
deposit transaction from L1 to ZkSync contract example

maaktweluit

Great work!

Left some comments, will approve on reply / fix

maaktweluit · 2021-04-12T09:56:55Z

core/payment-driver/zksync/src/zksync/wallet.rs

+        .await
+        .map_err(|err| GenericError::new(err))?;
+    info!(
+        "Check out deposit transaction at\nhttps://rinkeby.etherscan.io/tx/0x{}",


no new line in logs please

not me, cargo fmt is the one to blame ;)

cargo fmt does not need to read the logs, our futures selves do ;)

Oh, I misunderstood your original message - you mean \n new lines I added 😄

core/model/src/driver.rs

core/payment-driver/zksync/src/zksync/signer.rs

pnowosie · 2021-04-13T07:33:48Z

Regarding checking whether ETH balance covers GAS costs. ZkSync's ethereum client don't provide a function to get the actual gas price, their code is hardcoding 300k Wei for the deposit

When enter command is called without funds the following descriptive error is returned

Error: Network error: RPC error: Error { code: ServerError(-32000), message: "insufficient funds for gas * price + value", data: None }

I think it is good enough for now. 🤔 WDYT?

maaktweluit

I think it is good enough for now. 🤔 WDYT?

as long as the input values are logged i think this is sufficient :)

lots of nitty small comments:

only newline and std::env are important IMHO
formatting {:x} might need .as_bytes() or .as_fixed_bytes() to work

i have tested it OK previous review round, will test and approve on reply / fix

core/payment-driver/zksync/examples/deposit.rs

core/payment-driver/zksync/src/zksync/wallet.rs

maaktweluit · 2021-04-13T08:26:34Z

core/payment-driver/zksync/src/zksync/wallet.rs

+        .await
+        .map_err(|err| GenericError::new(err))?;
+    info!(
+        "Check out deposit transaction at\nhttps://rinkeby.etherscan.io/tx/0x{}",


cargo fmt does not need to read the logs, our futures selves do ;)

maaktweluit · 2021-04-13T08:28:47Z

core/payment-driver/zksync/src/zksync/signer.rs

@@ -105,3 +113,56 @@ fn sign_tx(
    });
    Box::pin(fut)
 }
+
+pub fn encode_signed_tx(raw_tx: &RawTransaction, signature: Vec<u8>, chain_id: u64) -> Vec<u8> {


does this need to be pub?

we can also move this to the base crate to share logic with gnt-driver ( i know i recommended against before, but now i see the need )

core/model/src/driver.rs

core/payment-driver/zksync/src/driver.rs

Wiezzel · 2021-04-13T14:45:59Z

.env-template

@@ -52,6 +52,9 @@ YAGNA_DATADIR="."
 ## ZkSync driver
 #ZKSYNC_RINKEBY_RPC_ADDRESS=https://rinkeby-api.zksync.io/jsrpc
 #ZKSYNC_MAINNET_RPC_ADDRESS=https://api.zksync.io/jsrpc
+#ZKSYNC_RINKEBY_GETH_ADDR=http://1.geth.testnet.golem.network:55555
+#ZKSYNC_MAINNET_GETH_ADDR=http://geth.golem.network:55555


Not sure if we should have separate env variables here. Are there legit cases when we would use different geth nodes for different drivers?

It was suggested by @maaktweluit

Even when we already have ERC20_MAINNET_GETH_ADDRESS and the RINKEBY version,
they are placed under erc20 driver section, so I think we can either:

prepare driver unspecific geth node address variables (supporting all cases)

leave it as is.

WDYT?

I would rename he variables and use them in both drivers.
ERC20_RINKEBY_GETH_ADDR -> RINKEBY_GETH_ADDR
ERC20_MAINNET_GETH_ADDR -> MAINNET_GETH_ADDR

core/payment-driver/zksync/src/zksync/wallet.rs

Adds files necessary for running goth tests as part of yagna repository. This also includes moving the VM E2E test from goth to yagna.

maaktweluit

LGTM!

maaktweluit suggested changes Apr 12, 2021

View reviewed changes

pnowosie force-pushed the pnowosie/pay-65-zksync-driver-deposit branch from cd4cd8e to f7208de Compare April 13, 2021 06:40

pnowosie requested review from maaktweluit and Wiezzel April 13, 2021 07:33

pnowosie marked this pull request as ready for review April 13, 2021 07:34

pnowosie requested a review from a team April 13, 2021 07:34

maaktweluit suggested changes Apr 13, 2021

View reviewed changes

Wiezzel reviewed Apr 13, 2021

View reviewed changes

pnowosie requested review from Wiezzel and maaktweluit April 14, 2021 07:28

Wiezzel reviewed Apr 14, 2021

View reviewed changes

core/payment-driver/zksync/src/zksync/wallet.rs Outdated Show resolved Hide resolved

core/payment-driver/zksync/src/zksync/wallet.rs Outdated Show resolved Hide resolved

core/payment-driver/zksync/src/zksync/wallet.rs Outdated Show resolved Hide resolved

pnowosie requested a review from Wiezzel April 14, 2021 12:44

kmazurek added 6 commits April 14, 2021 22:37

Set up goth integration tests

21422a2

Adds files necessary for running goth tests as part of yagna repository. This also includes moving the VM E2E test from goth to yagna.

Add dev-dependencies for goth tests

6a57ff0

Add --config-override option to goth tests

c87a55b

Add VM test assets directory

a418a81

Merge branch 'master' into km/goth-tests

4af22a5

Include upload dir in VM assets

eef9fcc

Wiezzel previously approved these changes Apr 19, 2021

View reviewed changes

Paweł Nowosielski added 8 commits April 19, 2021 11:23

add deposit as in the std example

95b7563

token needs to be approved before deposit

68315fc

feat: enter command

8b6453a

feat: signing transactions works

75bd40d

[after review] Gert-Jan comments

042ead8

[after review] Gert-Jan suggestions /2

b3370d2

[after review] Adam comments

34b0834

Adam's comments /2

bfaedbe

pnowosie dismissed Wiezzel’s stale review via bfaedbe April 19, 2021 10:31

pnowosie force-pushed the pnowosie/pay-65-zksync-driver-deposit branch from 36b68cb to bfaedbe Compare April 19, 2021 10:31

kmazurek added 5 commits April 19, 2021 16:00

Add e2e WASI integration test

ef910a2

Add ya-provider multi-activity integration test

24e32fc

Add payments integration tests

d501355

Specify goth patch version explicitly

04f7a99

Update poetry.lock

6527a2e

pnowosie requested a review from Wiezzel April 20, 2021 07:07

Wiezzel previously approved these changes Apr 20, 2021

View reviewed changes

Add integration tests readme

99b92cb

maaktweluit previously approved these changes Apr 20, 2021

View reviewed changes

kmazurek and others added 4 commits April 21, 2021 10:10

Review: update integration tests readme

31cfad0

Review: make config_overrides non-optional

889aeac

Review: move task packages to activity helpers module

09dfb86

Merge branch 'km/goth-tests' into pnowosie/pay-65-zksync-driver-deposit

0271908

pnowosie dismissed stale reviews from maaktweluit and Wiezzel via 0271908 April 21, 2021 12:37

pnowosie requested a review from a team April 21, 2021 12:37

Merge branch 'master' into pnowosie/pay-65-zksync-driver-deposit

1bb822a

pnowosie removed the request for review from a team April 21, 2021 19:48

pnowosie enabled auto-merge (squash) April 22, 2021 05:17

Wiezzel approved these changes Apr 22, 2021

View reviewed changes

pnowosie merged commit 793e700 into master Apr 22, 2021

pnowosie deleted the pnowosie/pay-65-zksync-driver-deposit branch April 22, 2021 07:24

kmazurek mentioned this pull request May 13, 2021

Update GETH_ADDR environment variable name golemfactory/goth#504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zksync driver deposit (`enter` CLI command) #1208

zksync driver deposit (`enter` CLI command) #1208

pnowosie commented Apr 6, 2021 •

edited

Loading

maaktweluit left a comment

maaktweluit Apr 12, 2021

pnowosie Apr 13, 2021

maaktweluit Apr 13, 2021

pnowosie Apr 13, 2021

pnowosie commented Apr 13, 2021

maaktweluit left a comment

maaktweluit Apr 13, 2021

maaktweluit Apr 13, 2021

Wiezzel Apr 13, 2021

pnowosie Apr 14, 2021 •

edited

Loading

Wiezzel Apr 14, 2021

maaktweluit left a comment

zksync driver deposit (enter CLI command) #1208

zksync driver deposit (enter CLI command) #1208

Conversation

pnowosie commented Apr 6, 2021 • edited Loading

maaktweluit left a 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

pnowosie commented Apr 13, 2021

maaktweluit left a 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

pnowosie Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maaktweluit left a comment

Choose a reason for hiding this comment

zksync driver deposit (`enter` CLI command) #1208

zksync driver deposit (`enter` CLI command) #1208

pnowosie commented Apr 6, 2021 •

edited

Loading

pnowosie Apr 14, 2021 •

edited

Loading