Change EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS

[souradeep-das]

#1399

Overview

This unifies sla_margin to hold values in seconds instead of the number of blocks and renames EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS. The internal logic to detect invalid exits has also been changed to use timestamps instead of number of blocks

EXIT_PROCESSOR_SLA_MARGIN_FORCED hasn't been renamed

Changes

• EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS
• Changing eth_height_now to eth_timestamp_now
• Changed logic to work with seconds
• Modified Tests
• EXIT_PROCESSOR_SLA_MARGIN_FORCED is unchanged till now

[coveralls]

Coverage increased (+0.009%) to 78.295% when pulling 99cf0720811c482a9d94b025f492020e25cc7bc2 on souradeep/margin_to_seconds into 2e2893cb6dd7f6e991e2d65b571e4a066b73ca7b on master.

[unnawut]

Also, could you add this change to https://github.com/omisego/elixir-omg/blob/master/CHANGELOG.md under the Unreleased header?

[unnawut]

@souradeep-das Is this ready for another review?

[souradeep-das]

@unnawut Almost, reviewing all the changes myself and then requesting for your reviews.

[souradeep-das]

Also @pnowosie asking for a re-review to confirm the latest changes. Thanks :bow:

[unnawut]

Adding upgrade path required label so we can collect them up and inform infra/users easily of manual upgrade tasks needed.

[souradeep-das]

@unnawut do we need to update any env vars?

[unnawut]

@unnawut do we need to update any env vars?

Yeah the ones you have in the infra PR should suffice

[souradeep-das]

Hey @InoMurko! Thanks for the review :bow: So, eth_timestamp_now is mostly storing the timestamp ( eth_height_now ) replacing eth_height_now we had previously.

Previously we had -

late_invalid_exits =
      Enum.filter(invalid_exits, fn {_, %ExitInfo{eth_height: eth_height}} ->
        eth_height + sla_margin eth_height_now

Since, we added the exit timestamp already, using it for the check and checking with timestamp(eth_height_now)

late_invalid_exits =
       Enum.filter(invalid_exits, fn {_, %ExitInfo{block_timestamp: block_timestamp}} ->
        block_timestamp + sla_seconds eth_timestamp_now

assigning the actual timestamp of the block here, https://github.com/omgnetwork/elixir-omg/pull/1491/files#diff-f1ce1871bf4b96d60bda0fbd02e3891cR632

For the tests, trying for the exit_timestamp to be similar (in value) to the exit_height, mostly fixing the value of the exit_timestamp for the tests to constant like we do for eth_height. And then passing values for eth_timestamp_now as the number of seconds that is ++ from the block_timestamp (exit_timestamp) https://github.com/omgnetwork/elixir-omg/pull/1491/files#diff-feeb2e7cdff5093e568214f358154985R46

Should eth_timestamp_now be renamed for clarity?

[souradeep-das]

Do we need a successive change for the change in API endpoint? @InoMurko @unnawut @T-Dnzt

[unnawut]

Do we need a successive change for the change in API endpoint? @InoMurko @unnawut @T-Dnzt

@T-Dnzt @achiurizo I think we should get a thumbs up from either of you for any API breaking change.

It's the watcher & watcher_info /configuration.get's exit_processor_sla_margin -> exit_processor_sla_seconds. I don't think there's a workaround e.g. keeping the old key-value, because the value would no longer applicable.

So I guess either:

1. Scrap this proposal. Downside is we have to stick with sla_margin which is different from the contract's sla_seconds
2. A 👍 from @T-Dnzt @achiurizo and the release manager making sure the breaking change is in the release comms.

I recommend 2 for consistency with the contracts outweighs dropping this key from /configuration.get