Merge pull request #852 from omisego/815-more_forgiving_handling_of_c…

…orrupted_fee_file

fix: change fee_server not to crash on corrupted json

apps/omg/lib/omg/alert/alarm.ex

@@ -21,24 +21,25 @@ defmodule OMG.Alert.Alarm do
   @typedoc """
   The raw alarm being used to `set` the Alarm
   """
-  @type raw_t :: {:boot_in_progress | :ethereum_client_connection, node(), module()}
+  @type raw_t ::
+          {:boot_in_progress
+           | :ethereum_client_connection
+           | :invalid_fee_file, node(), module()}
 
   def ethereum_client_connection_issue(node, reporter),
     do: {:ethereum_client_connection, %{node: node, reporter: reporter}}
 
   def boot_in_progress(node, reporter),
     do: {:boot_in_progress, %{node: node, reporter: reporter}}
 
+  def invalid_fee_file(node, reporter),
+    do: {:invalid_fee_file, %{node: node, reporter: reporter}}
+
   @spec set(raw_t()) :: :ok | :duplicate
-  def set(raw_alarm = {_, node, reporter}) when is_atom(node) and is_atom(reporter) do
-    alarm = make_alarm(raw_alarm)
-    do_raise(alarm)
-  end
+  def set(raw_alarm), do: raw_alarm |> make_alarm() |> do_raise()
 
-  def clear(raw_alarm) do
-    make_alarm(raw_alarm)
-    |> :alarm_handler.clear_alarm()
-  end
+  @spec clear(raw_t()) :: :ok | :not_raised
+  def clear(raw_alarm), do: raw_alarm |> make_alarm() |> do_clear()
 
   def clear_all do
     all_raw()
@@ -56,16 +57,30 @@ defmodule OMG.Alert.Alarm do
       else: :alarm_handler.set_alarm(alarm)
   end
 
+  defp do_clear(alarm) do
+    if Enum.member?(all_raw(), alarm),
+      do: :alarm_handler.clear_alarm(alarm),
+      else: :not_raised
+  end
+
   defp format_alarm({id, details}), do: %{id: id, details: details}
   defp format_alarm(alarm), do: %{id: alarm}
 
   defp all_raw, do: :gen_event.call(:alarm_handler, AlarmHandler, :get_alarms)
 
-  defp make_alarm({:ethereum_client_connection, node, reporter}) do
+  @spec make_alarm(raw_t()) :: {atom(), %{node: node(), reporter: module()}}
+  defp make_alarm(raw_alarm = {_, node, reporter}) when is_atom(node) and is_atom(reporter),
+    do: make_alarm_for(raw_alarm)
+
+  defp make_alarm_for({:ethereum_client_connection, node, reporter}) do
     ethereum_client_connection_issue(node, reporter)
   end
 
-  defp make_alarm({:boot_in_progress, node, reporter}) do
+  defp make_alarm_for({:boot_in_progress, node, reporter}) do
     boot_in_progress(node, reporter)
   end
+
+  defp make_alarm_for({:invalid_fee_file, node, reporter}) do
+    invalid_fee_file(node, reporter)
+  end
 end

apps/omg/lib/omg/alert/alarm_handler.ex

@@ -25,7 +25,7 @@ defmodule OMG.Alert.AlarmHandler do
   end
 
   # subscribing to alarms of type
-  def alarm_types, do: [:ethereum_client_connection, :boot_in_progress]
+  def alarm_types, do: [:ethereum_client_connection, :boot_in_progress, :invalid_fee_file]
 
   def init(_args) do
     {:ok, %{alarms: []}}

apps/omg/lib/omg/fees.ex

@@ -64,15 +64,17 @@ defmodule OMG.Fees do
   """
   @spec parse_file_content(binary()) :: {:ok, fee_t()} | {:error, list({:error, atom()})}
   def parse_file_content(file_content) do
-    {:ok, json} = Jason.decode(file_content)
-
-    {errors, token_fee_map, _} =
-      json
-      |> Enum.map(&parse_fee_spec/1)
-      |> Enum.reduce({[], %{}, 1}, &spec_reducer/2)
-
-    {Enum.reverse(errors), token_fee_map}
-    |> handle_parser_output()
+    with {:ok, json} <- Jason.decode(file_content) do
+      {errors, token_fee_map, _} =
+        json
+        |> Enum.map(&parse_fee_spec/1)
+        |> Enum.reduce({[], %{}, 1}, &spec_reducer/2)
+
+      errors
+      |> Enum.reverse()
+      |> (&{&1, token_fee_map}).()
+      |> handle_parser_output()
+    end
   end
 
   defp is_merge_transaction?(recovered_tx) do

apps/omg/test/support/test_helper.ex

@@ -143,18 +143,24 @@ defmodule OMG.TestHelper do
   @doc """
   Always creates file in the priv/ folder of the application.
   """
-  @spec write_fee_file(%{Crypto.address_t() => non_neg_integer}) :: {:ok, binary, binary}
-  def write_fee_file(map) do
+  @spec write_fee_file(%{Crypto.address_t() => non_neg_integer} | binary(), binary() | nil) :: {:ok, binary, binary}
+  def write_fee_file(fee_map, file_name \\ nil)
+
+  def write_fee_file(map, file_name) when is_map(map) do
     {:ok, json} =
       map
       |> Enum.map(fn {"0x" <> _ = k, v} -> %{token: k, flat_fee: v} end)
       |> Jason.encode()
 
+    write_fee_file(json, file_name)
+  end
+
+  def write_fee_file(content, file_name) do
     priv_dir = :code.priv_dir(:omg_child_chain)
-    file = "omisego_operator_test_fees_file-#{DateTime.to_unix(DateTime.utc_now())}"
+    file = file_name || "omisego_operator_test_fees_file-#{DateTime.to_unix(DateTime.utc_now())}"
     full_path = "#{priv_dir}/#{file}"
 
-    :ok = File.write(full_path, json, [:write])
+    :ok = File.write(full_path, content, [:write])
     {:ok, full_path, file}
   end
 

apps/omg_child_chain/lib/omg_child_chain/fee_server.ex

@@ -15,36 +15,42 @@
 defmodule OMG.ChildChain.FeeServer do
   @moduledoc """
   Maintains current fee rates and tokens in which fees may be paid.
-  Updates fees information from external source.
-  Provides function to validate transaction's fee.
+  Periodically updates fees information from external source (file in omg_child_chain/priv config :omg_child_chain,
+  :fee_specs_file_name) until switched off with config :omg_child_chain, :ignore_fees.
+
+  Fee's file parsing and rules of transaction's fee validation are in `OMG.Fees`
   """
 
+  alias OMG.Alert.Alarm
   alias OMG.Fees
 
   use GenServer
   use OMG.Utils.LoggerExt
 
-  @file_changed_check_interval_ms 10_000
+  @file_check_default_interval_ms 10_000
 
-  def start_link(_args) do
-    GenServer.start_link(__MODULE__, :ok, name: __MODULE__)
+  def start_link(opts) do
+    GenServer.start_link(__MODULE__, opts, name: __MODULE__)
   end
 
   def init(args) do
     :ok = ensure_ets_init()
 
-    _ =
+    args =
       case Application.get_env(:omg_child_chain, :ignore_fees) do
         true ->
           :ok = save_fees(:ignore, 0)
           _ = Logger.warn("Fee specs from file are ignored.")
+          args
 
         opt when is_nil(opt) or is_boolean(opt) ->
           :ok = update_fee_spec()
-          {:ok, _} = :timer.apply_interval(@file_changed_check_interval_ms, __MODULE__, :update_fee_spec, [])
+          interval = Keyword.get(args, :interval_ms, @file_check_default_interval_ms)
+          {:ok, tref} = :timer.send_interval(interval, self(), :update_fee_spec)
+          Keyword.put(args, :tref, tref)
       end
 
-    _ = Logger.info("Started FeeServer")
+    _ = Logger.info("Started #{inspect(__MODULE__)}")
     {:ok, args}
   end
 
@@ -56,34 +62,44 @@ defmodule OMG.ChildChain.FeeServer do
     {:ok, load_fees()}
   end
 
-  @doc """
-   Reads fee specification file if needed and updates :ets state with current fees information
-  """
-  @spec update_fee_spec() :: :ok
-  def update_fee_spec do
-    path = get_fees()
+  def handle_info(:update_fee_spec, state) do
+    _ =
+      if Application.get_env(:omg_child_chain, :ignore_fees) do
+        _ = Logger.warn("Fee specs from file are ignored. Updates takes no effect.")
+      else
+        alarm = {:invalid_fee_file, Node.self(), __MODULE__}
 
-    :ok =
-      with {:reload, changed_at} <- should_load_file(path),
-           {:ok, content} <- File.read(path),
-           {:ok, specs} <- Fees.parse_file_content(content) do
-        :ok = save_fees(specs, changed_at)
-        _ = Logger.info("Reloaded #{inspect(Enum.count(specs))} fee specs from file, changed at #{inspect(changed_at)}")
+        case update_fee_spec() do
+          :ok ->
+            Alarm.clear(alarm)
 
-        :ok
-      else
-        {:file_unchanged, _last_change_at} ->
-          :ok
+          _ ->
+            Alarm.set(alarm)
+        end
+      end
+
+    {:noreply, state}
+  end
 
-        {:error, :enoent} ->
-          _ = Logger.error("The fee specification file #{inspect(path)} not found.")
+  # Reads fee specification file if needed and updates :ets state with current fees information
+  @spec update_fee_spec() :: :ok | {:error, atom() | [{:error, atom()}, ...]}
+  defp update_fee_spec do
+    path = get_fees()
 
-          {:error, :fee_spec_not_found}
+    with {:reload, changed_at} <- should_load_file(path),
+         {:ok, content} <- File.read(path),
+         {:ok, specs} <- Fees.parse_file_content(content) do
+      :ok = save_fees(specs, changed_at)
+      _ = Logger.info("Reloaded #{inspect(Enum.count(specs))} fee specs from file, changed at #{inspect(changed_at)}")
+      :ok
+    else
+      {:file_unchanged, _last_change_at} ->
+        :ok
 
-        error ->
-          _ = Logger.error("Unable to update fees from file. Reason: #{inspect(error)}")
-          error
-      end
+      error ->
+        _ = Logger.error("Unable to update fees from file. Reason: #{inspect(error)}")
+        error
+    end
   end
 
   defp save_fees(fee_specs, loaded_at) do