feat: experimental payment v2 migration script #616

boolafish · 2020-04-17T05:53:46Z

Note

Add experimental payment v2 contract into the migration scripts.

Chosen the 300+ instead of 2000+ to be the migration index to not disrupt with the future deployment. So even if we run this migration script in existing deployed network. It would not take any effect.

Also, the deployment is guarded by env var MULTI_EXIT_GAME_EXPERIMENT. Need to be set to true to deploy the payment v2 contracts.

closes: https://github.com/omisego/new-tx-type-poc/issues/2

Test

turned on MULTI_EXIT_GAME_EXPERIMENT env var in CI and shows sth like:
"Registering paymentV2ToPaymentV3Condition (0x963eC3789de59707309bAe6D955E44c7D0756513) to spendingConditionRegistry"

Experimental e2e test with `MULTI_EXIT_GAME_EXPERIMENT` env var

Contract: PaymentExitGame - V2 Extension experiment
    Given contracts deployed, exit game and both ETH and ERC20 vault registered
      Given Alice deposited ETH and upgrade the output from payment v1 to v2
        When Alice tries to start the standard exit on the payment v2 output
          ✓ should start successfully (53ms)
          And then someone processes the exits for ETH after two weeks
            ✓ should return the output amount plus standard exit bond to Alice

This commit allow deployment of payment v2 with EXPERIMENT env var flag set to true. Payment V2 here is the exactly same feature set as payment V1.

- add the explanation of using `EXPERIMENT` env var - add explantion of other existing env var in migration scripts - put the deploying.md link to README - add a bit more detailed explantion on `local` and `remote` network flag in README

…iment

Smoke test using standard exit

boolafish · 2020-04-20T09:52:45Z

From slack: https://omisego.slack.com/archives/C9KNA675J/p1587376124427600?thread_ts=1587375985.425600&cid=C9KNA675J

Ino 3 minutes ago
so maybe EXPERIMENTAL isn’t the right flag. Maybe you need proper feature flags?

Ino 2 minutes ago
like more precise settings

Ino 2 minutes ago
one would like to know what is being enabled with a flag

plasma_framework/docs/deploying/deploying.md

thec00n · 2020-04-21T02:36:13Z

plasma_framework/test/endToEndTests/PaymentV2Experiment.e2e.test.js

+    const PAYMENT_V2_TX_TYPE = config.registerKeys.txTypes.paymentV2;
+    const MERKLE_TREE_DEPTH = 16;
+
+    const alicePrivateKey = '0x7151e5dab6f8e95b5436515b83f423c4df64fe4c6149f864daa209b26adb10ca';


Can we load the private key from the the environment?

we could but why?

Yeah, this key is just for testing - no need to hide it

It's generally a bad practice to have private keys in code.

thec00n · 2020-04-21T02:41:17Z

@boolafish how will that experiment flag be used in the future? For future development will we always have a deployable (and audited) version on master?

pik694

Let's think whether we want to use experimental in the future - if yes, then let's refactor these scripts, so that they will not need any changes, no matter what experiment is.

pik694 · 2020-04-21T14:23:21Z

plasma_framework/docs/deploying/deploying.md

+- 2000: Future extensions, new exit games, etc.
+
+## ENV VAR settings
+- TENDERLY: set to `true` when needed to push the contract to tenderly. Default not pushing to tenderly.


Suggested change

- TENDERLY: set to `true` when needed to push the contract to tenderly. Default not pushing to tenderly.

- TENDERLY: set to `true` to push the contract to tenderly. Default: `false`.

Default: false.

This is slightly incorrect. It is default to undefined

actually it is false :
const isExperiment = process.env.EXPERIMENT || false;

eh..yes. the || false part make isExperiment false. I think what I want to point out is that the env var itself (process.env.EXPERIMENT) would be considered undefined, so it is slightly incorrect in this sense.

pik694 · 2020-04-21T14:53:21Z

plasma_framework/docs/deploying/deploying.md

+
+## ENV VAR settings
+- TENDERLY: set to `true` when needed to push the contract to tenderly. Default not pushing to tenderly.
+- DEPLOY_TEST_CONTRACTS: set to `true` when need to deploy some testing contracts like mock, wrapper for conformance testing. Default not deploying test contracts.


Suggested change

- DEPLOY_TEST_CONTRACTS: set to `true` when need to deploy some testing contracts like mock, wrapper for conformance testing. Default not deploying test contracts.

- DEPLOY_TEST_CONTRACTS: set to `true` to deploy helpers contracts like mocks, wrappers for conformance testing. Default: `false`.

pik694 · 2020-04-21T14:53:54Z

plasma_framework/docs/deploying/deploying.md

+## ENV VAR settings
+- TENDERLY: set to `true` when needed to push the contract to tenderly. Default not pushing to tenderly.
+- DEPLOY_TEST_CONTRACTS: set to `true` when need to deploy some testing contracts like mock, wrapper for conformance testing. Default not deploying test contracts.
+- EXPERIMENT: set to `true` when needed to deploy experimental contracts. For instance, deploying new payment exit game contract to test the flow of extension.


Suggested change

- EXPERIMENT: set to `true` when needed to deploy experimental contracts. For instance, deploying new payment exit game contract to test the flow of extension.

- EXPERIMENT: set to `true` to deploy experimental contracts. For instance, deploying new payment exit game contract to test the flow of extension. Default: `false`.

pik694 · 2020-04-21T15:00:17Z

plasma_framework/migrations/20_deploy_plasma_framework.js

@@ -10,11 +10,15 @@ module.exports = async (
    // eslint-disable-next-line no-unused-vars
    [deployerAddress, maintainerAddress, authorityAddress],
 ) => {
+    const isExperiment = process.env.EXPERIMENT || false;
+    const initialImmuneExitGames = (isExperiment)
+        ? config.experimental.frameworks.initialImmuneExitGames : config.frameworks.initialImmuneExitGames;


What about just swapping the used config?
This works only for this additional exit game, but if we switched the whole config, then whatever changes in the future - no changes are required.

pik694 · 2020-04-21T15:01:43Z

plasma_framework/migrations/300_payment_v2_spending_condition_registry.js

+    const isExperiment = process.env.EXPERIMENT || false;
+    if (isExperiment) {


Suggested change

const isExperiment = process.env.EXPERIMENT || false;

if (isExperiment) {

if (process.env.EXPERIMENT) {

pik694 · 2020-04-21T15:03:28Z

.circleci/config.yml

@@ -72,7 +72,7 @@ jobs:
      - setup_truffle_env
      - run:
          name: Run tests
-          command: truffle test 
+          command: EXPERIMENT=true npm run test


what about making parametrising these jobs and running them once forEXPERIMENT=true and once for EXPERIMENT=false

I don't think it brings any benefits now and the whole set of tests is pretty time consuming.
Currently test with EXPERIMENT=true runs all the test suite already.

but, what Im worried about is that now when I run EXPERIMENT=false Im still getting:

Registering paymentToPaymentCondition (0xf204a4Ef082f5c04bB89F7D5E6568B796096735a) to spendingConditionRegistry Registering paymentToPaymentV2Condition (0x82D50AD3C1091866E258Fd0f1a7cC9674609D254) to spendingConditionRegistry Registering feeClaimOutputToPaymentTxCondition (0x0d8cc4b8d15D4c3eF1d70af0071376fb26B5669b) to spendingConditionRegistry Registering paymentToPaymentV2Condition (0x1411CB266FCEd1587b0AA29E9d5a9Ef3Db64A9C5) to spendingConditionRegistry Registering paymentV2ToPaymentV2Condition (0x855d1c79Ad3fb086D516554Dc7187E3Fdfc1C79a) to spendingConditionRegistry Registering paymentV2ToPaymentV3Condition (0x8065F4c7b8c2bf53561af92D9DA2Ea022A0b28Ca) to spendingConditionRegistry {"authority_address":"0xc5fdf4076b8f3a5357c5e395ab970b5b54098fef","eth_vault":"0x4d2d24899c0b115a1fce8637fca610fe02f1909e","erc20_vault":"0xc8c03647d39a96f02f6ce8999bc22493c290e734","payment_exit_game":"0xcfed223fab2a41b5a5a5f9aaae2d1e882cb6fe2d","plasma_framework_tx_hash":"0x938aa3c16a2840b79c365ffde567d1edaa0e4739402ad89ca60bb7a738e6a418","plasma_framework":"0x345ca3e014aaf5dca488057592ee47305d9b3e10"}

commit: 62b841c7f7673e34760567e7ebaf714c51420591

you mean you set EXPERIMENT=false rpm run test and it is still printing the log above? 😨
Or can you point me to the setup that you found this?

So I run migrations scripts on the current HEAD and the problem does not persist (maybe that was sth with my env back then), but still one paymentToPaymentV2Condition is being registered (do not know if it is an issue though).

$ MULTI_EXIT_GAME_EXPERIMENT=true npx truffle migrate --network local | grep 'Registering.*V2' Registering paymentToPaymentV2Condition (0x170f89CFed0bE4dd0875c8EB403094B24f7b7b34) to spendingConditionRegistry Registering paymentToPaymentV2Condition (0x0EEa09b4E919C511Da053bD85cb88A5dD501b541) to spendingConditionRegistry Registering paymentV2ToPaymentV2Condition (0xc29bE658676d32f16e31AC54C67875a0D1a72298) to spendingConditionRegistry Registering paymentV2ToPaymentV3Condition (0x1f75D1F63bD0d62e56Cfa7E8B759536649DD467A) to spendingConditionRegistry

$ npx truffle migrate --network local | grep 'Registering.*V2' Registering paymentToPaymentV2Condition (0xa5a60d16bDC4d5694b2eDF94b144e8513D4E78aE) to spendingConditionRegistry

README.md

pik694 · 2020-04-21T15:07:45Z

plasma_framework/config.js

@@ -35,6 +35,21 @@ const development = {
            erc20: 2,
        },
    },
+    experimental: {


shouldn't this copy default config and just override changed values? This may make it easier to reason about what changes in comparison to non-experimental

pik694 · 2020-04-21T15:10:27Z

plasma_framework/migrations/302_renounce_payment_v2_spending_condition_registery.js

+    [deployerAddress, maintainerAddress, authorityAddress],
+) => {
+    const isExperiment = process.env.EXPERIMENT || false;
+    if (isExperiment) {


same as a few files above

pik694 · 2020-04-21T15:11:51Z

plasma_framework/migrations/301_payment_v2_spending_conditions.js

+    [deployerAddress, maintainerAddress, authorityAddress],
+) => {
+    const isExperiment = process.env.EXPERIMENT || false;
+    if (isExperiment) {


PR review suggestion Co-Authored-By: thec00n <thec00n@fork.at>

Use MUTLI_EXIT_GAME_EXPERIMENT to be the feature flag name instead. Also restructure the config a little bit.

boolafish · 2020-04-22T03:17:51Z

@boolafish how will that experiment flag be used in the future? For future development will we always have a deployable (and audited) version on master?

I think the master should always be deployable and that is why using the flag. I think probably each experiment would bring their own flag and can start some POC first quickly in local docker env.

Although I don't think we can assume master only have audited contract code....like a new experiment contract would still be merged to master first and probably make it elixir-omg and other downstream dependency code close-to-ready and then do the audit. However, I think master should default to deploy the audited code if without flags.

boolafish · 2020-04-22T03:25:54Z

@pik694 @thec00n I have update the PR with a more explicit flag name: MULTI_EXIT_GAME_EXPERIMENT. Can you review the update? Also I have fix most comments.

pik694 · 2020-04-23T21:12:08Z

@boolafish such an explicit flag, means that each time we want to create docker images, we have not only to create a PR pointing to a commit we want to build, but we also have to change building scripts. Is it ok, or should we think about some more convenient method? Maybe grouping all experimental features under some .env file which would be used by docker-compose and would latch experimental features we want to use.

sth like:
e2aec88

and then used like this

omisego-images/docker-elixir-omg@016253e

pik694

LGTM

plasma_framework/docs/deploying/deploying.md

boolafish · 2020-04-24T10:01:08Z

such an explicit flag, means that each time we want to create docker image...

@pik694 yeah, I think probably the docker side should be more flexible on injecting the env var then the current PR if we would like to go with explicit feature flag naming.

But I don't think we should put the file in this .env repo as that should be of control of the downstream. In my opinion the docker-elixir-omg should have the ability to define and inject the flags instead. So the .env file should be defined there.

Also, I don't think we need explicit EXPERIMENT flag to allow experiment mode.

Test with commit of this PR from plasma-contracts: 1d25e66420aaa4d23cac145c2ecea64123218b9a Using MULTI_EXIT_GAME_EXPERIMENT flag. omgnetwork/plasma-contracts#616

Co-Authored-By: Piotr Żelazko <piotr.zelazko@imapp.pl>

kevsul · 2020-04-22T07:44:06Z

plasma_framework/test/endToEndTests/PaymentV2Experiment.e2e.test.js

+    const PAYMENT_V2_TX_TYPE = config.registerKeys.txTypes.paymentV2;
+    const MERKLE_TREE_DEPTH = 16;
+
+    const alicePrivateKey = '0x7151e5dab6f8e95b5436515b83f423c4df64fe4c6149f864daa209b26adb10ca';


Yeah, this key is just for testing - no need to hide it

* feature: add flag to enable experimental contracts' features Refers omisego/new-tx-type-poc#3 * docs: update readme * fix: allow reset to commit in PR branch instead of master Previous setup it would only pull the master by default. Thus you cannot reset to commit still in a PR stage. * test: it works with the experiment flag Test with commit of this PR from plasma-contracts: 1d25e66420aaa4d23cac145c2ecea64123218b9a Using MULTI_EXIT_GAME_EXPERIMENT flag. omgnetwork/plasma-contracts#616 * revert: "test: it works with the experiment flag" This reverts commit d01163d. * fix: grammar Co-authored-by: boolafish <boolafish945@gmail.com>

boolafish added 5 commits April 17, 2020 14:04

docs: better format on the deploying.md

d045ebc

feat: support experiment payment V2

7601db2

This commit allow deployment of payment v2 with EXPERIMENT env var flag set to true. Payment V2 here is the exactly same feature set as payment V1.

docs: update readme and deploying

4ceb2b4

- add the explanation of using `EXPERIMENT` env var - add explantion of other existing env var in migration scripts - put the deploying.md link to README - add a bit more detailed explantion on `local` and `remote` network flag in README

fix: restructure config and set initialImmuneExitGames to 3 for exper…

dee8f3e

…iment

test: e2e payment v2 extention experiment

6dc207d

Smoke test using standard exit

boolafish marked this pull request as ready for review April 17, 2020 08:50

boolafish marked this pull request as draft April 17, 2020 08:56

feat: run with EXPERIMENT=true in CI

62b841c

boolafish marked this pull request as ready for review April 17, 2020 09:18

boolafish requested review from a team and pik694 April 17, 2020 09:18

thec00n reviewed Apr 21, 2020

View reviewed changes

plasma_framework/docs/deploying/deploying.md Outdated Show resolved Hide resolved

thec00n reviewed Apr 21, 2020

View reviewed changes

plasma_framework/docs/deploying/deploying.md Outdated Show resolved Hide resolved

thec00n reviewed Apr 21, 2020

View reviewed changes

plasma_framework/docs/deploying/deploying.md Outdated Show resolved Hide resolved

thec00n reviewed Apr 21, 2020

View reviewed changes

pik694 previously approved these changes Apr 21, 2020

View reviewed changes

fix: grammar in deploying document

3e8586d

PR review suggestion Co-Authored-By: thec00n <thec00n@fork.at>

boolafish dismissed pik694’s stale review via 3e8586d April 22, 2020 02:46

refactor: make EXPERIMENT flag explicit

590117c

Use MUTLI_EXIT_GAME_EXPERIMENT to be the feature flag name instead. Also restructure the config a little bit.

fix: use correct config path

1d25e66

pik694 reviewed Apr 23, 2020

View reviewed changes

plasma_framework/docs/deploying/deploying.md Outdated Show resolved Hide resolved

pik694 previously approved these changes Apr 23, 2020

View reviewed changes

boolafish requested review from thec00n and kevsul April 28, 2020 05:12

boolafish requested a review from T-Dnzt April 28, 2020 09:31

fix: capital starting word in the sentence

9f67db7

Co-Authored-By: Piotr Żelazko <piotr.zelazko@imapp.pl>

boolafish dismissed pik694’s stale review via 9f67db7 April 28, 2020 09:35

boolafish requested a review from pik694 April 28, 2020 09:35

pik694 approved these changes Apr 28, 2020

View reviewed changes

pik694 mentioned this pull request Apr 28, 2020

feat: support experiment feature flag on contract deployment omisego-images/docker-elixir-omg#43

Merged

kevsul approved these changes Apr 28, 2020

View reviewed changes

boolafish merged commit 1bfb545 into master Apr 29, 2020

boolafish deleted the boolafish/payment_v2_migration_script branch April 29, 2020 01:27

boolafish mentioned this pull request Jun 17, 2020

multiple exit processors omgnetwork/elixir-omg#1573

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: experimental payment v2 migration script #616

feat: experimental payment v2 migration script #616

boolafish commented Apr 17, 2020 •

edited

Loading

boolafish commented Apr 20, 2020

thec00n Apr 21, 2020

boolafish Apr 22, 2020

kevsul Apr 22, 2020

thec00n Apr 29, 2020

thec00n commented Apr 21, 2020

pik694 left a comment •

edited

Loading

pik694 Apr 21, 2020

boolafish Apr 22, 2020

pik694 Apr 22, 2020

boolafish Apr 22, 2020

pik694 Apr 21, 2020

pik694 Apr 21, 2020

pik694 Apr 21, 2020

pik694 Apr 21, 2020

pik694 Apr 21, 2020

boolafish Apr 22, 2020

pik694 Apr 22, 2020

boolafish Apr 22, 2020

pik694 Apr 23, 2020

pik694 Apr 21, 2020

pik694 Apr 21, 2020

pik694 Apr 21, 2020

boolafish commented Apr 22, 2020

boolafish commented Apr 22, 2020

pik694 commented Apr 23, 2020 •

edited

Loading

pik694 left a comment

boolafish commented Apr 24, 2020

kevsul Apr 22, 2020

	- TENDERLY: set to `true` when needed to push the contract to tenderly. Default not pushing to tenderly.
	- TENDERLY: set to `true` to push the contract to tenderly. Default: `false`.

	- DEPLOY_TEST_CONTRACTS: set to `true` when need to deploy some testing contracts like mock, wrapper for conformance testing. Default not deploying test contracts.
	- DEPLOY_TEST_CONTRACTS: set to `true` to deploy helpers contracts like mocks, wrappers for conformance testing. Default: `false`.

	- EXPERIMENT: set to `true` when needed to deploy experimental contracts. For instance, deploying new payment exit game contract to test the flow of extension.
	- EXPERIMENT: set to `true` to deploy experimental contracts. For instance, deploying new payment exit game contract to test the flow of extension. Default: `false`.

		const isExperiment = process.env.EXPERIMENT \|\| false;
		if (isExperiment) {

	const isExperiment = process.env.EXPERIMENT \|\| false;
	if (isExperiment) {
	if (process.env.EXPERIMENT) {

feat: experimental payment v2 migration script #616

feat: experimental payment v2 migration script #616

Conversation

boolafish commented Apr 17, 2020 • edited Loading

Note

Test

boolafish commented Apr 20, 2020

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

thec00n commented Apr 21, 2020

pik694 left a comment • edited Loading

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

boolafish commented Apr 22, 2020

boolafish commented Apr 22, 2020

pik694 commented Apr 23, 2020 • edited Loading

pik694 left a comment

Choose a reason for hiding this comment

boolafish commented Apr 24, 2020

Choose a reason for hiding this comment

boolafish commented Apr 17, 2020 •

edited

Loading

pik694 left a comment •

edited

Loading

pik694 commented Apr 23, 2020 •

edited

Loading