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

dcrjson: add listtickets command #1267

Merged
merged 1 commit into from
Oct 11, 2018

Conversation

kLkA
Copy link
Contributor

@kLkA kLkA commented Jun 6, 2018

@kLkA kLkA force-pushed the add_listalltickets_rpc_method branch from bf692ed to 4327d49 Compare June 6, 2018 21:32
type ListTicketsCmd struct {
}

// NewListAccountsCmd returns a new instance which can be used to issue a
Copy link
Member

Choose a reason for hiding this comment

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

// NewListTicketsCmd ...

Hash string `json:"hash"`
Transaction string `json:"transaction"`
MyInputs []ListTicketsTransactionSummaryInput `json:"my_inputs"`
MyOutputs []ListTicketsTransactionSummaryOutput `json:"my_outputs"`
Copy link
Member

Choose a reason for hiding this comment

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

I think the "My" prefix is confusing. These would be clear as just Inputs and Outputs

Copy link
Contributor Author

@kLkA kLkA Jun 7, 2018

Choose a reason for hiding this comment

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

This was initially here https://github.com/decred/dcrwallet/blob/master/wallet/notifications.go#L465
@chappjc @jrick do I also have to remove it in txsummary to keep code consistent?

Copy link
Member

Choose a reason for hiding this comment

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

@chappjc the "My" names just mean they are inputs or outputs relevant to the wallet, it doesn't necessarily include all inputs and outputs from the transaction. If you are familiar with what wallet calls "credits" and "debits" internally, these are the same thing.

@kLkA please remove the underscores in the json keys, none of the other messages use them.

Copy link
Member

Choose a reason for hiding this comment

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

gotcha

@kLkA kLkA force-pushed the add_listalltickets_rpc_method branch 3 times, most recently from 0a85512 to e2b0171 Compare June 7, 2018 20:33
dcrjson/walletsvrresults.go Outdated Show resolved Hide resolved
@davecgh davecgh added this to the 1.3.0 milestone Jun 12, 2018
@davecgh davecgh modified the milestones: 1.3.0, 1.4.0 Jul 27, 2018
@kLkA kLkA force-pushed the add_listalltickets_rpc_method branch from e2b0171 to 88ca6b1 Compare September 22, 2018 12:05
@kLkA kLkA force-pushed the add_listalltickets_rpc_method branch 2 times, most recently from 7a94973 to 007f218 Compare October 6, 2018 09:53
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

I was going to approve this, as the code is correct, but decided against it, because if we don't stick with the established style standards, the code base is going to become increasingly inconsistent over time and thus deteriorate

Please define the types before they are used to be consistent with the vast majority of the dcrd code. There are certainly a few exceptions that have slipped through, but it is not the norm.

e.g.

type ListTicketsTransactionSummaryInput struct {
...
}
type ListTicketsTransactionSummaryOutput struct {
...
}
type ListTicketsTransactionSummary struct {
...
}
type ListTicketsResult struct {
...
}

current wallet with ticket, spender transactions' info and status
@kLkA kLkA force-pushed the add_listalltickets_rpc_method branch from 007f218 to 06b18cd Compare October 11, 2018 19:04
@davecgh davecgh merged commit fdac5fe into decred:master Oct 11, 2018
JFixby pushed a commit to JFixby/dcrd that referenced this pull request Oct 21, 2018
…ding_updates

txscript: Cleanup strict signature enforcement.
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.

5 participants