Add support ERC20 to Native #85

patitonar · 2018-09-26T14:34:05Z

This PR includes the following changes:

Add ERC20-to-Native abis
Add workers support for ERC20-to-Native mode
Update related scripts to support new mode
Update README, including fixes proposed on Update README #79
Add e2e test for ERC20-to-Native

Once omni/tokenbridge-contracts#82 is merged, we should update contract submodule and e2e/Dockerfile to point to correct contract branch.

Closes #81 , closes #79

fvictorio · 2018-09-26T17:14:13Z

Just tested it and everything seemed to work fine.

akolotov

The gas priced received from the oracle is in GWei whereas the gas price from the contract is in Wei:

https://github.com/poanetwork/bridge-nodejs/blob/8359556d9e9976a9388720d97de70e1bd7b35af8/src/services/gasPrice.js#L43-L57

It will cause incorrect conversion in the following code:

https://github.com/poanetwork/bridge-nodejs/blob/8359556d9e9976a9388720d97de70e1bd7b35af8/src/tx/sendTx.js#L18-L29

It make sense to have the fallback gas price in the bridge config set in Wei as well.

Therefore consider to modify fetchGasPriceFromOracle to return the value converted from GWei to Wei.

patitonar · 2018-09-26T19:20:05Z

Thanks @akolotov , good catch! I updated sendTx to remove conversion from Gwei, updated fetchGasPriceFromOracle to return value converted to Wei, updated scripts to pass gasPrice in Wei and set fallback env variables in Wei. Please check last commit

# Conflicts: # README.md

# Conflicts: # e2e/Dockerfile

akolotov · 2018-10-10T19:52:13Z

Since you have merged new README.md file please update it with new meaning of configuration parameters (e.g. HOME_GAS_PRICE_FALLBACK and FOREIGN_GAS_PRICE_FALLBACK are now in wei).

akolotov · 2018-10-12T15:14:33Z

scripts/erc20_to_erc20/sendHome.js

@@ -80,7 +80,7 @@ async function main() {
        privateKey: USER_ADDRESS_PRIVATE_KEY,
        data,
        nonce,
-        gasPrice: '1',
+        gasPrice: '1000000000',


Can we introduce a parameter to configure the gas price which will be used for testing transactions?

Done. Added HOME_TEST_TX_GAS_PRICE and FOREIGN_TEST_TX_GAS_PRICE

akolotov · 2018-10-12T15:15:51Z

scripts/erc20_to_native/sendForeign.js

+  USER_ADDRESS_PRIVATE_KEY,
+  FOREIGN_BRIDGE_ADDRESS,
+  FOREIGN_MIN_AMOUNT_PER_TX,
+  ERC20_TOKEN_ADDRESS


Can ERC20_TOKEN_ADDRESS will be extracted from the bridge contract?

scripts/erc20_to_native/sendHome.js

akolotov · 2018-10-12T15:22:39Z

.env.example

@@ -13,12 +13,12 @@ VALIDATOR_ADDRESS_PRIVATE_KEY=

 HOME_GAS_PRICE_ORACLE_URL=https://gasprice.poa.network/


Here is one of the comment we received during the security audit is: https://hackmd.io/s/Sk6P7Uqtm#2-Bad-private-data-storing.
It is related to an environment when watcher and senders are run in the docker containers.
In order to address the comment I suggest the following:

For the environment when the watchers and senders are run on the same system (not containerized) VALIDATOR_ADDRESS is redundant - it can be calculated from the VALIDATOR_ADDRESS_PRIVATE_KEY. The official recommendation will be to put have VALIDATOR_ADDRESS_PRIVATE_KEY only in the configuration file. Moreover it will reduce the number of issues when a developer puts the validator address which does not correspond to the private key.

If the watcher finds that VALIDATOR_ADDRESS_PRIVATE_KEY is not configured (it means that it run in the container), it should use VALIDATOR_ADDRESS.

As soon as these changes are done, we will re-write the deployment playbooks as so they will store VALIDATOR_ADDRESS_PRIVATE_KEY in sender's containers only. Also the bridge startup script will calculate VALIDATOR_ADDRESS from VALIDATOR_ADDRESS_PRIVATE_KEY programmatically and initialize the watcher's contaners as so they will have VALIDATOR_ADDRESS without mentioning VALIDATOR_ADDRESS_PRIVATE_KEY.

What do you think?

Let me see if I understood this correctly:

Senders extract the VALIDATOR_ADDRESS_PRIVATE_KEY from the environment variables and use it to calculate the validator address.

Watchers extract VALIDATOR_ADDRESS from the environment: this will be injected by the startup script. Also watchers should work fine if the private key is not present in the environment variables (since they don't use it anyway)

Did I get this right?

Right.
And watchers should work in the case if VALIDATOR_ADDRESS is not specified (the case when watchers and senders are run in the same environment - not containerized) - VALIDATOR_ADDRESS_PRIVATE_KEY is used by watchers to calculate the validator address.

I see, thanks!

@fvictorio when could we expect these set of changes? I need to send them for review to the security auditor to response on the opened finding.

Done in 6575f0f, but notice that the signature requests watcher still needs the private key:

https://github.com/poanetwork/token-bridge/blob/2fad9b9cca14b3eedbcecc8c931c845906c70bc4/src/events/processSignatureRequests/index.js#L60

Do the watchers have to work without the private key? Maybe there's some way to rewrite this so we don't need it.

Ok. Let's leave the ability to work with the private key for the signature-requests watcher. Later we will think how to split this logic.

scripts/native_to_erc20/sendForeign.js

scripts/erc20_to_erc20/sendForeign.js

scripts/erc20_to_erc20/sendHome.js

patitonar · 2018-10-25T14:09:37Z

@akolotov I think all comments were addressed. Unless we are missing something I think we can merge this PR

akolotov

Let's do a stress testing for this set of changes and if everything is OK, merge this PR.

Add watchdog for watchers

patitonar · 2018-10-25T19:22:41Z

@akolotov These are the results of the stress testing:

Signature Requests

Tests	1	2	3	4	5	AVG
count	1000	1000	1000	1000	1000	1000
mean	12112	17714	15377	16468	15745	15483
median	11968	17925	15518	16994	15931	15667
min	5584	6423	5803	6121	6090	6004
max	18685	27554	23844	25310	24198	23918

Collected Signatures

Tests	1	2	3	4	5	AVG
count	1000	1000	1000	1000	1000	1000
mean	8564	5399	6653	9684	9605	7981
median	8771	5971	7295	9854	10056	8389
min	7301	2783	3003	8331	7165	5716
max	9156	7453	9492	10889	10687	9535

One thing that is worth mentioning is that I had to set MAX_PROCESSING_TIME=25000 to be able to complete processing the 1000 transactions.

akolotov · 2018-10-25T19:28:23Z

Do you have the previous statistic? Can we store it somewhere for reference to find possible degradation?

akolotov · 2018-10-25T19:30:54Z

One thing that is worth mentioning is that I had to set MAX_PROCESSING_TIME=25000 to be able to complete processing the 1000 transactions.

That's strange. This is not how I would expect it should work.

fvictorio · 2018-10-26T13:31:20Z

That's strange. This is not how I would expect it should work.

The watcher processes all the transactions at the same time (even if internally it handles them in chunks of 50), and the watchdog is at the top level of the watcher. I think that's why that happens.

We could move the watchdog at a lower level, or we could make the max processing time a multiple of the number of events that are going to be processed (this also would mean moving it a little lower, but not that much). What do you think?

akolotov · 2018-10-26T13:38:33Z

We could move the watchdog at a lower level, or we could make the max processing time a multiple of the number of events that are going to be processed (this also would mean moving it a little lower, but not that much). What do you think?

I don't think that it will help if the process hangs in the moment of reading events.
I think that we need to implement a way to kick the watchdog timer after every chunk of transactions. By doing this the watcher/sender will notify the watchdog that it is still alive.

fvictorio · 2018-10-26T13:52:42Z

That's a great idea, let's do that.

akolotov · 2018-10-26T13:54:47Z

let's do that.

My suggestion is to do this under a separate issue.
Currently it is required to confirm that there is no much degradation from the previous version of the token bridge. As soon as we confirm this, the PR could be merged.

fvictorio · 2018-10-26T13:57:18Z

Issue created: #110

patitonar · 2018-10-29T12:57:02Z

@akolotov Here are the results of stress testing on develop branch:

Signature Requests

Tests	1	2	3	AVG
count	1000	1000	1000	1000
mean	15431	14850	13376	14552
median	15396	15185	13462	14681
min	5624	5663	5720	5669
max	24536	22354	20412	22434

Collected Signatures

Tests	1	2	3	AVG
count	1000	1000	1000	1000
mean	9310	9462	9745	9505
median	9755	9476	9961	9730
min	7344	8482	8398	8074
max	10232	10182	10660	10358

If this statistics compared with the new values looks good to you, I can add the new ones on docs/stress-testing.md so we can use them for future reference.

akolotov · 2018-10-29T13:21:51Z

Thanks @patitonar. I see that there is no significant performance difference between these code sets. So, yes, we can merge these changes.

patitonar added 8 commits September 25, 2018 11:43

Add ERC20-to-Native contract abis

f43e99c

Add support ERC20-to-Native

9487e38

Update blocks related scripts

54b3e4b

Update transactions scripts

9966070

Update README

b268e5e

Add ERC20-to-Native deploy on e2e tests WiP

d38bf87

Update docker config for e2e

5443e3d

Add ERC20-to-Native e2e tests

0f8d775

patitonar requested review from fvictorio and akolotov September 26, 2018 14:34

ghost assigned patitonar Sep 26, 2018

ghost added the review label Sep 26, 2018

Update README.md

5bd965b

ghost assigned fvictorio Sep 26, 2018

akolotov requested changes Sep 26, 2018

View reviewed changes

patitonar added 2 commits September 26, 2018 15:37

Set default mode on test environment

ceabbf3

Update sendTx to receive gasPrice on wei unit

2173d84

patitonar mentioned this pull request Oct 2, 2018

Add support ERC20 to Native omni/bridge-ui#138

Merged

patitonar added 5 commits October 9, 2018 09:32

Merge branch 'develop' into support-erc20-native-#81

6bdf661

# Conflicts: # README.md

Merge branch 'develop' into support-erc20-native-#81

cbeab85

# Conflicts: # e2e/Dockerfile

Fix oracle gasPrice conversion to wei

7fe51a3

Use string value to convert gas price to wei

451da02

Update e2e contracts addresses

ec3aaf6

Update README gas price fallback description

591b06c

akolotov reviewed Oct 12, 2018

View reviewed changes

fvictorio and others added 2 commits October 13, 2018 18:04

Get tokens addresses from bridge contracts in scripts

5e36ca2

Add gas price variable for testing transactions

5ec159d

fvictorio and others added 7 commits October 24, 2018 08:59

Use exit code constant for general errors

3d706dd

Add default max processing time

f38688c

Update getting past events log level to info

962f19a

Update transaction nonce log level to info

19ec932

Add log when getting las processed block

39d6f3f

Fix last processed block being log as Hex value

bbb2c85

Fix code formatting

5d19cca

akolotov approved these changes Oct 25, 2018

View reviewed changes

patitonar added 2 commits October 25, 2018 14:30

Merge branch 'support-erc20-native-#81' into add-watchdog

597675e

Merge pull request #106 from poanetwork/add-watchdog

e13bf28

Add watchdog for watchers

fvictorio mentioned this pull request Oct 26, 2018

Let watcher notify watchdog that it's still alive #110

Open

Add stress testing statistics results

aafd86f

patitonar merged commit 7069ef9 into develop Oct 29, 2018

ghost removed the review label Oct 29, 2018

patitonar deleted the support-erc20-native-#81 branch October 29, 2018 14:53

This was referenced Oct 30, 2018

Check .env.example file #100

Closed

Support of new bridge mode: ERC20 to Native #81

Closed

Watcher should log the block number it is currently working on #70

Closed

ArseniiPetrovich mentioned this pull request Dec 11, 2018

Security review fixes #119

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support ERC20 to Native #85

Add support ERC20 to Native #85

patitonar commented Sep 26, 2018

fvictorio commented Sep 26, 2018

akolotov left a comment

patitonar commented Sep 26, 2018

akolotov commented Oct 10, 2018

akolotov Oct 12, 2018

patitonar Oct 16, 2018

akolotov Oct 12, 2018

fvictorio Oct 13, 2018

akolotov Oct 12, 2018

fvictorio Oct 12, 2018

akolotov Oct 12, 2018

fvictorio Oct 12, 2018

akolotov Oct 16, 2018

fvictorio Oct 16, 2018

akolotov Oct 17, 2018

patitonar commented Oct 25, 2018

akolotov left a comment

patitonar commented Oct 25, 2018

akolotov commented Oct 25, 2018

akolotov commented Oct 25, 2018

fvictorio commented Oct 26, 2018

akolotov commented Oct 26, 2018

fvictorio commented Oct 26, 2018

akolotov commented Oct 26, 2018

fvictorio commented Oct 26, 2018

patitonar commented Oct 29, 2018

akolotov commented Oct 29, 2018

		@@ -13,12 +13,12 @@ VALIDATOR_ADDRESS_PRIVATE_KEY=

		HOME_GAS_PRICE_ORACLE_URL=https://gasprice.poa.network/

Add support ERC20 to Native #85

Add support ERC20 to Native #85

Conversation

patitonar commented Sep 26, 2018

fvictorio commented Sep 26, 2018

akolotov left a comment

Choose a reason for hiding this comment

patitonar commented Sep 26, 2018

akolotov commented Oct 10, 2018

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

patitonar commented Oct 25, 2018

akolotov left a comment

Choose a reason for hiding this comment

patitonar commented Oct 25, 2018

akolotov commented Oct 25, 2018

akolotov commented Oct 25, 2018

fvictorio commented Oct 26, 2018

akolotov commented Oct 26, 2018

fvictorio commented Oct 26, 2018

akolotov commented Oct 26, 2018

fvictorio commented Oct 26, 2018

patitonar commented Oct 29, 2018

akolotov commented Oct 29, 2018