Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: exclude active exiting utxos from calls to /account.get_exitable_utxos #1505

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion apps/omg_watcher/lib/omg_watcher/api/account.ex
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,32 @@ defmodule OMG.Watcher.API.Account do
Module provides operations related to plasma accounts.
"""

require OMG.Utxo

@doc """
Gets all utxos belonging to the given address. Slow operation.
"""
@spec get_exitable_utxos(OMG.Crypto.address_t()) :: list(OMG.State.Core.exitable_utxos())
def get_exitable_utxos(address) do
# OMG.DB.utxos() takes a while.
{:ok, utxos} = OMG.DB.utxos()
standard_exitable_utxos = OMG.State.Core.standard_exitable_utxos(utxos, address)

# OMG.DB.exit_infos() takes a while.
{:ok, standard_exits} = OMG.DB.exit_infos()
active_standard_exiting_utxos = OMG.Watcher.ExitProcessor.Core.active_standard_exiting_utxos(standard_exits)

# active standard exiting utxos are excluded
filter_standard_exiting_utxos(standard_exitable_utxos, active_standard_exiting_utxos)
end

OMG.State.Core.standard_exitable_utxos(utxos, address)
defp filter_standard_exiting_utxos(standard_exitable_utxos, active_standard_exiting_utxos) do
Enum.filter(
standard_exitable_utxos,
fn %{blknum: blknum, txindex: txindex, oindex: oindex} ->
exit_position = OMG.Utxo.position(blknum, txindex, oindex)
not MapSet.member?(active_standard_exiting_utxos, exit_position)
end
)
end
end
2 changes: 1 addition & 1 deletion apps/omg_watcher/lib/omg_watcher/api/status.ex
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ defmodule OMG.Watcher.API.Status do
eth_syncing = syncing?()

validated_child_block_number = get_validated_child_block_number()
# wtf is eth diagnostics?
# Where is eth diagnostics?
pgebal marked this conversation as resolved.
Show resolved Hide resolved
contracts = Configuration.contracts()
contract_addr = contract_map_from_hex(contracts)

Expand Down
12 changes: 12 additions & 0 deletions apps/omg_watcher/lib/omg_watcher/exit_processor/core.ex
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,18 @@ defmodule OMG.Watcher.ExitProcessor.Core do
|> Enum.map(&prepare_in_flight_exit/1)
end

@doc """
Returns a set of utxo positions for standard exiting utxos
"""
@spec active_standard_exiting_utxos(list(map)) :: MapSet.t(Utxo.Position.t())
def active_standard_exiting_utxos(db_exits) do
db_exits
|> Stream.map(&ExitInfo.from_db_kv/1)
|> Stream.filter(fn {_, exit_info} -> exit_info.is_active end)
|> Enum.map(&Kernel.elem(&1, 0))
|> MapSet.new()
end

defp prepare_in_flight_exit({txhash, ife_info}) do
%{tx: tx, eth_height: eth_height} = ife_info

Expand Down
6 changes: 3 additions & 3 deletions apps/omg_watcher/lib/omg_watcher/exit_processor/exit_info.ex
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ defmodule OMG.Watcher.ExitProcessor.ExitInfo do
defp utxo_pos_for(%{call_data: %{utxo_pos: utxo_pos_enc}} = _exit_info),
do: Utxo.Position.decode!(utxo_pos_enc)

@spec do_new(map(), list(map())) :: t()
@spec do_new(map(), list(keyword())) :: t()
defp do_new(contract_status, fields) do
fields = Keyword.put_new(fields, :is_active, parse_contract_exit_status(contract_status))
struct!(__MODULE__, fields)
Expand All @@ -109,7 +109,7 @@ defmodule OMG.Watcher.ExitProcessor.ExitInfo do
end

# NOTE: we have no migrations, so we handle data compatibility here (make_db_update/1 and from_db_kv/1), OMG-421
@spec make_db_update({Utxo.Position.t(), t()}) :: Utxo.Position.db_t()
@spec make_db_update({Utxo.Position.t(), t()}) :: {:put, :exit_info, {Utxo.Position.db_t(), map()}}
def make_db_update({position, exit_info}) do
value = %{
amount: exit_info.amount,
Expand All @@ -127,7 +127,7 @@ defmodule OMG.Watcher.ExitProcessor.ExitInfo do
{:put, :exit_info, {Utxo.Position.to_db_key(position), value}}
end

@spec from_db_kv({Utxo.Position.db_t(), t()}) :: Utxo.Position.t()
@spec from_db_kv({Utxo.Position.db_t(), map()}) :: {Utxo.Position.t(), t()}
def from_db_kv({db_utxo_pos, exit_info}) do
# mapping is used in case of changes in data structure
value = %{
Expand Down
42 changes: 42 additions & 0 deletions apps/omg_watcher/test/omg_watcher/api/account_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ defmodule OMG.Watcher.API.AccountTest do
use OMG.DB.Fixtures

alias OMG.TestHelper
alias OMG.Utxo
alias OMG.Watcher.API.Account

require Utxo

@eth OMG.Eth.zero_address()
@payment_output_type OMG.WireFormatTypes.output_type_for(:output_payment_v1)

Expand Down Expand Up @@ -63,5 +66,44 @@ defmodule OMG.Watcher.API.AccountTest do

assert %{blknum: ^blknum, txindex: ^txindex, oindex: ^oindex} = utxo
end

@tag fixtures: [:db_initialized]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will get rid of this fixture before making it ready to review, but review comments are welcomed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we're heading to ditch all fixtures, once they already there. Such fixtures are very useful and also keeps the codebase clean. That's my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I like them to. The problem we have is that if test uses a fixture defined in other app it can be run only in bulk, not as a single test. There might be a problem in how we designed tests. In this case removing fixture was easy, so I did it.

test "does not return exiting utxos" do
alice = TestHelper.generate_entity()

amount = 333
blknum = 1927
txindex = 78
oindex = 1
utxo_position = Utxo.position(blknum, txindex, oindex)

_ =
OMG.DB.multi_update([
{:put, :utxo,
{
{blknum, txindex, oindex},
%{
output: %{amount: amount, currency: @eth, owner: alice.addr, output_type: @payment_output_type},
creating_txhash: nil
}
}},
{:put, :exit_info,
{
Utxo.Position.to_db_key(utxo_position),
%{
amount: amount,
currency: @eth,
owner: alice.addr,
is_active: true,
exit_id: 1,
exiting_txbytes: <<0>>,
eth_height: 1,
root_chain_txhash: nil
}
}}
])

assert [] == Account.get_exitable_utxos(alice.addr)
end
end
end
29 changes: 29 additions & 0 deletions apps/omg_watcher/test/omg_watcher/exit_processor/core_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -263,4 +263,33 @@ defmodule OMG.Watcher.ExitProcessor.CoreTest do
assert Enum.sort(expected_inputs) == Enum.sort(request.ife_input_utxos_to_check)
end
end

describe "active_standard_exiting_utxos" do
test "returns a set of exiting utxo positions" do
utxo_pos_active = {active_blknum, active_txindex, active_txoutput} = {1000, 0, 0}

active_exit = %{
amount: 1,
block_timestamp: 1,
currency: <<1::160>>,
eth_height: 1,
exit_id: 1,
exiting_txbytes: "txbytes",
is_active: true,
owner: <<1::160>>,
root_chain_txhash: <<1::256>>,
scheduled_finalization_time: 2,
spending_txhash: nil
}

utxo_pos_inactive = {1000, 0, 1}
inactive_exit = Map.replace!(active_exit, :is_active, false)

db_exits = [{utxo_pos_active, active_exit}, {utxo_pos_inactive, inactive_exit}]

expected = MapSet.new([OMG.Utxo.position(active_blknum, active_txindex, active_txoutput)])

assert expected == Core.active_standard_exiting_utxos(db_exits)
end
end
end