multiple exit processors #1573

boolafish · 2020-06-09T09:19:36Z

📋 ref https://github.com/omgnetwork/new-tx-type-poc/issues/10

This PR is aiming to merge to payment_v2_poc branch.

Overview

TL;DR a lots of hack to have something running and make existing tests happy

Changes

Have ExitProcessorDispatcher that dispatch and forward requests to ExitProcessors of each exit game, and then merge the result from all exit processors.
Add payment v2 tx type. And spin up the exit processor according to tx_type.
Add exit_games to Configuration. However, in this PR we still keep the payment_exit_game under contract_addr so it makes the tests pass. We can refactor that out later.
Use the snapshot with 2 payment exit games
add address (the source contract address) to events. Dispatcher relies on this to know where to direct the events.

Testing

CI testing passes

... and run mix format

1. fix the unit tests, add address to log result 2. change how we bind contract address to log, use the pre-decoded address data

Fix Conflicts: - apps/omg_watcher/lib/omg_watcher/api/status.ex - apps/omg_watcher/lib/omg_watcher/block_getter.ex - apps/omg_watcher/lib/omg_watcher/exit_processor.ex - apps/omg_watcher/lib/omg_watcher/sync_supervisor.ex

Previous snapshot was wrong. Using this one: omisego-images/docker-elixir-omg#43 (comment)

boolafish · 2020-06-09T09:23:51Z

apps/omg_eth/lib/omg_eth/release_tasks/set_contract.ex

+    {exit_games, eth_vault, erc20_vault, min_exit_period_seconds, contract_semver, child_block_interval}
+  end
+
+  defp merge_configuration(config, extra_config) do


dialyzer is arguing too many args.

I think you mean credo probably?

InoMurko · 2020-06-12T17:27:47Z

I don't quite understand this PR. There's lots of #TODOs and hacks. What are we looking at? What are we trying to accomplish with this PR?

boolafish · 2020-06-13T01:46:10Z

@InoMurko so one of our team's goal in this cycle is to exercise a POC soft upgrade to ensure that yes, it is doable with elixir-omg code base. A nice-to-have goal is to as well refactor the code to make it ALD ready.

The goal of this PR is to handle the exit processor part on soft upgrade. However, the strategy chosen here, IMO, is sth we should avoid in long-term as it is building on top of the ExitProcessor monster 🤦‍♂️ and this strategy would probably only work for upgrading to another payment tx, not flexible enough. However, this is the simplest thing we can do now. I have tried on the path to remove the monster state in ExitProcessorCore in some of the previous PR (closed) but I re-prioritize the exercising a POC over refactoring now due to it's entering second half of the cycle and not enough engineering resource to cover full refactoring + exercising.

After adding the necessary parts, we will add cabbage test to demo multi-exit game support. I don't know exactly how yet, but with the cabbage test there it might be easier for us to refactor out to better code design afterward.

InoMurko

I guess it's OK for a POC. But I would be against putting this to master (or taking it as a possible solution - has considerable design flaws for long term use).

InoMurko · 2020-06-16T13:29:52Z

apps/omg_eth/lib/omg_eth/release_tasks/set_contract.ex

-      plasma_framework |> exit_game_contract_address(ExPlasma.payment_v1(), rpc_api) |> Encoding.to_hex()
+    # TODO: get the list of types from ex_plasma?
+    exit_games =
+      Enum.into(OMG.WireFormatTypes.exit_game_tx_types(), %{}, fn type ->


Not sure if this version of ex_plasma has these but in childchain v2 I call ExPlasma.payment_v1()

InoMurko · 2020-06-16T13:33:43Z

apps/omg_eth/test/fixtures.exs

@@ -64,16 +68,33 @@ defmodule OMG.Eth.Fixtures do
    token_addr
  end

+  # inject the exit games into :omg_eth
+  # test fixture does not rely on the release task so it would need this setup
+  defp setup_exit_games() do


this seems unnecessary.

all contracts are set in test.exs, let's keep it there not to pollute tests and global state (application config).

not sure how we do this in test.exs. Can the config file load the module we need and make the call to Geth? (Did a bit try after seeing your comment, but it argues those module as unknown)

Alternatively is we add payment_v2_exit_game to localchain_contract_addresses.env which would need all the change from plasma-contracts, docker-elixir-omg and I doubt this is what we want, I think we probably prefer to get exit game from plasma_framework more?

InoMurko · 2020-06-16T13:37:38Z

apps/omg_watcher/lib/omg_watcher/exit_processor_dispatcher.ex

+    do_chack_validity(timeout)
+  end
+
+  defp do_chack_validity(timeout \\ 5000) do


check not chack

InoMurko · 2020-06-16T13:38:45Z

apps/omg_watcher/lib/omg_watcher/exit_processor_dispatcher.ex

+
+      case acc_chain_validity do
+        :ok ->
+          {chain_validity, acc_events ++ events}


a good practice is to avoid list concatenation this way.
use [acc_events | events] and Enum.reverse when you're done.

InoMurko · 2020-06-16T13:39:29Z

apps/omg_watcher/lib/omg_watcher/exit_processor_dispatcher.ex

+      :get_active_in_flight_exits
+      |> forward()
+      |> Enum.reduce([], fn {:ok, in_flight_exits}, acc ->
+        acc ++ in_flight_exits


same as above, [acc | in_flight_exits] and enum.reverse if order is important.

InoMurko · 2020-06-16T13:41:07Z

apps/omg_watcher/lib/omg_watcher/exit_processor_dispatcher.ex

+  # TODO: move private funcs at the bottom of the file
+  defp forward(func) do
+    Enum.map(OMG.WireFormatTypes.exit_game_tx_types(), fn tx_type ->
+      GenServer.call(tx_type, func)


it's pretty expensive to sync call in a tight loop. might be better to decouple these.

InoMurko · 2020-06-16T13:41:25Z

apps/omg_watcher/lib/omg_watcher/exit_processor_dispatcher.ex

+  defp dispatch(event_name, events) do
+    db_updates =
+      Enum.flat_map(group_events(events), fn {transaction_type, events} ->
+        {:ok, db_updates} = GenServer.call(transaction_type, {event_name, events})


same as above.

Use [a|b] as best practice

pnowosie

I've asked questions to understand more... One thing looks suspicious is exit contracts configuration.

pnowosie · 2020-06-16T12:31:10Z

apps/omg_eth/lib/omg_eth/application.ex

+    # TODO: Make this smarter. Adding new exit game contracts
+    # should be allowed, changing them, not so much


Can we rephrase this comment to better suggest what is the task behind this todo. "Make this smarter" could mean many things :D

pnowosie · 2020-06-16T13:50:51Z

apps/omg_eth/lib/omg_eth/release_tasks/set_contract.ex

@@ -210,4 +215,29 @@ defmodule OMG.Eth.ReleaseTasks.SetContract do
    {:ok, _} = Application.ensure_all_started(:logger)
    {:ok, _} = Application.ensure_all_started(:ethereumex)
  end
+
+  defp valid_extra_config?(extra_config) do


extra_config_valid? ❓

pnowosie · 2020-06-16T14:02:43Z

apps/omg_eth/test/fixtures.exs

@@ -20,6 +20,8 @@ defmodule OMG.Eth.Fixtures do

  alias OMG.Eth.Configuration
  alias OMG.Eth.Encoding
+  alias OMG.Eth.RootChain.Abi, as: RootChainABI


It pretty uncommon pattern in our codebase - I'd opt to avoid when it's unnecessary ❓

I am using this because in this file there is sth like ABI.encode where ABI is another library.
If we simply use alias, it will resulting in ABI and Abi and is pretty confusing though there is slight difference.

apps/omg_eth/test/fixtures.exs

pnowosie · 2020-06-16T14:15:22Z

apps/omg_watcher/lib/omg_watcher/block_getter/core.ex

@@ -424,6 +424,12 @@ defmodule OMG.Watcher.BlockGetter.Core do
  Takes results from `ExitProcessor.check_validity` into account, to potentially stop getting blocks
  """
  @spec consider_exits(t(), ExitProcessor.Core.check_validity_result_t()) :: t()
+  def consider_exits(%__MODULE__{} = state, exit_processor_results) when is_list(exit_processor_results) do


Is declaration order valid here? I'd see it as the last function pattern - wdyt?

pnowosie · 2020-06-16T14:24:57Z

apps/omg_watcher/lib/omg_watcher/exit_processor_dispatcher.ex

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+defmodule OMG.Watcher.ExitProcessorDispatcher do


Can we rearrange public functions first then private ones?

pnowosie · 2020-06-16T14:25:28Z

apps/omg_watcher/lib/omg_watcher/exit_processor_dispatcher.ex

+    {:ok, in_flight_exits}
+  end
+
+  # TODO: move private funcs at the bottom of the file


pnowosie · 2020-06-16T14:34:38Z

apps/omg_watcher/lib/omg_watcher/exit_processor_dispatcher.ex

+
+  defp forward(func, timeout) do
+    Enum.map(OMG.WireFormatTypes.exit_game_tx_types(), fn tx_type ->
+      GenServer.call(tx_type, func, timeout)


tx_type is a GenServer's name here ❓

Yes, and I think it's OK for POC

pnowosie · 2020-06-16T14:36:47Z

apps/omg_watcher/lib/omg_watcher/exit_processor_dispatcher.ex

+
+  defp dispatch(event_name, events) do
+    db_updates =
+      Enum.flat_map(group_events(events), fn {transaction_type, events} ->


Let's be consistent

Suggested change

Enum.flat_map(group_events(events), fn {transaction_type, events} ->

Enum.flat_map(group_events(events), fn {tx_type, events} ->

pnowosie · 2020-06-16T14:42:42Z

snapshots.env

-SNAPSHOT_MIX_EXIT_PERIOD_SECONDS_120=https://storage.googleapis.com/circleci-docker-artifacts/data-elixir-omg-tester-plasma-deployer-stable-20200410-MIN_EXIT_PERIOD-120-PLASMA_CONTRACTS_SHA-a69c763f239b81c5eb46b0bbdef3459f764360dc.tar.gz
-SNAPSHOT_MIX_EXIT_PERIOD_SECONDS_240=https://storage.googleapis.com/circleci-docker-artifacts/data-elixir-omg-tester-plasma-deployer-stable-20200410-MIN_EXIT_PERIOD-240-PLASMA_CONTRACTS_SHA-a69c763f239b81c5eb46b0bbdef3459f764360dc.tar.gz
-CONTRACT_SHA=a69c763f239b81c5eb46b0bbdef3459f764360dc
+SNAPSHOT_MIX_EXIT_PERIOD_SECONDS_20=https://storage.googleapis.com/circleci-docker-artifacts/data-elixir-omg-tester-plasma-deployer-dev-d01163d-MIN_EXIT_PERIOD-20-PLASMA_CONTRACTS_SHA-1d25e66420aaa4d23cac145c2ecea64123218b9a.tar.gz


WTH is this file for?

This is the snapshot with 2 payment exit game contract deployed

contract PR: omgnetwork/plasma-contracts#616

pnowosie

All concerns we've already discussed.

* feat: start multiple exit processors * chore: use snapshot support payment v2 ... and run mix format * fix: setup exit_games for test fixture * fix: dispatch return in the format of {:ok, []} * fix: make watcher integ test to work * fix: unit test for EthEventAggregator 1. fix the unit tests, add address to log result 2. change how we bind contract address to log, use the pre-decoded address data * fix: patch payment_exit_game back into contract_addr * fix: wrong filter_events mapping * fix: linter, dializer, remove tmp logging * fix: use snapwhot with payment v2 Previous snapshot was wrong. Using this one: omisego-images/docker-elixir-omg#43 (comment) * refactor: restrain from using ++ Use [a|b] as best practice * chore: use Task.async_stream instad of sync looping * refactor: move private functions to the end * fix: map after Task.async_stream Co-authored-by: Thibault Denizet <thibault@omisego.co>

* feat: multiple exit processors (#1573) * feat: start multiple exit processors * chore: use snapshot support payment v2 ... and run mix format * fix: setup exit_games for test fixture * fix: dispatch return in the format of {:ok, []} * fix: make watcher integ test to work * fix: unit test for EthEventAggregator 1. fix the unit tests, add address to log result 2. change how we bind contract address to log, use the pre-decoded address data * fix: patch payment_exit_game back into contract_addr * fix: wrong filter_events mapping * fix: linter, dializer, remove tmp logging * fix: use snapwhot with payment v2 Previous snapshot was wrong. Using this one: omisego-images/docker-elixir-omg#43 (comment) * refactor: restrain from using ++ Use [a|b] as best practice * chore: use Task.async_stream instad of sync looping * refactor: move private functions to the end * fix: map after Task.async_stream Co-authored-by: Thibault Denizet <thibault@omisego.co> * feat: test with payment v2 transfer Tested by hacking the watcher info /transaction.create to generate payment v2 tx instead. And then the tx would be signed and send normally. Tested with the cabbage transaction tests. This commit instead of providing new module for payment, it reuses most of the part instead of the function that generate the payment struct. This commit introduces Payment.Builder to build different payment tx. Probably not the most clean way of handling things but the least change required to allow cabbage test to work. * feat: allow /transaction.create to pass in tx_type add tx_type as a param of 'order'. If not passed, default to 1 for backward competibility. * test: cabbage test for payment v2 transfer * fix: cleanup linter, dialyzer, remove branching on tx type when building payment tx. * refactor: use Alice and Bob instead Simplify the test code and make it easier to read * fix: credo complain single pipe * test: payment v2 with standard exit * fix: compiler warning me :( * refactor: remove commented codes * fix: linter * style: simpler code * refactor: parameterize users Co-authored-by: Thibault Denizet <thibault@omisego.co>

* feat: start multiple exit processors * chore: use snapshot support payment v2 ... and run mix format * fix: setup exit_games for test fixture * fix: dispatch return in the format of {:ok, []} * fix: make watcher integ test to work * fix: unit test for EthEventAggregator 1. fix the unit tests, add address to log result 2. change how we bind contract address to log, use the pre-decoded address data * fix: patch payment_exit_game back into contract_addr * fix: wrong filter_events mapping * fix: linter, dializer, remove tmp logging * fix: use snapwhot with payment v2 Previous snapshot was wrong. Using this one: omisego-images/docker-elixir-omg#43 (comment) * refactor: restrain from using ++ Use [a|b] as best practice * chore: use Task.async_stream instad of sync looping * refactor: move private functions to the end * fix: map after Task.async_stream Co-authored-by: Thibault Denizet <thibault@omisego.co>

* feat: multiple exit processors (#1573) * feat: start multiple exit processors * chore: use snapshot support payment v2 ... and run mix format * fix: setup exit_games for test fixture * fix: dispatch return in the format of {:ok, []} * fix: make watcher integ test to work * fix: unit test for EthEventAggregator 1. fix the unit tests, add address to log result 2. change how we bind contract address to log, use the pre-decoded address data * fix: patch payment_exit_game back into contract_addr * fix: wrong filter_events mapping * fix: linter, dializer, remove tmp logging * fix: use snapwhot with payment v2 Previous snapshot was wrong. Using this one: omisego-images/docker-elixir-omg#43 (comment) * refactor: restrain from using ++ Use [a|b] as best practice * chore: use Task.async_stream instad of sync looping * refactor: move private functions to the end * fix: map after Task.async_stream Co-authored-by: Thibault Denizet <thibault@omisego.co> * feat: test with payment v2 transfer Tested by hacking the watcher info /transaction.create to generate payment v2 tx instead. And then the tx would be signed and send normally. Tested with the cabbage transaction tests. This commit instead of providing new module for payment, it reuses most of the part instead of the function that generate the payment struct. This commit introduces Payment.Builder to build different payment tx. Probably not the most clean way of handling things but the least change required to allow cabbage test to work. * feat: allow /transaction.create to pass in tx_type add tx_type as a param of 'order'. If not passed, default to 1 for backward competibility. * test: cabbage test for payment v2 transfer * fix: cleanup linter, dialyzer, remove branching on tx type when building payment tx. * refactor: use Alice and Bob instead Simplify the test code and make it easier to read * fix: credo complain single pipe * test: payment v2 with standard exit * fix: compiler warning me :( * refactor: remove commented codes * fix: linter * style: simpler code * refactor: parameterize users Co-authored-by: Thibault Denizet <thibault@omisego.co>

Thibault Denizet and others added 11 commits May 21, 2020 16:58

feat: start multiple exit processors

021a8ad

chore: use snapshot support payment v2

0d6f452

... and run mix format

fix: setup exit_games for test fixture

8bbd996

fix: dispatch return in the format of {:ok, []}

5d34711

fix: make watcher integ test to work

efebcb8

fix: unit test for EthEventAggregator

19b4022

1. fix the unit tests, add address to log result 2. change how we bind contract address to log, use the pre-decoded address data

Merge branch 'master' into boolafish/multiple-exit-processors

6b38d27

Fix Conflicts: - apps/omg_watcher/lib/omg_watcher/api/status.ex - apps/omg_watcher/lib/omg_watcher/block_getter.ex - apps/omg_watcher/lib/omg_watcher/exit_processor.ex - apps/omg_watcher/lib/omg_watcher/sync_supervisor.ex

fix: patch payment_exit_game back into contract_addr

0abfd96

fix: wrong filter_events mapping

31b59ce

fix: linter, dializer, remove tmp logging

dadd5de

fix: use snapwhot with payment v2

8cf40fe

Previous snapshot was wrong. Using this one: omisego-images/docker-elixir-omg#43 (comment)

boolafish commented Jun 9, 2020

View reviewed changes

boolafish marked this pull request as ready for review June 10, 2020 00:26

boolafish requested review from pnowosie and T-Dnzt June 10, 2020 00:26

T-Dnzt requested review from InoMurko and unnawut June 12, 2020 08:19

boolafish mentioned this pull request Jun 16, 2020

cabbage test for payment v2 #1578

Merged

InoMurko approved these changes Jun 16, 2020

View reviewed changes

boolafish added 4 commits June 17, 2020 16:21

refactor: restrain from using ++

ecb51ae

Use [a|b] as best practice

chore: use Task.async_stream instad of sync looping

08c9439

refactor: move private functions to the end

6ca8d48

fix: map after Task.async_stream

368be8e

pnowosie reviewed Jun 17, 2020

View reviewed changes

pnowosie self-requested a review June 17, 2020 09:17

pnowosie approved these changes Jun 17, 2020

View reviewed changes

boolafish merged commit 1423403 into payment_v2_poc Jun 18, 2020

boolafish deleted the boolafish/multiple-exit-processors branch June 18, 2020 02:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multiple exit processors #1573

multiple exit processors #1573

boolafish commented Jun 9, 2020 •

edited

Loading

boolafish Jun 9, 2020

InoMurko Jun 12, 2020

InoMurko commented Jun 12, 2020

boolafish commented Jun 13, 2020 •

edited

Loading

InoMurko left a comment •

edited

Loading

InoMurko Jun 16, 2020

InoMurko Jun 16, 2020

boolafish Jun 17, 2020

InoMurko Jun 16, 2020

InoMurko Jun 16, 2020

InoMurko Jun 16, 2020

InoMurko Jun 16, 2020

InoMurko Jun 16, 2020

pnowosie left a comment

pnowosie Jun 16, 2020

pnowosie Jun 16, 2020

pnowosie Jun 16, 2020

boolafish Jun 18, 2020

pnowosie Jun 16, 2020

pnowosie Jun 16, 2020

pnowosie Jun 16, 2020

pnowosie Jun 16, 2020

pnowosie Jun 17, 2020

pnowosie Jun 16, 2020

pnowosie Jun 16, 2020

boolafish Jun 17, 2020 •

edited

Loading

pnowosie left a comment

		# TODO: Make this smarter. Adding new exit game contracts
		# should be allowed, changing them, not so much

	Enum.flat_map(group_events(events), fn {transaction_type, events} ->
	Enum.flat_map(group_events(events), fn {tx_type, events} ->

multiple exit processors #1573

multiple exit processors #1573

Conversation

boolafish commented Jun 9, 2020 • edited Loading

Overview

Changes

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InoMurko commented Jun 12, 2020

boolafish commented Jun 13, 2020 • edited Loading

InoMurko 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

pnowosie 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

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 Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

pnowosie left a comment

Choose a reason for hiding this comment

boolafish commented Jun 9, 2020 •

edited

Loading

boolafish commented Jun 13, 2020 •

edited

Loading

InoMurko left a comment •

edited

Loading

boolafish Jun 17, 2020 •

edited

Loading