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-3 Identify transaction types and define them in txs.hrl #30

Merged
merged 2 commits into from Aug 29, 2017

Conversation

zp-sd
Copy link
Contributor

@zp-sd zp-sd commented Aug 28, 2017

@sennui @wagerlabs This is the initial list of transactions (probably incomplete, but this may evolve once we implement). Thoughts?

Copy link
Contributor

@joelreymont joelreymont left a comment

Choose a reason for hiding this comment

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

I think that this indenting looks much better, specially if record names are long

record(oracle_new_tx, {
         id = 0          :: non_neg_integer(),
         account = <<>>  :: pubkey(),
         question = <<>> :: binary(),
         difficulty = 0  :: non_neg_integer(),
         start_date = 0  :: integer(),
         fee = 0         :: non_neg_integer(),
         nonce = 0       :: non_neg_integer()
        }).

compared to the original

record(oracle_new_tx, {id = 0          :: non_neg_integer(),
                       account = <<>>  :: pubkey(),
                       question = <<>> :: binary(),
                       difficulty = 0  :: non_neg_integer(),
                       start_date = 0  :: integer(),
                       fee = 0         :: non_neg_integer(),
                       nonce = 0       :: non_neg_integer()}).

@sennui What do you think?

@sennui
Copy link
Contributor

sennui commented Aug 29, 2017

@wagerlabs I recognise the orignal as emacs-otp indenting. If we can put rules around the first, I dont mind it (I would like just to make it more specific)

fee = 0 :: non_neg_integer(),
nonce = 0 :: non_neg_integer()}).

-record(channel_slash_tx, {id = 0 :: non_neg_integer(),
Copy link
Contributor

Choose a reason for hiding this comment

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

can we improve naming? slash is not saying anything? is anyone using it and this is why we use slash? Maybe: Overload?

Copy link
Contributor

Choose a reason for hiding this comment

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

The indent I prefer is how my Emacs is indenting if you leave the opening brace hanging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Slash is a cryptocurrency term so I would leave it. We'll evaluate the transaction itself when we get to channels.

fee = 0 :: non_neg_integer(),
nonce = 0 :: non_neg_integer()}).

-record(channel_solo_close_tx, {id = 0 :: non_neg_integer(),
Copy link
Contributor

Choose a reason for hiding this comment

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

self close?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't comment on the naming because some of the transactions may go eventually. I do think that having both close and solo close is superfluous as the system should be able to recognize that you are closing the channel unilaterally. Besides, it makes for less conflicts to resolve, e.g. when one side did a close and the other one a solo close.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well.. do we want to evaluate these transactions now, or leave it as it is, and massage when channels implementation takes place?

@zp-sd
Copy link
Contributor Author

zp-sd commented Aug 29, 2017

@wagerlabs Yes, agree with indentation comment. @sennui What is proposed is still emacs indenting.

@zp-sd
Copy link
Contributor Author

zp-sd commented Aug 29, 2017

@sennui @wagerlabs can we merge?

@joelreymont joelreymont merged commit 01a754e into master Aug 29, 2017
@dincho dincho deleted the gh3-identify-transaction-types branch February 27, 2018 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants