integrate se challenges from ALD with elixir-omg #1005

pdobacz · 2019-09-25T13:42:45Z

fixes #986, builds upon #949 (which needs to merge to wherever we opt to merge it to)

Overview

The integration test for SE challenges passes now.

Changes

integrate to contracts from rebased branch that has the spending condition registering finalized, this brought a couple of necessary modifications
updated EIP712 hashing

Testing

mix test --only integration test/omg_watcher/integration/invalid_exit_test.exs:68

among the usual tests.

coveralls · 2019-09-25T13:46:58Z

Coverage increased (+0.1%) to 85.785% when pulling a2b7afc on 986_integrate_SE_challenges into 3b26fc0 on master.

pdobacz · 2019-09-25T13:47:44Z

This still needs us to update the hashes in the Metamask compatibility test. For short term solution, would you be able to, @pnowosie? You'd probably be much faster to it than myself.

Also a long-term solution to test the compatibility (if we decide to keep this ofc) could be figured out. I had a brief idea to leverage this utility from Metamask maybe? Not sure if this was considered. It would require us to loop an npm package into the CI... :/ not nice, but if we might do it for truffle anyway in a near future, then maybe?

pnowosie · 2019-10-02T05:19:44Z

This still needs us to update the hashes in the Metamask compatibility test. For short term solution, would you be able to, @pnowosie? You'd probably be much faster to it than myself.

As you said long-term solution is to keep the tests outside. I discused with the omisego/omg-js and we agreed to combine tests there. (omgnetwork/omg-js#130)

pdobacz · 2019-10-02T08:22:28Z

OK, so I'll just 🔪 those tests out in this PR.

pdobacz · 2019-10-03T14:52:15Z

apps/omg/lib/omg/state/transaction/payment.ex

@@ -124,8 +124,10 @@ defmodule OMG.State.Transaction.Payment do
  end

  defp reconstruct_outputs(outputs_rlp) do
+    # TODO: ugly but to be cleaned up in the abstract output PR soon, so leaving as is


taking the liberty to cut a corner here. Handling this properly requires the abstract output type refactor, otherwise it's wasted effort.

InoMurko · 2019-10-03T15:02:28Z

apps/omg/lib/omg/typed_data_hash.ex

@@ -47,6 +47,7 @@ defmodule OMG.TypedDataHash do
  @spec hash_transaction(Transaction.Payment.t()) :: Crypto.hash_t()
  def hash_transaction(%Transaction.Payment{} = raw_tx) do


this might be stupid but now that we're talking about specification docs so much. could you point me to where did you figure out that a payment transaction's hash consist of these fields? is there a doc or did you read solidity code?

https://github.com/omisego/plasma-contracts/blob/master/plasma_framework/contracts/src/transactions/eip712Libs/PaymentEip712Lib.sol#L133 or https://github.com/omisego/plasma-contracts/blob/master/plasma_framework/test/helpers/paymentEip712.js#L32

InoMurko · 2019-10-03T15:05:27Z

apps/omg/lib/omg/typed_data_hash/config.ex

@@ -22,23 +22,31 @@ defmodule OMG.TypedDataHash.Config do
  alias OMG.TypedDataHash.Tools

  # Needed for test only to have value of address when `:contract_address` is not set
-  @fallback_ari_network_address "0x44de0ec539b8c4a4b530c78620fe8320167f2f74"
+  @fallback_contract_addr <<1::size(20)-unit(8)>>


could you help me understand why this is needed? I'm sensing there might be a better way of doing this...

yeah, I scratched my head a bit here too, finding a better way. So normally we want to use the address from the config. But if it's not there (e.g. in a unit test, but not in an integration test) we want just anything.

I tried to put the fallback addr into config/test.exs but something wasn't clicking.

I can revisit this now, having all the other stuff more or less behind.

so I think a more proper way to do it would be by a very simple Mock, which would inject the @fallback... address when needed, and then all the unit tests that don't have the contract_addr configured would have to explicitly name this mock.

The non-mocked behavior would go for the value in the config.

Another option is to have contract_addr config entry default to %{plasma_framework: 0x000000...1} (the fallback address) in test environment, instead of nil, and let it be overridden in the tests that require it to be properly set.

I think I'll try the second option first actually, the mock seems like a huge gun to fire here, and it would litter all those tests with mocks, while all we want is a test-specific config value.

InoMurko · 2019-10-03T15:08:55Z

apps/omg/test/omg/dependency_conformance/signature_test.exs

+    verify(contract, tx)
+  end
+
+  test "no outputs test", context do


is this all payment transaction type?

apps/omg/test/omg/dependency_conformance/signature_test.exs

InoMurko · 2019-10-03T15:13:31Z

apps/omg/test/omg/state/core_test.exs

@@ -485,8 +485,8 @@ defmodule OMG.State.CoreTest do

    # precomputed fixed hash to check compliance with hashing algo
    assert block_hash ==
-             <<16, 139, 87, 31, 138, 148, 220, 238, 229, 44, 183, 103, 56, 33, 63, 158, 60, 71, 34, 132, 92, 211, 169,
-               221, 234, 110, 103, 233, 122, 84, 126, 180>>
+             <<218, 153, 13, 247, 52, 151, 31, 191, 14, 98, 96, 181, 113, 239, 1, 101, 78, 189, 159, 121, 97, 13, 89,


what made the signature change?

Note that's a hash and it was the addition of output_type into the RLP encoding being the hash preimage that made it change.

InoMurko · 2019-10-03T15:14:10Z

apps/omg/test/omg/state/transaction_test.exs

@@ -56,8 +56,8 @@ defmodule OMG.State.TransactionTest do

    test "raw transaction hash is invariant" do
      assert Transaction.raw_txhash(@transaction) ==
-               <<22, 144, 106, 111, 111, 175, 135, 22, 122, 230, 103, 220, 153, 161, 147, 239, 150, 205, 18, 226, 161,
-                 163, 127, 20, 163, 205, 24, 68, 0, 213, 103, 161>>
+               <<32, 195, 156, 58, 16, 182, 26, 126, 67, 146, 125, 172, 69, 109, 119, 124, 233, 106, 160, 244, 41, 62,


same as above, is that because a normal payment type transaction now consist of more fields then it did before ALD?

yes, same reason and more specifically: addition of output_type field in the output

another noob question but a more fundamental one. why do we need output_type in the output hash?

because a transaction referencing different output_types might mean a different thing and should thereby be included in the signed struct hash.

InoMurko · 2019-10-07T05:28:47Z

apps/omg/test/omg/dependency_conformance/signature_test.exs

+
+  test "signature test - small tx", context do
+    contract = context[:contract]
+    tx = TestHelper.create_signed([{1, 0, 0, @alice}], [{@alice, @eth, 100}])


is there a way around using TestHelpers? I think they're somehow becoming a bucket for everything and make tests that should be self sufficient into a tree of jumps between modules and functions.

perhaps something for a separate issue? Maybe let's propose a bunch of renames and split that particular module into submodules (entities, txs and fee-stuff separately), if it's really bugging you.

Note this PR doesn't really change the approach it just makes it consistent (use create_signed in all cases)

InoMurko · 2019-10-07T05:29:55Z

apps/omg/test/omg/dependency_conformance/signature_test.exs


-    assert solidity_signer == elixir_signer
+    assert solidity_hash == OMG.TypedDataHash.hash_struct(tx)


are these results not predictable? why do we need an external source to verify the results in what should be a unit test?

not really, given the pace at which the EIP712 changes :). Also the point of this test is to test against a particular "reference" impl, vide our discussion on Merkling elsewhere (#901).

InoMurko · 2019-10-07T05:32:49Z

apps/omg_eth/lib/omg_eth/root_chain.ex

@@ -294,7 +294,7 @@ defmodule OMG.Eth.RootChain do
  # TODO - missing description + could this be moved to a statefull process?
  @spec get_root_deployment_height(binary() | nil, optional_addr_t()) ::
          {:ok, integer()} | Ethereumex.HttpClient.error()
-  def get_root_deployment_height(txhash \\ nil, contract \\ nil) do
+  def get_root_deployment_height(txhash \\ nil, contract \\ %{}) do


if we're going down the route of replacing contracts with a map %{}, maybe a struct would be a better representation what's being expected?

yeah, let's maybe tackle separately, this is the part that's not fully fleshed out yet (contract address management), but I think pieces of the puzzle are slowly falling into their places now.

InoMurko · 2019-10-07T05:33:41Z

apps/omg_eth/lib/omg_eth/root_chain.ex

@@ -479,7 +479,7 @@ defmodule OMG.Eth.RootChain do

  def decode_in_flight_exit_output_finalized(log) do
    non_indexed_keys = [:in_flight_exit_id, :output_index]
-    non_indexed_key_types = [{:uint, 192}, {:uint, 8}]
+    non_indexed_key_types = [{:uint, 160}, {:uint, 8}]


where in solidity did you find the change from uint 160 to 192?

(I hope I got this right, since IFEs aren't really tackled yet - separate issue).

This one I actually was just aware of (was following the debate on the topic of the change 192->160). But you can also see it in the diffs, wherever exit_id/exitId is mentioned.

InoMurko · 2019-10-07T05:35:07Z

apps/omg_watcher/test/omg_watcher/exit_processor/standard_exit_test.exs

+      {exiting_txbytes, _} = get_bytes_sig(exiting_tx)
+
+      assert {:ok, %{exiting_tx: ^exiting_txbytes, txbytes: ^txbytes}} =
+               %ExitProcessor.Request{


single pipe

see other comment

InoMurko · 2019-10-07T05:35:14Z

apps/omg_watcher/test/omg_watcher/exit_processor/standard_exit_test.exs

+    test "returns a deposit exiting_tx as part of the challenge response",
+         %{alice: alice, processor_empty: processor} do
+      exiting_tx = TestHelper.create_recovered([], [{alice, @eth, 10}])
+      processor = processor |> start_se_from(exiting_tx, @utxo_pos_deposit)


single pipe

not in this PR. Changing this just here would make it inconsistent with the rest of the file. Changing everywhere would obscure the PR with unrelated style changes, while I want this PR to only contain the changes relevant to/required by the change it's centered around.

Among other reasons behind this approach: your ability to review.

InoMurko · 2019-10-07T05:35:33Z

apps/omg_watcher/test/omg_watcher/integration/invalid_exit_test.exs

+    challenge = TestHelper.get_exit_challenge(first_tx_blknum, 0, 0)
+
+    assert {:ok, %{"status" => "0x1"}} =
+             OMG.Eth.RootChainHelper.challenge_exit(


single pipe

same here, not in this PR. This test is the same as the test removed in the previous commit, which is the point I'm trying to make here. If it wasn't removed in the prior commit, we'd see this in the diff here (you can still see this, if you search for the right diff - against pre-ALD master).

to squash: get rid of getting creating blocks when challenging SEs

new PaymentExitCondition contract structure + libs the `token` field has been dropped from `standardExits` call return value `challengeStandardExit` doesn't require challenge_tx_type anymore need to register the spending conditions properly

following a comment from CR

…ion (#1036) * refactor: dilute Transaction.Protocol.get_effects/3 in generic code OMG.Output covers for the polymorphism now * refactor: standardize the handling of inputs don't pass around zero inputs * refactor: abstract out OMG.Output protocol and its fungible token impl * refactor: clarify and streamline the input/output parsing code * refactor: leverage OMG.Output protocol some more * style: make outputs and transactions protocols look similar * refactor: abstract out input pointer type * refactor: abstract out input_pointer/5 out for OMG.Output * refactor: abstract out the non empty checking of ins/outs * refactor: change approach to filtering out unsynced-yet exiting utxos * fix: amend some missed failing tests after the refactor * refactor: unbrittle some exit processor tests wrt Transaction inputs * fix: take care of FIXMEs * fix: update after rebase on top of ALD integration (WIP, needs cleaning) * refactor: rearrange code related to unusual requests around the UTXOSet * fix: fix SE & SE challenge integration to work with abstract outputs * fix: make peace with the dialyzer * refactor: add some improvements after experimenting with erc721 * docs: add more documentation and comments after CR * test: add low-level UtxoSet tests along with specs and clarifications * fix: update after rebasing, remove hackarounds after #1005

pdobacz force-pushed the 986_integrate_SE_challenges branch from 2545454 to 56dfc69 Compare September 26, 2019 12:59

kasima mentioned this pull request Sep 27, 2019

Use PlasmaFramework & friends to deposit, tx, start SE, finalize SE #949

Closed

pdobacz mentioned this pull request Oct 2, 2019

Add metamask compliance tests omgnetwork/omg-js#130

Open

pdobacz force-pushed the 986_integrate_SE_challenges branch 3 times, most recently from 8ba06a1 to 99f89d8 Compare October 3, 2019 14:48

pdobacz commented Oct 3, 2019

View reviewed changes

pdobacz requested a review from InoMurko October 3, 2019 14:57

InoMurko reviewed Oct 3, 2019

View reviewed changes

InoMurko suggested changes Oct 3, 2019

View reviewed changes

pdobacz force-pushed the 986_integrate_SE_challenges branch from 99f89d8 to 1e96167 Compare October 4, 2019 10:43

pdobacz requested a review from InoMurko October 4, 2019 12:13

pdobacz force-pushed the 986_integrate_SE_challenges branch from bc4423b to ff52cac Compare October 4, 2019 12:39

InoMurko reviewed Oct 7, 2019

View reviewed changes

pdobacz mentioned this pull request Oct 8, 2019

Refactor: Use exvcr to record eth/rootchain network requests #1037

Merged

3 tasks

InoMurko approved these changes Oct 8, 2019

View reviewed changes

Pawel Nowosielski and others added 4 commits October 8, 2019 12:13

test: remove metamask compliance tests

26b4ae3

feat: allow to produce and start SE challenges from the watcher

0a248e1

to squash: get rid of getting creating blocks when challenging SEs

feat: include failure reason in return value from transact_sync!

605eb93

fix: fix problem with RootChain.maybe_fetch_addr! override

5ed023b

Piotr Dobaczewski Imapp added 5 commits October 8, 2019 12:13

feat: update the EIP712 signature/hashing schemes and conformance test

b1417c1

docs: resolve the issue with scanning exit statuses (it works, actually)

f233554

feat: integrate with updated ALD contracts

ccadaf2

new PaymentExitCondition contract structure + libs the `token` field has been dropped from `standardExits` call return value `challengeStandardExit` doesn't require challenge_tx_type anymore need to register the spending conditions properly

refactor: move the fallback contract address handling to test-only code

7acee03

following a comment from CR

style: abandon the confusing clauses to Deployer.create_new for modules

a2b7afc

pdobacz force-pushed the 986_integrate_SE_challenges branch from ff52cac to a2b7afc Compare October 8, 2019 10:15

pdobacz merged commit 2df08a1 into master Oct 8, 2019

pdobacz deleted the 986_integrate_SE_challenges branch October 8, 2019 11:13

pdobacz pushed a commit that referenced this pull request Oct 9, 2019

fix: update after rebasing, remove hackarounds after #1005

170a4d3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrate se challenges from ALD with elixir-omg #1005

integrate se challenges from ALD with elixir-omg #1005

pdobacz commented Sep 25, 2019 •

edited

Loading

coveralls commented Sep 25, 2019 •

edited

Loading

pdobacz commented Sep 25, 2019 •

edited

Loading

pnowosie commented Oct 2, 2019

pdobacz commented Oct 2, 2019

pdobacz Oct 3, 2019 •

edited

Loading

InoMurko Oct 3, 2019

pdobacz Oct 4, 2019

InoMurko Oct 3, 2019

pdobacz Oct 4, 2019

pdobacz Oct 4, 2019

InoMurko Oct 3, 2019

pdobacz Oct 4, 2019

InoMurko Oct 3, 2019

pdobacz Oct 4, 2019

InoMurko Oct 3, 2019

pdobacz Oct 4, 2019

InoMurko Oct 7, 2019 •

edited

Loading

pdobacz Oct 7, 2019

InoMurko Oct 7, 2019

pdobacz Oct 7, 2019

InoMurko Oct 7, 2019

pdobacz Oct 7, 2019

InoMurko Oct 7, 2019

pdobacz Oct 7, 2019

InoMurko Oct 7, 2019

pdobacz Oct 7, 2019

InoMurko Oct 7, 2019

pdobacz Oct 7, 2019

InoMurko Oct 7, 2019

pdobacz Oct 7, 2019

InoMurko Oct 7, 2019

pdobacz Oct 7, 2019

		@@ -47,6 +47,7 @@ defmodule OMG.TypedDataHash do
		@spec hash_transaction(Transaction.Payment.t()) :: Crypto.hash_t()
		def hash_transaction(%Transaction.Payment{} = raw_tx) do


		assert solidity_signer == elixir_signer
		assert solidity_hash == OMG.TypedDataHash.hash_struct(tx)

integrate se challenges from ALD with elixir-omg #1005

integrate se challenges from ALD with elixir-omg #1005

Conversation

pdobacz commented Sep 25, 2019 • edited Loading

Overview

Changes

Testing

coveralls commented Sep 25, 2019 • edited Loading

pdobacz commented Sep 25, 2019 • edited Loading

pnowosie commented Oct 2, 2019

pdobacz commented Oct 2, 2019

pdobacz Oct 3, 2019 • 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

InoMurko Oct 7, 2019 • 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

pdobacz commented Sep 25, 2019 •

edited

Loading

coveralls commented Sep 25, 2019 •

edited

Loading

pdobacz commented Sep 25, 2019 •

edited

Loading

pdobacz Oct 3, 2019 •

edited

Loading

InoMurko Oct 7, 2019 •

edited

Loading