rpc: add GetTickets command #957

alexlyp · 2017-10-04T16:21:02Z

This PR implements a new RPC command for GetTickets.

It uses the same request settings for start/end block as GetTransactions. It then streams GetTicketsResponses back that contain single TicketDetails information in order of the block the ticket was mined.

TicketDetails contains everything that should be needed to collect all the required information about a ticket for the upcoming 'My Tickets' page in decrediton.

Closes #833

jrick

Reviewed the documented changes to the API. Since there are some things that need to be changed and clarified I'm holding off on the code review until it's clear what the API needs to be doing.

jrick · 2017-10-10T14:51:14Z

rpc/documentation/api.md

+- `TicketDetails tickets`: A given ticket's details.
+
+  The `TicketDetails` message is used by other methods and is documented
+  [here](#ticketdetails).


This isn't true, it's a new message that's only used here, and can be documented inline.

jrick · 2017-10-10T14:52:35Z

rpc/documentation/api.md

+relevant to the wallet.  The message includes details such as which previous
+wallet inputs are spent by this transaction, whether each output is controlled
+by the wallet or not, the total fee (if calculable), and the earlist time the
+transaction was seen.


This description is a copy pasta of TransactionDetails' description and needs to be redone.

jrick · 2017-10-10T14:53:17Z

rpc/documentation/api.md

+
+- `bytes hash`: The hash of the given ticket.
+
+- `bytes spender_hash`: The hash of the spender (if applicable, otherwise empty).


Instead of empty, say it uses the default value.

jrick · 2017-10-10T14:55:04Z

rpc/documentation/api.md

+- `bytes spender_hash`: The hash of the spender (if applicable, otherwise empty).
+
+- `int64 ticket_age`: If unspent the blocks since being mined.  Otherwise,
+   the number of blocks from when it was mined to when it was spent.


This is confusing to me and I'm not sure how it would be useful, even more so since it will take on different values depending on if it was spent.

jrick · 2017-10-10T14:56:11Z

rpc/documentation/api.md

+
+- `int64 ticket_price`: The value of the first output from the ticket transaction.
+  Due to the way tickets are constructed, we know that this is always the 'price'
+  of any given ticket.


This is an implementation detail that we don't need to discuss here. We should just document that it is the ticket price. If a ticket was purchased with a price greater than required, reporting that value as the ticket price should be considered a bug and fixed.

jrick · 2017-10-10T14:56:56Z

rpc/documentation/api.md

+  of any given ticket.
+
+- `int64 ticket_cost`: The total cost of the wallet to purchase the ticket.  Typically,
+  this would just be the ticket fee.


Then why not call this field ticket_fee? What else would contribute to the "cost" that isn't the ticket price itself?

jrick · 2017-10-10T14:58:55Z

rpc/documentation/api.md

+
+- `int64 spender_return`: The total return from the spender transaction.  If a vote,
+  this number should be positive (PoS vote subsidy - poolfee).  If a revocation, 
+  this would be negative (minus txfee and poolfee). 


It's not clear if this is referring to the coins generated by the vote/revocation that this wallet controls, or the total (including fees for split ticket ownership and stakepool tickets).

jrick · 2017-10-10T14:59:30Z

rpc/documentation/api.md

+
+  - `UNMINED`: A ticket that has yet to be mined into a block.
+
+**Stability**: Unstable:


Can nuke the trailing colon if there's no explanation afterwards

jrick · 2017-10-10T15:00:42Z

rpc/documentation/api.md

+
+- `TicketStatus ticket_status`: The observed status of the given ticket.
+
+  **Nested enum:** `TicketStatus`


The ordering of these seems odd. I would start with unmined, then immature, then live, followed by the rest.

jrick · 2017-10-13T14:12:26Z

wallet/udb/txquery.go

@@ -8,6 +8,8 @@ package udb
 import (
 	"fmt"

+	"github.com/decred/dcrd/blockchain/stake"
+


extra newline

jrick · 2017-10-13T14:14:24Z

wallet/wallet.go

+	Tickets []*TicketSummary
+}
+
+// GetTickets implements the rpc request command for gettickets


This comment really needs to be more specific regarding its behavior and what all parameters do, and we shouldn't isolate this API as only for the RPC server.

jrick · 2017-10-13T14:18:20Z

wallet/wallet.go

+}
+
+// GetTickets implements the rpc request command for gettickets
+func (w *Wallet) GetTickets(startBlock, endBlock *BlockIdentifier, cancel <-chan struct{}) (*GetTicketsResult, error) {


I would change this function signature to something more like:

func (w *Wallet) GetTickets(ctx context.Context, startBlock, endBlock *BlockIdentifier, results chan<- *TicketSummary) error

A context should be used for cancellation and should be the first parameter. The channel is used to stream the results back to the caller rather than returning all of the results at once. This will allow the RPC server to actually stream the results to clients as it creates them, instead of loading all of the data up at once into a single slice and then sending messages to the client stream. (This is also a problem with the GetTransactions RPC and should be resolved in a separate commit)

jrick · 2017-10-13T14:19:23Z

wallet/notifications.go

+	// TicketStatusLive any currently live ticket.
+	TicketStatusLive TicketStatus = iota
+	// TicketStatusUnmined any not yet mined ticket.
+	TicketStatusUnmined


unmined and immature both come before the ticket being live, so those two should be values 0 and 1 respectively.

jrick · 2017-10-13T14:22:18Z

wallet/notifications.go

+			live, err := w.chainClient.ExistsLiveTicket(&details.Ticket.Hash)
+			if err != nil {
+				log.Errorf("Unable to check if ticket was live for ticket status: %v", &details.Ticket.Hash)
+				return nil


This behavior is not documented at all and will result in a runtime panic later if it occurs.

So you think it should just set the status to TicketUnknown, log the error and carry on?

jrick · 2017-10-13T14:26:05Z

wallet/notifications.go

+		txmgrNs := dbtx.ReadBucket(wtxmgrNamespaceKey)
+
+		_, tipHeight := w.TxStore.MainChainTip(txmgrNs)
+		ticketAge := int64(tipHeight - details.Ticket.Height())


rather than calculating the age this way to determine if the ticket is immature or expired, do a check on the number of confirmations. This is the way other code in the wallet does this check. There's also an off by one to be aware of. Grep the wallet package for other uses of TicketMaturity and TicketExpiry to find the correct check.

jrick · 2017-10-13T14:28:25Z

wallet/notifications.go

+}
+
+// TicketStatus describes the current status a ticket can be observed to be.
+type TicketStatus int8


we may need an "unknown" value as well. This will allow the ticket results to still be created even if not all info regarding its state can be determined. See the comment about returning nil and the panic above.

jrick · 2017-10-13T14:29:55Z

wallet/wallet.go

+		} else {
+			err := walletdb.View(w.db, func(dbtx walletdb.ReadTx) error {
+				ns := dbtx.ReadBucket(wtxmgrNamespaceKey)
+				meta, err := w.TxStore.GetBlockMetaForHash(ns, startBlock.hash)


Look up the header and extract the height from that. I want to remove BlockMeta eventually as it is old code that is made irrelevant now that all headers are saved.

jrick · 2017-10-13T14:31:18Z

wallet/wallet.go

+		} else {
+			err := walletdb.View(w.db, func(dbtx walletdb.ReadTx) error {
+				ns := dbtx.ReadBucket(wtxmgrNamespaceKey)
+				meta, err := w.TxStore.GetBlockMetaForHash(ns, endBlock.hash)


same as above

jrick · 2017-10-13T14:33:46Z

wallet/wallet.go

+				// XXX Here is where I would look up the ticket information from the db so I can populate spenderhash and ticket status
+				ticketInfo, err := w.TxStore.TicketDetails(txmgrNs, &details[i])
+				if err != nil {
+					log.Errorf("error while trying to get ticket details for ")


this should return the error instead of logging (and the log is missing the details about what was looked up too)

- merge with hx

Merge for Hx: - Remove context changes added in 90f42ca.

alexlyp added rpc-api WIP labels Oct 4, 2017

alexlyp changed the title ~~rpc: add GetTickets WIP~~ rpc: add GetTickets command Oct 4, 2017

alexlyp removed the WIP label Oct 4, 2017

alexlyp mentioned this pull request Oct 4, 2017

Add GetTickets basic implementation decred/decrediton#745

Merged

alexlyp force-pushed the ayp_gettickets branch from 7687853 to a4ac0b4 Compare October 5, 2017 15:41

jrick requested changes Oct 10, 2017

View reviewed changes

jrick requested changes Oct 13, 2017

View reviewed changes

alexlyp added 22 commits October 13, 2017 15:34

First pass adding gettickets request to rpc

8721349

Stub in various pieces

12fe8f7

A try at doing range transactions for tickets

8e0afb9

More

5f53514

Add a comment

572192c

More rigging

10e3fc2

More rigging to get the hash and spenderhash populated

37e114a

More rigging

f09fd19

Oops

8300a44

Add is ticket check

0104bfb

More fixes

0e093c1

Getting close

8c2aae1

more ticket summary stuff

fdb96cc

Update api.proto with new fields

7579bed

More ticketsummary

b6b825a

fix panic

150dba3

Add mockup missed check

2192d66

More updates

d549879

Add grpc doco

17e128b

ineff

0c3f242

Bump rpc version

d8cc2c4

update comment

c9d769e

alexlyp added 4 commits October 13, 2017 15:34

Couple fixes

c2ac437

Populate TicketPrice

eb5d9e6

Some updates from jrick review

de50a5d

Make changes as discussed with jrick

1692d1e

alexlyp force-pushed the ayp_gettickets branch from 105fd23 to 1692d1e Compare October 13, 2017 20:34

alexlyp added 9 commits October 19, 2017 12:10

Extra newline

132bf24

More issues resolved for jrick

289d8fc

more fixes

7fddce2

More changes

3c18c5a

Return ctx.Err()

0ca7bbf

Make requested changes to getTransactions too

3229010

More changes

9a61a38

Add TicketStatus Unknown

2abfab2

Update status order

d5faea5

jrick approved these changes Oct 19, 2017

View reviewed changes

alexlyp merged commit c51ca68 into decred:master Oct 19, 2017

alexlyp deleted the ayp_gettickets branch October 19, 2017 18:57

buckbeakk pushed a commit to hybridnetwork/hxwallet that referenced this pull request Feb 19, 2018

rpc: add GetTickets command (decred#957)

7935268

- merge with hx

buckbeakk pushed a commit to hybridnetwork/hxwallet that referenced this pull request Feb 20, 2018

rpc: add GetTickets command (decred#957)

713b28f

- merge with hx

buckbeakk pushed a commit to hybridnetwork/hxwallet that referenced this pull request Feb 25, 2018

rpc: add GetTickets command (decred#957)

ee9d7ac

Merge for Hx: - Remove context changes added in 90f42ca.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: add GetTickets command #957

rpc: add GetTickets command #957

alexlyp commented Oct 4, 2017 •

edited

Loading

jrick left a comment

jrick Oct 10, 2017

jrick Oct 10, 2017

jrick Oct 10, 2017

jrick Oct 10, 2017

jrick Oct 10, 2017

jrick Oct 10, 2017

jrick Oct 10, 2017

jrick Oct 10, 2017

jrick Oct 10, 2017

jrick Oct 13, 2017

jrick Oct 13, 2017

jrick Oct 13, 2017

jrick Oct 13, 2017

jrick Oct 13, 2017

alexlyp Oct 19, 2017

jrick Oct 19, 2017

jrick Oct 13, 2017

jrick Oct 13, 2017

jrick Oct 13, 2017

jrick Oct 13, 2017

jrick Oct 13, 2017


		- `bytes hash`: The hash of the given ticket.

		- `bytes spender_hash`: The hash of the spender (if applicable, otherwise empty).


		- `UNMINED`: A ticket that has yet to be mined into a block.

		Stability: Unstable:


		- `TicketStatus ticket_status`: The observed status of the given ticket.

		Nested enum: `TicketStatus`

rpc: add GetTickets command #957

rpc: add GetTickets command #957

Conversation

alexlyp commented Oct 4, 2017 • edited Loading

jrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexlyp commented Oct 4, 2017 •

edited

Loading