Refactor code & tests for OMG.Fees
MODIFY
apps/omg/lib/omg/fees.ex
apps/omg/lib/omg/fees.ex
defmodule OMG.Fees do
@moduledoc """
- Transaction's fee validation functions
+ Transaction's fee validation functions.
"""
+ alias OMG.FeesValidator
+ alias OMG.MergeTransactionValidator
alias OMG.State.Transaction
alias OMG.Utxo
@@ -25,13 +27,13 @@ defmodule OMG.Fees do
use OMG.Utils.LoggerExt
@type fee_spec_t() :: %{token: Transaction.Payment.currency(), flat_fee: non_neg_integer}
- @type fee_t() :: %{Transaction.Payment.currency() => non_neg_integer} | :ignore
+ @type fee_t() :: %{Transaction.Payment.currency() => non_neg_integer} | :no_fees_transaction
@doc """
- Checks whether transaction's funds cover the fee
+ Checks whether the transaction's inputs cover the fees.
"""
@spec covered?(implicit_paid_fee_by_currency :: map(), fees :: fee_t()) :: boolean()
- def covered?(_, :ignore), do: true
+ def covered?(_, :no_fees_transaction), do: true
def covered?(implicit_paid_fee_by_currency, fees) do
for {input_currency, implicit_paid_fee} <- implicit_paid_fee_by_currency do
@@ -44,54 +46,18 @@ defmodule OMG.Fees do
end
@doc """
- Returns fees to require for a particular transaction, and under particular fee specs listed in `fee_map`
+ Returns the fees to pay for a particular transaction,
+ and under particular fee specs listed in `fee_map`.
"""
- @spec for_tx(Transaction.Recovered.t(), fee_t()) :: fee_t()
- def for_tx(tx, fee_map) do
- if is_merge_transaction?(tx),
- do: :ignore,
- # TODO: reducing fees to output currencies only is incorrect, let's deffer until fees get large
- else: fee_map
- end
-
- defp is_merge_transaction?(recovered_tx) do
- [
- &is_payment?/1,
- &has_less_outputs_than_inputs?/1,
- &has_single_currency?/1,
- &has_same_account?/1
- ]
- |> Enum.all?(fn predicate -> predicate.(recovered_tx) end)
- end
-
- defp is_payment?(%Transaction.Recovered{signed_tx: %{raw_tx: %Transaction.Payment{}}}), do: true
- defp is_payment?(_), do: false
-
- defp has_same_account?(%Transaction.Recovered{witnesses: witnesses} = tx) do
- spenders = Map.values(witnesses)
+ @spec for_transaction(Transaction.Recovered.t(), fee_t()) :: fee_t()
+ def for_transaction(transaction, fee_map) do
+ case MergeTransactionValidator.is_merge_transaction?(transaction) do
+ true ->
+ :no_fees_transaction
- tx
- |> Transaction.get_outputs()
- |> Enum.map(& &1.owner)
- |> Enum.concat(spenders)
- |> single?()
- end
-
- defp has_single_currency?(tx) do
- tx
- |> Transaction.get_outputs()
- |> Enum.map(& &1.currency)
- |> single?()
+ # TODO: reducing fees to output currencies only is incorrect, let's deffer until fees get large
+ false ->
+ fee_map
end
-
- defp has_less_outputs_than_inputs?(tx) do
- has_less_outputs_than_inputs?(
- Transaction.get_inputs(tx),
- Transaction.get_outputs(tx)
- )
end
-
- defp has_less_outputs_than_inputs?(inputs, outputs), do: length(inputs) >= 1 and length(inputs) > length(outputs)
-
- defp single?(list), do: 1 == list |> Enum.dedup() |> length()
end

MODIFY
apps/omg/lib/omg/state/transaction/validator.ex
apps/omg/lib/omg/state/transaction/validator.ex
@@ -20,6 +20,7 @@ defmodule OMG.State.Transaction.Validator do
@maximum_block_size 65_536
alias OMG.Fees
+ alias OMG.FeesValidator
alias OMG.State.Core
alias OMG.State.Transaction
alias OMG.State.UtxoSet

ADD
apps/omg/lib/omg/validators/merge_transaction_validator.ex
+# Copyright 2019 OmiseGO Pte Ltd
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+defmodule OMG.MergeTransactionValidator do
+ @moduledoc """
+ Checks for transaction attributes. Used by OMG.Fees.
+ """
+
+ alias OMG.State.Transaction
+ alias OMG.Utxo
+
+ require Utxo
+
+ use OMG.Utils.LoggerExt
+
+ def is_merge_transaction?(recovered_transaction) do
+ [
+ &is_payment?/1,
+ &has_less_outputs_than_inputs?/1,
+ &has_single_currency?/1,
+ &has_same_account?/1
+ ]
+ |> Enum.all?(fn predicate -> predicate.(recovered_transaction) end)
+ end
+
+ defp is_payment?(%Transaction.Recovered{signed_tx: %{raw_tx: %Transaction.Payment{}}}), do: true
+ defp is_payment?(_), do: false
+
+ defp has_same_account?(%Transaction.Recovered{witnesses: witnesses} = tx) do
+ spenders = Map.values(witnesses)
+
+ tx
+ |> Transaction.get_outputs()
+ |> Enum.map(& &1.owner)
+ |> Enum.concat(spenders)
+ |> single?()
+ end
+
+ defp has_single_currency?(tx) do
+ tx
+ |> Transaction.get_outputs()
+ |> Enum.map(& &1.currency)
+ |> single?()
+ end
+
+ defp has_less_outputs_than_inputs?(tx) do
+ has_less_outputs_than_inputs?(
+ Transaction.get_inputs(tx),
+ Transaction.get_outputs(tx)
+ )
+ end
+
+ defp has_less_outputs_than_inputs?(inputs, outputs), do: length(inputs) >= 1 and length(inputs) > length(outputs)
+
+ defp single?(list), do: 1 == list |> Enum.dedup() |> length()
+end

MODIFY
apps/omg/test/omg/fees_test.exs
apps/omg/test/omg/fees_test.exs
@@ -19,9 +19,10 @@ defmodule OMG.FeesTest do
use OMG.Fixtures
use ExUnit.Case, async: true
- import OMG.Fees
import OMG.TestHelper
+ alias OMG.Fees
+
@eth OMG.Eth.RootChain.eth_pseudo_address()
@not_eth <<1::size(160)>>
@@ -30,87 +31,88 @@ defmodule OMG.FeesTest do
@not_eth => 3
}
- # TODO: brittle test? why not test via public API (State.Core)?
- @tag fixtures: [:alice, :bob]
- test "Transactions covers the fee only in one currency accepted by the operator", %{alice: alice, bob: bob} do
- fees =
- create_recovered([{1, 0, 0, alice}], @eth, [{bob, 6}, {alice, 3}])
- |> for_tx(@fees)
+ describe "covered?/2" do
+ test "does not check the fees when :no_fees_transaction is passed" do
+ assert Fees.covered?(%{@eth => 0}, :no_fees_transaction)
+ end
- assert covered?(%{@eth => 1}, fees)
+ test "returns true when fees are covered by eth" do
+ assert Fees.covered?(%{@eth => 2}, @fees)
+ end
- fees =
- create_recovered([{1, 0, 0, alice}], @not_eth, [{bob, 4}, {alice, 3}])
- |> for_tx(@fees)
+ test "returns true when fees are covered by another currency" do
+ assert Fees.covered?(%{@not_eth => 5}, @fees)
+ end
- assert covered?(%{@not_eth => 3}, fees)
+ test "returns true when multiple implicit fees are given and fee is covered by eth" do
+ assert Fees.covered?(%{@eth => 2, @not_eth => 2}, @fees)
+ end
- fees =
- create_recovered(
- [{1, 0, 0, alice}, {2, 0, 0, alice}],
- [{bob, @eth, 4}, {alice, @eth, 1}, {bob, @not_eth, 5}, {alice, @not_eth, 5}]
- )
- |> for_tx(@fees)
+ test "returns true when multiple implicit fees are given and fee is covered by another currency" do
+ assert Fees.covered?(%{@eth => 0.5, @not_eth => 4}, @fees)
+ end
- assert covered?(%{@eth => 0, @not_eth => 3}, fees)
+ test "returns false when the implicit fees currency does not match any of the supported fee currencies" do
+ other_currency = <<2::160>>
+ refute Fees.covered?(%{other_currency => 100}, @fees)
end
+ # TODO: Fix this test?
@tag fixtures: [:alice, :bob]
- test "Transaction which does not transfer any fee currency is object to fees", %{alice: alice, bob: bob} do
+ test "returns true when one input is dedicated for fee payment", %{alice: alice, bob: bob} do
other_token = <<2::160>>
- fees =
- create_recovered([{1, 0, 0, alice}], other_token, [{bob, 5}, {alice, 3}])
- |> for_tx(@fees)
+ transaction =
+ create_recovered([{1, 0, 0, alice}, {2, 0, 0, alice}], [{bob, other_token, 5}, {alice, other_token, 5}])
+
+ fees = Fees.for_transaction(transaction, @fees)
- assert false == covered?(%{other_token => 3}, fees)
+ assert Fees.covered?(%{@not_eth => 5, other_token => 0}, fees)
+ end
end
+ describe "for_transaction/2" do
@tag fixtures: [:alice, :bob]
- test "Transaction can dedicate one input for a fee entirely, reducing to tx's outputs currencies is incorrect",
- %{alice: alice, bob: bob} do
- other_token = <<2::160>>
-
- fees =
- create_recovered([{1, 0, 0, alice}, {2, 0, 0, alice}], [{bob, other_token, 5}, {alice, other_token, 5}])
- |> for_tx(@fees)
-
- assert covered?(%{@not_eth => 5, other_token => 0}, fees)
+ test "returns the fee map when not a merge transaction (different addresses)", %{alice: alice, bob: bob} do
+ transaction = create_recovered([{1, 0, 0, alice}], @eth, [{bob, 6}, {alice, 3}])
+ assert Fees.for_transaction(transaction, @fees) == @fees
end
- describe "Merge transactions are free of cost" do
@tag fixtures: [:alice]
- test "merging utxo erases the fee", %{alice: alice} do
- fees =
- create_recovered([{1, 0, 0, alice}, {2, 0, 0, alice}], @not_eth, [{alice, 10}])
- |> for_tx(@fees)
-
- assert covered?(%{@not_eth => 0}, fees)
+ test "returns :no_fees_transaction for merge transactions with single currency (eth) and same in/out address (free)",
+ %{alice: alice} do
+ transaction = create_recovered([{1, 0, 0, alice}, {2, 0, 0, alice}], @eth, [{alice, 10}])
+ assert Fees.for_transaction(transaction, @fees) == :no_fees_transaction
end
@tag fixtures: [:alice]
- test "merge is single currency transaction", %{alice: alice} do
- fees =
+ test "returns :no_fees_transaction for valid merge transactions with multiple inputs/ouputs",
+ %{alice: alice} do
+ transaction =
create_recovered(
[{1, 0, 0, alice}, {1, 0, 1, alice}, {2, 0, 0, alice}, {2, 1, 0, alice}],
- [{alice, @eth, 10}, {alice, @not_eth, 10}]
+ [{alice, @eth, 10}, {alice, @eth, 10}]
)
- |> for_tx(@fees)
- assert not covered?(%{@eth => 0}, fees)
+ assert Fees.for_transaction(transaction, @fees) == :no_fees_transaction
end
- @tag fixtures: [:alice, :bob]
- test "merge is single same address transaction", %{alice: alice, bob: bob} do
- fees =
+ @tag fixtures: [:alice]
+ test "returns :no_fees_transaction for merge transactions with single currency (not eth) and same in/out address (free)",
+ %{alice: alice} do
+ transaction = create_recovered([{1, 0, 0, alice}, {2, 0, 0, alice}], @not_eth, [{alice, 10}])
+ assert Fees.for_transaction(transaction, @fees) == :no_fees_transaction
+ end
+
+ @tag fixtures: [:alice]
+ test "returns fees for merge transactions when not single currency", %{alice: alice} do
+ transaction =
create_recovered(
- [{1, 0, 0, alice}, {1, 0, 1, alice}, {2, 0, 0, alice}],
- @eth,
- [{alice, 5}, {bob, 5}]
+ [{1, 0, 0, alice}, {1, 0, 1, alice}, {2, 0, 0, alice}, {2, 1, 0, alice}],
+ [{alice, @eth, 10}, {alice, @not_eth, 10}]
)
- |> for_tx(@fees)
- assert not covered?(%{@eth => 0}, fees)
+ assert Fees.for_transaction(transaction, @fees) == @fees
end
end
end

MODIFY
apps/omg/test/omg/state/core_test.exs
apps/omg/test/omg/state/core_test.exs
@@ -151,7 +151,7 @@ defmodule OMG.State.CoreTest do
test "Merge transaction is fee free", %{alice: alice, state_empty: state} do
fees = %{@eth => 2}
tx = create_recovered([{1, 0, 0, alice}, {2, 0, 0, alice}], @eth, [{alice, 15}])
- fee = Fees.for_tx(tx, fees)
+ fee = Fees.for_transaction(tx, fees)
state
|> do_deposit(alice, %{amount: 10, currency: @eth, blknum: 1})

ADD
apps/omg/test/omg/validators/merge_transaction_validator_test.exs
+# Copyright 2019 OmiseGO Pte Ltd
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+defmodule OMG.MergeTransactionValidatorTest do
+ @moduledoc false
+
+ use ExUnitFixtures
+ use ExUnit.Case, async: true
+
+ import OMG.TestHelper
+
+ alias OMG.MergeTransactionValidator
+ alias OMG.State.Transaction
+
+ @eth OMG.Eth.RootChain.eth_pseudo_address()
+ @not_eth <<1::size(160)>>
+
+ describe "is_merge_transaction?/1" do
+ @tag fixtures: [:alice]
+ test "returns true when the transaction is a payment, has less outputs than inputs, has single currency, and has same account",
+ %{alice: alice} do
+ transaction = create_recovered([{1, 0, 0, alice}, {2, 0, 0, alice}], @eth, [{alice, 10}])
+ assert MergeTransactionValidator.is_merge_transaction?(transaction)
+ end
+
+ test "returns false when transaction is not of payment type" do
+ refute MergeTransactionValidator.is_merge_transaction?(%Transaction.Recovered{signed_tx: %{raw_tx: "fake"}})
+ end
+
+ @tag fixtures: [:alice]
+ test "returns false when transaction has as many outputs than inputs", %{alice: alice} do
+ transaction = create_recovered([{1, 0, 0, alice}, {2, 0, 0, alice}], @eth, [{alice, 5}, {alice, 5}])
+ refute MergeTransactionValidator.is_merge_transaction?(transaction)
+ end
+
+ @tag fixtures: [:alice]
+ test "returns false when transaction has more outputs than inputs", %{alice: alice} do
+ transaction = create_recovered([{1, 0, 0, alice}, {2, 0, 0, alice}], @eth, [{alice, 3}, {alice, 3}, {alice, 4}])
+ refute MergeTransactionValidator.is_merge_transaction?(transaction)
+ end
+
+ @tag fixtures: [:alice]
+ test "returns false when not a single currency", %{alice: alice} do
+ transaction =
+ create_recovered(
+ [{1, 0, 0, alice}, {1, 0, 1, alice}, {2, 0, 0, alice}, {2, 1, 0, alice}],
+ [{alice, @eth, 10}, {alice, @not_eth, 10}]
+ )
+
+ refute MergeTransactionValidator.is_merge_transaction?(transaction)
+ end
+
+ @tag fixtures: [:alice, :bob]
+ test "returns false when two different accounts in outputs", %{alice: alice, bob: bob} do
+ transaction =
+ create_recovered([{1, 0, 0, alice}, {1, 0, 1, alice}, {2, 0, 0, alice}], @eth, [{bob, 10}, {alice, 10}])
+
+ refute MergeTransactionValidator.is_merge_transaction?(transaction)
+ end
+ end
+end

MODIFY
apps/omg_child_chain/lib/child_chain.ex
apps/omg_child_chain/lib/child_chain.ex
@@ -37,7 +37,7 @@ defmodule OMG.ChildChain do
def submit(transaction) do
with {:ok, recovered_tx} <- Transaction.Recovered.recover_from(transaction),
{:ok, fees} <- FeeServer.transaction_fees(),
- fees = Fees.for_tx(recovered_tx, fees),
+ fees = Fees.for_transaction(recovered_tx, fees),
{:ok, {tx_hash, blknum, tx_index}} <- State.exec(recovered_tx, fees) do
{:ok, %{txhash: tx_hash, blknum: blknum, txindex: tx_index}}
end

Refactor code & tests for OMG.Fees