feat: configurable fee specs path from env var #1385

mederic-p · 2020-03-11T06:58:23Z

Overview

This PR allows the fee specs path to be configured via the environment variable FEE_SPECS_FILE_PATH.

Changes

Updated OMG.ChildChain.Fees.FileAdapter and OMG.ChildChain.ReleaseTasks.SetFeeFileAdapterOpts to support passing FEE_SPECS_FILE_PATH environment variable
Updated Fees.FileAdapter and Fees.FeedAdapter to support FEE_ADAPTER=file and FEE_ADAPTER=feed respectively so it can be changed without a new compile & version release.
Added tests for SetFeeFeedAdapterOpts and fixed the code resulting from test failures

Usage

On the childchain, set FEE_ADAPTER and FEE_SPECS_FILE_PATH:

export FEE_ADAPTER=file
export FEE_SPECS_FILE_PATH=/path/to/fee/specs.json

Testing

Unit tests pass: mix test test/omg_child_chain/release_tasks/set_fee_file_adapter_opts_test.exs test/omg_child_chain/release_tasks/set_fee_feed_adapter_opts_test.exs
Specs test pass: cd priv/cabbage && mix test

Fees are reflected from the fee specs file e.g. the specs file:

{
    "1": [{
        "token": "0x0000000000000000000000000000000000000000",
        "amount": 3256762,
        "subunit_to_unit": 1000000000000000000,
        "pegged_amount": null,
        "pegged_currency": null,
        "pegged_subunit_to_unit": null,
        "updated_at": "2019-01-01T10:10:00+00:00"
    }]
}

Gives

$ curl -X POST http://localhost:7534/fees.al
-H "Content-Type: application/json" \
-d '{}' \
-v -w "\n" | jq

{
  "data": {
    "1": [
      {
        "amount": 3256762,
        "currency": "0x0000000000000000000000000000000000000000",
        "pegged_amount": null,
        "pegged_currency": null,
        "pegged_subunit_to_unit": null,
        "subunit_to_unit": 1e+18,
       "updated_at": "2019-01-01T10:10:00Z"
      }
    ]
  },
  "service_name": "watcher_info",
  "success": true,
  "version": "0.4.6+280022b"
}

coveralls · 2020-03-11T10:31:33Z

Coverage increased (+0.3%) to 77.917% when pulling 14ee7cd on mederic-p/1353-fee-file-path into 6a26336 on master.

…file-path

# Conflicts: # apps/omg_child_chain/test/omg_child_chain/integration/fee_server_test.exs

# Conflicts: # apps/omg_child_chain/config/config.exs # apps/omg_child_chain/lib/omg_child_chain/fees/file_adapter.ex # apps/omg_child_chain/test/omg_child_chain/fees/file_adapter_test.exs # apps/omg_child_chain/test/omg_child_chain/integration/fee_server_test.exs # apps/omg_child_chain/test/omg_child_chain/integration/fixtures.exs # apps/omg_watcher/test/fixtures.exs # docs/deployment_configuration.md # docs/details.md

InoMurko · 2020-04-02T06:54:19Z

apps/omg/test/support/test_helper.ex

-  @spec write_fee_file(%{Crypto.address_t() => map()} | binary(), binary() | nil) :: {:ok, binary, binary}
-  def write_fee_file(fee_map, file_name \\ nil)
+  @spec write_fee_file(%{Crypto.address_t() => map()} | binary()) :: {:ok, binary, binary}
+  def write_fee_file(fee_map, file_path \\ nil)


what purpose does this serve though?

The fee specs file is supposed to be updatable but it's residing deep in _build/.../<app-version>/.../omg_child_chain/priv folder making it hard and error prone to update.

So going for a path instead so infra could keep the file somewhere else outside the repo directory and we can refer to it via env var.

do we stil need the default arument? file_path \\ nil?

Looks like we still do because https://github.com/omisego/elixir-omg/blob/master/apps/omg_child_chain/test/omg_child_chain/integration/fee_server_test.exs specifically tests the behaviour when the same file gets updated.

apps/omg_child_chain/lib/omg_child_chain/fees/file_adapter.ex

InoMurko · 2020-04-02T06:57:40Z

apps/omg_watcher/test/fixtures.exs

@@ -90,7 +90,7 @@ defmodule OMG.Watcher.Fixtures do
      config :omg_db, path: "#{db_path}"
      # this causes the inner test child chain server process to log info. To see these logs adjust test's log level
      config :logger, level: :info
-      config :omg_child_chain, fee_adapter_opts: [specs_file_name: "#{fee_file}"]
+      config :omg_child_chain, fee_adapter_opts: [specs_file_path: "#{fee_file}"]


in the last week, this configuration has been changed 4 times :D

Yeah the other big change to fees was from #1373 which turned the file-only into an adapter for file & feed. I had to rebase and fix conflicts a few times 🤦‍♂

pnowosie

I'm curious was there an issue created for this? It isn't linked.

I want to understand more because what I thought is that having fees in file is hard/impossible to change in the production deployment.

I also thought that fee file become obsolete by the feed service.

Thanks for explaination!

apps/omg_child_chain/lib/omg_child_chain/fees/file_adapter.ex

pnowosie · 2020-04-08T12:17:06Z

apps/omg_child_chain/lib/omg_child_chain/release_tasks/set_fee_feed_adapter_opts.ex

@@ -13,65 +13,86 @@
 # limitations under the License.

 defmodule OMG.ChildChain.ReleaseTasks.SetFeeFeedAdapterOpts do
-  @moduledoc false
+  @moduledoc """


apps/omg_child_chain/lib/omg_child_chain/release_tasks/set_fee_file_adapter_opts.ex

# Conflicts: # apps/omg_child_chain/config/config.exs # apps/omg_child_chain/config/test.exs # apps/omg_child_chain/lib/omg_child_chain/release_tasks/set_fee_feed_adapter_opts.ex # apps/omg_watcher/test/fixtures.exs # docs/deployment_configuration.md # rel/config.exs

# Conflicts: # docker-compose.yml

InoMurko · 2020-04-20T13:04:55Z

apps/omg/test/support/test_helper.ex

-  @spec write_fee_file(%{Crypto.address_t() => map()} | binary(), binary() | nil) :: {:ok, binary, binary}
-  def write_fee_file(fee_map, file_name \\ nil)
+  @spec write_fee_file(%{Crypto.address_t() => map()} | binary()) :: {:ok, binary, binary}
+  def write_fee_file(fee_map, file_path \\ nil)


do we stil need the default arument? file_path \\ nil?

InoMurko · 2020-04-20T13:05:49Z

apps/omg/test/support/test_helper.ex


-  def write_fee_file(map, file_name) when is_map(map) do
+  def write_fee_file(map, file_path) when is_map(map) do


why does this first parameter keep on changing it's name?
fee_map, map, content?

🤦 renamed fee_map to a more accurate map_or_content

InoMurko · 2020-04-20T13:06:55Z

apps/omg/test/support/test_helper.ex

-    full_path = "#{priv_dir}/#{file}"
+  def write_fee_file(content, file_path) do
+    file_path =
+      file_path || "#{:code.priv_dir(:omg_child_chain)}/test_fees_file-#{DateTime.to_unix(DateTime.utc_now())}"


maybe this ought to be expanded?
is this a case?

path = case file_path do nil->"#{:code.priv_dir(:omg_child_chain)}/test_fees_file-#{DateTime.to_unix(DateTime.utc_now())}" _ -> file_path

InoMurko · 2020-04-20T13:09:52Z

apps/omg_child_chain/lib/omg_child_chain/release_tasks/set_fee_feed_adapter_opts.ex

+    |> parse_adapter_value()
+    |> case do
+      "FEED" -> configure_feed_adapter(existing_config, config)
+      _ -> config


seems like in this case we're not margining into Config.Reader.merge/2?
is this not tested?

In this case no config is changed so I'm passing the original config back. Refactored in d2b16ca to make this clearer though.

And the test for this is this one https://github.com/omisego/elixir-omg/pull/1385/files#diff-123f6adf27c8ab33907e154dfc96b071R103-R110

ahhh shit. my bad!

InoMurko · 2020-04-20T13:11:48Z

apps/omg_child_chain/lib/omg_child_chain/release_tasks/set_fee_file_adapter_opts.ex

+    |> parse_adapter_value()
+    |> case do
+      "FILE" -> configure_file_adapter(config)
+      _ -> config


same as above, do we not need to Config.Reader.merge/2?

In this case no config is changed so I'm passing the original config back. Refactored in d2b16ca to make this clearer though.

InoMurko · 2020-04-20T13:13:54Z

apps/omg_child_chain/test/omg_child_chain/release_tasks/set_fee_feed_adapter_opts_test.exs

+    :ok = System.put_env(@env_fee_adapter, "feed")
+
+    :ok = System.put_env(@env_fee_change_tolerance_percent, "1.5")
+    assert_raise ArgumentError, fn -> SetFeeFeedAdapterOpts.load([], []) end


is this raising with a message key? do you want to check if's returning your message?

It was a plain ArgumentError from String.to_integer/1. I've updated it to include a message + assertions though! f2157dc

Mederic added 3 commits March 11, 2020 13:10

feat: move fee file path to env var

92f5674

feat: add release task

fd945da

Merge branch 'master' into mederic-p/1353-fee-file-path

af45389

Mederic added 4 commits March 13, 2020 15:08

feat: docker working?

8aa2c8c

Merge remote-tracking branch 'origin/master' into mederic-p/1353-fee-…

9e8f6a9

…file-path

fix: tests

5d085a2

fix: tests

4432a89

unnawut self-assigned this Mar 17, 2020

Merge branch 'master' into mederic-p/1353-fee-file-path

4c0f23f

# Conflicts: # apps/omg_child_chain/test/omg_child_chain/integration/fee_server_test.exs

unnawut mentioned this pull request Mar 27, 2020

feat: Enforce consistent fee_specs.json between releases #1370

Closed

Unnawut Leepaisalsuwanna added 17 commits March 31, 2020 00:32

refactor: use fee_specs_file_path for both dir and file name combined

e531250

test: fix failures

42a6246

style: mix format

8a45c53

test: fix bad matching

8f28303

test: fix function call

dd211f1

feat: add FEE_ADAPTER for file and feed

5e2dd73

test: fix Fees.FileAdapter tests

dcd9d35

fix: make SetFeeFeedAdapterOpts actually work

94ae40e

feat: hint that operation:service_unavailable is due to alarms

c7d3cfd

style: mix format

d850b38

test: add negative tests for SetFeeFeedAdapterOpts

b0ec80e

test: add fee_specs.json for testing

e96ac4e

test: fix test fee_specs.json path

84e4968

style: don't want to underscore large integers in tests

7b28039

feat: add a development fee specs file

f9a6ab1

test: add dev fee specs for barebone tests

8af0bf9

unnawut changed the title ~~feat: move fee file path to env var~~ feat: configurable fee specs path from env var Apr 1, 2020

fix: wrong FEE_SPECS_FILE_PATH env name

af6fd47

unnawut requested review from T-Dnzt and InoMurko April 1, 2020 16:50

unnawut marked this pull request as ready for review April 1, 2020 16:51

InoMurko suggested changes Apr 2, 2020

View reviewed changes

Unnawut Leepaisalsuwanna added 2 commits April 2, 2020 14:53

refactor: use or instead of ||

9aa6778

refactor: turn || into case

b426fe4

pnowosie suggested changes Apr 3, 2020

View reviewed changes

pnowosie approved these changes Apr 8, 2020

View reviewed changes

Unnawut Leepaisalsuwanna and others added 11 commits April 16, 2020 16:59

refactor: use :code.priv_dir/1 instead of Application.app_dir/2

1f76c93

Merge branch 'master' into mederic-p/1353-fee-file-path

aea5df6

Merge branch 'master' into mederic-p/1353-fee-file-path

2522c7d

# Conflicts: # docker-compose.yml

test: fix config provider after elixir upgrade

2275f28

fix: typespecs

d589745

fix: remove obsolete distillery config

c7d4304

feat: add fee file and fee feed adapter opts to release config

4ae01ac

fix: mix_based_child_chain using old fee config opts

f4cc81d

style: mix formatting

705fb66

test: fix fee specs file path in tests

cf814ba

unnawut requested a review from InoMurko April 20, 2020 12:25

InoMurko suggested changes Apr 20, 2020

View reviewed changes

Unnawut Leepaisalsuwanna added 4 commits April 21, 2020 11:26

refactor: more consistent TestHelper.write_fee_file arg names

3cc47a2

refactor: use case instead of ||

a6add40

refactor: make fee adapter config merge more visible

d2b16ca

test: assert raised message for fee feed adapter tests

f2157dc

InoMurko approved these changes Apr 21, 2020

View reviewed changes

Merge branch 'master' into mederic-p/1353-fee-file-path

14ee7cd

unnawut merged commit e209c46 into master Apr 21, 2020

unnawut deleted the mederic-p/1353-fee-file-path branch April 21, 2020 09:27

unnawut added the enhancement New feature or request label Aug 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: configurable fee specs path from env var #1385

feat: configurable fee specs path from env var #1385

mederic-p commented Mar 11, 2020 •

edited by unnawut

Loading

coveralls commented Mar 11, 2020 •

edited

Loading

InoMurko Apr 2, 2020

unnawut Apr 2, 2020

InoMurko Apr 20, 2020

unnawut Apr 21, 2020

InoMurko Apr 2, 2020

unnawut Apr 2, 2020

pnowosie left a comment

pnowosie Apr 8, 2020

InoMurko Apr 20, 2020

InoMurko Apr 20, 2020

unnawut Apr 21, 2020

InoMurko Apr 20, 2020

InoMurko Apr 20, 2020

unnawut Apr 21, 2020

unnawut Apr 21, 2020

InoMurko Apr 21, 2020

InoMurko Apr 20, 2020

unnawut Apr 21, 2020

InoMurko Apr 20, 2020

unnawut Apr 21, 2020


		def write_fee_file(map, file_name) when is_map(map) do
		def write_fee_file(map, file_path) when is_map(map) do

feat: configurable fee specs path from env var #1385

feat: configurable fee specs path from env var #1385

Conversation

mederic-p commented Mar 11, 2020 • edited by unnawut Loading

Overview

Changes

Usage

Testing

coveralls commented Mar 11, 2020 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mederic-p commented Mar 11, 2020 •

edited by unnawut

Loading

coveralls commented Mar 11, 2020 •

edited

Loading