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

[GH-757] Add delegates to Channel.Worker #809

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[GH-757] Add delegates to Channel.Worker #809

wants to merge 8 commits into from

Conversation

cytadela8
Copy link
Contributor

@cytadela8 cytadela8 commented Nov 29, 2018

  • Added delegates functionality to Channel Worker.
  • Adjusted transaction checks
  • Added tests for Delegates

Resolves #757

@cytadela8 cytadela8 self-assigned this Nov 29, 2018
Copy link
Collaborator

@thepiwo thepiwo left a comment

Choose a reason for hiding this comment

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

looks good to me, didn't check in detail

@thepiwo thepiwo requested a review from d-velev November 30, 2018 10:07
@@ -779,6 +818,10 @@ defmodule Aecore.Channel.ChannelStatePeer do
Retrieves our offchain account balance from the latest offchain chainstate
"""
@spec our_offchain_account_balance(ChannelStatePeer.t()) :: non_neg_integer()
def our_offchain_account_balance(%ChannelStatePeer{role: :delegate}) do
throw({:error, "#{__MODULE__}: Delagate doesn't have a balance"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the throws handled anywhere and can they be replaced with regular {:error, _} returns?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should only throw due to programmer error. It doesn't make sense to take a balance of delegate. It makes even less sens to get foreign_offchain_account_balance when ChannelStatePeer is for delegate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use this functions only in places where it's know the ChannelStatePeer.role is not delegate.

Copy link
Contributor

@gorbak25 gorbak25 left a comment

Choose a reason for hiding this comment

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

Good code, some small typos.

def receive_half_signed_tx(_, _, _, _opts \\ %{})

def receive_half_signed_tx(%ChannelStatePeer{role: :delegate}, _, _, _) do
{:error, "#{__MODULE__}: Deleagte can't handle half_signed_txs"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Deleagte -> Delegate

def receive_half_signed_tx(
%ChannelStatePeer{fsm_state: :open} = peer_state,
tx,
privkey,
opts \\ %{}
opts
Copy link
Contributor

Choose a reason for hiding this comment

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

By default no additional options should be passed for update validation.

@@ -927,6 +978,10 @@ defmodule Aecore.Channel.ChannelStatePeer do
{non_neg_integer(), non_neg_integer()},
Keys.sign_priv_key()
) :: {:ok, ChannelStatePeer.t(), SignedTx.t()} | error()
def receive_close_tx(%ChannelStatePeer{role: :delegate}, _, _, _, _) do
{:error, "#{__MODULE__}: Deleagte can't receive close tx"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Deleagte -> Delegate

@@ -1002,6 +1057,10 @@ defmodule Aecore.Channel.ChannelStatePeer do
non_neg_integer(),
Keys.sign_priv_key()
) :: {:ok, ChannelStatePeer.t(), SignedTx.t()} | error()
def solo_close(%ChannelStatePeer{role: :delegate}, _, _, _) do
{:error, "#{__MODULE__}: Delagate can't solo close"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Delagate -> Delegate

@@ -1060,7 +1125,7 @@ defmodule Aecore.Channel.ChannelStatePeer do
poi: dispute_poi_for_latest_state(peer_state),
offchain_tx: ChannelTransaction.dispute_payload(most_recent_tx)
},
our_pubkey(peer_state),
pubkey,
Copy link
Contributor

Choose a reason for hiding this comment

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

If the role is :initiator or :responder we should check that pubkey == our_pubkey(peer_state)

@@ -1090,6 +1156,7 @@ defmodule Aecore.Channel.ChannelStatePeer do
},
fee,
nonce,
pubkey,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@@ -1180,6 +1251,10 @@ defmodule Aecore.Channel.ChannelStatePeer do
"""
@spec settle(ChannelStatePeer.t(), non_neg_integer(), non_neg_integer(), Keys.sign_priv_key()) ::
{:ok, SignedTx.t()} | error()
def settle(%ChannelStatePeer{role: :delegate}, _, _, _) do
{:error, "#{__MODULE__}: Deleagte can't settle"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Deleagte -> Delegate

@gorbak25 gorbak25 changed the title [GH-757] Add delagates to Channel.Worker [GH-757] Add delegates to Channel.Worker Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants