Skip to content

Commit

Permalink
refactor: endpoint to return boolean result
Browse files Browse the repository at this point in the history
  • Loading branch information
okalouti committed Aug 7, 2020
1 parent c3362a5 commit a4d023f
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 19 deletions.
6 changes: 3 additions & 3 deletions apps/omg_watcher/lib/omg_watcher/block_validator.ex
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ defmodule OMG.Watcher.BlockValidator do
- Verifies that transactions are correctly formed.
- Verifies that given Merkle root matches reconstructed Merkle root.
"""
@spec stateless_validate(Block.t()) :: {:ok, Block.t()} | {:error, atom()}
@spec stateless_validate(Block.t()) :: {:ok, boolean()} | {:error, atom()}
def stateless_validate(submitted_block) do
with {:ok, recovered_transactions} <- verify_transactions(submitted_block.transactions),
{:ok, block} <- verify_merkle_root(submitted_block, recovered_transactions) do
{:ok, block}
{:ok, _block} <- verify_merkle_root(submitted_block, recovered_transactions) do
{:ok, true}
end
end

Expand Down
4 changes: 2 additions & 2 deletions apps/omg_watcher/test/omg_watcher/block_validator_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ defmodule OMG.WatcherRPC.Web.Validator.BlockValidatorTest do
transactions: [signed_txbytes_1, signed_txbytes_2]
}

assert {:ok, block} == BlockValidator.stateless_validate(block)
assert {:ok, true} == BlockValidator.stateless_validate(block)
end

test "returns error for non-matching Merkle root hash" do
Expand Down Expand Up @@ -118,7 +118,7 @@ defmodule OMG.WatcherRPC.Web.Validator.BlockValidatorTest do
transactions: signed_txbytes
}

assert {:ok, block} = BlockValidator.stateless_validate(block)
assert {:ok, true} = BlockValidator.stateless_validate(block)
end
end
end
11 changes: 8 additions & 3 deletions apps/omg_watcher_rpc/lib/web/controllers/block.ex
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,14 @@ defmodule OMG.WatcherRPC.Web.Controller.Block do
Executes stateful and stateless validation of a block.
"""
def validate_block(conn, params) do
with {:ok, block} <- Validator.BlockConstraints.parse_to_validate(params),
{:ok, _block} <- Watcher.BlockValidator.stateless_validate(block) do
api_response(block, conn, :validate_block)
with {:ok, block} <- Validator.BlockConstraints.parse_to_validate(params) do
case Watcher.BlockValidator.stateless_validate(block) do
{:ok, true} ->
api_response(%{valid: true}, conn, :validate_block)

{:error, reason} ->
api_response(%{valid: false, error: reason}, conn, :validate_block)

This comment has been minimized.

Copy link
@InoMurko

InoMurko Aug 7, 2020

Contributor

what is the requirement for error: reason? I don't think clients needs that. We should remove if it's not a requirement.

This comment has been minimized.

Copy link
@unnawut

unnawut Aug 7, 2020

Contributor

hmm won't removing this make it super difficult to debug why it's retuning valid: false?

This comment has been minimized.

Copy link
@kalouo

kalouo Aug 7, 2020

@InoMurko brings up the security point that it hints to a potential attacker why a block is invalid.

This comment has been minimized.

Copy link
@kalouo

kalouo Aug 7, 2020

which makes sense to me, but ultimately would defer to the need of the client.

In this case the future client is the Vault - we can double check with @thec00n but since deploying the Vault is a few months' off we don't have full clarity on how the requirement will evolve. Either way, it's trivial to add in the error cause.

This comment has been minimized.

Copy link
@InoMurko

InoMurko Aug 7, 2020

Contributor

the purpose of the endpoint is not to debug blocks, it's to provide validation. the vague requirements allow us to do whats the most easiest and code friendly solution. which is true/false.

This comment has been minimized.

Copy link
@InoMurko

InoMurko Aug 7, 2020

Contributor

if you don't provide engineers with full requirements and specification you should deliver the least amount of effort solution. not the most, because we're all going to be maintain it. and we're not in that kind of business.

end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,8 @@ defmodule OMG.WatcherRPC.Web.Controller.BlockTest do
%{"data" => data} = WatcherHelper.rpc_call("block.validate", params, 200)

assert data == %{
"code" => "block.validate:invalid_merkle_root",
"description" => "Block hash does not match reconstructed Merkle root.",
"object" => "error"
"valid" => false,
"error" => "invalid_merkle_root"
}
end

Expand Down Expand Up @@ -245,9 +244,8 @@ defmodule OMG.WatcherRPC.Web.Controller.BlockTest do
%{"data" => data} = WatcherHelper.rpc_call("block.validate", params, 200)

assert data == %{
"code" => "validate_block:duplicate_inputs",
"description" => nil,
"object" => "error"
"error" => "duplicate_inputs",
"valid" => false
}
end

Expand Down Expand Up @@ -278,11 +276,7 @@ defmodule OMG.WatcherRPC.Web.Controller.BlockTest do

%{"data" => data} = WatcherHelper.rpc_call("block.validate", params, 200)

assert data == %{
"hash" => params.hash,
"number" => params.number,
"transactions" => params.transactions
}
assert data == %{"valid" => true}
end
end
end

0 comments on commit a4d023f

Please sign in to comment.