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

dcr: Allow ticket purchasing for rpc spv wallets #2756

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

Conversation

JoeGruffins
Copy link
Member

closes #2611

@JoeGruffins
Copy link
Member Author

I guess the rpcwallet also needs to be a ticketPager

@JoeGruffins JoeGruffins force-pushed the suppressstakestatus branch 5 times, most recently from a6713e3 to b7484aa Compare May 7, 2024 08:51
Comment on lines +1273 to +1257
var _ ticketPager = (*rpcWallet)(nil)

func (w *rpcWallet) TicketPage(ctx context.Context, scanStart int32, n, skipN int) ([]*asset.Ticket, error) {
return make([]*asset.Ticket, 0), nil
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this there will be an error:

2024-05-07 08:50:36.970 [ERR] WEB: error retrieving ticket page for 42: ticket pagination not supported for this wallet

But it doesn't look like the pager is needed. Should this error be silence somewhere else?

Also the statuses are all unknown, which I think is a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Without TicketPage will it show the tickets that have already voted?

Also the statuses are all unknown, which I think is a separate issue.

The native wallet doesn't show undefined.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we should consider using the tx history db for paginating ticket history. At least for spv wallet but maybe for all. No need to do it in this PR though. Sounds like a bit of work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Without TicketPage will it show the tickets that have already voted?

The already voted tickets always seem to go away. Without being a TicketPage you can't view any pages with under 10 tickets.

Comment on lines 1066 to 1070
w.ticketCache.Lock()
defer w.ticketCache.Unlock()
if w.ticketCache.stamp.Add(ticketCacheExpiry).After(time.Now()) {
return w.ticketCache.tickets, nil
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes this was returning tickets then none if called close together. I guess a cache is alright to have?

Maybe the cache can be made more intelligent to get statuses more correct.

@JoeGruffins JoeGruffins marked this pull request as ready for review May 7, 2024 08:57
@JoeGruffins
Copy link
Member Author

This looks ok for purchasing tickets but viewing them afterwards seems to have a lot of problems still.

func (w *rpcWallet) tickets(ctx context.Context, includeImmature bool) ([]*asset.Ticket, error) {
w.ticketCache.Lock()
defer w.ticketCache.Unlock()
if w.ticketCache.stamp.Add(ticketCacheExpiry).After(time.Now()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Can use time.Since

Comment on lines +5306 to +5315
if errors.Is(err, oldSPVWalletErr) {
return nil, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case the UI doesn't show any information that the current version of dcrwallet is too low. Might be helpful to show that.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reasonably easy way to do that? I don't expect the number of rpc+spv users will be high. If it's more that a few lines of work to properly communicate this message to the front end, I'm okay with how you have it, @JoeGruffins.

Comment on lines +1273 to +1257
var _ ticketPager = (*rpcWallet)(nil)

func (w *rpcWallet) TicketPage(ctx context.Context, scanStart int32, n, skipN int) ([]*asset.Ticket, error) {
return make([]*asset.Ticket, 0), nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Without TicketPage will it show the tickets that have already voted?

Also the statuses are all unknown, which I think is a separate issue.

The native wallet doesn't show undefined.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Can you configure the vsp for our harness beta wallet?

Comment on lines +5306 to +5315
if errors.Is(err, oldSPVWalletErr) {
return nil, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reasonably easy way to do that? I don't expect the number of rpc+spv users will be high. If it's more that a few lines of work to properly communicate this message to the front end, I'm okay with how you have it, @JoeGruffins.

Comment on lines 90 to 91
// Calling w.rpcClient.GetTickets too close together will return no
// tickets for the second call. A ticket cache alleviates this.
Copy link
Member

Choose a reason for hiding this comment

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

It's a dcrwallet bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unsure...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I was wrong about this. Removing for now.

Comment on lines +1273 to +1257
var _ ticketPager = (*rpcWallet)(nil)

func (w *rpcWallet) TicketPage(ctx context.Context, scanStart int32, n, skipN int) ([]*asset.Ticket, error) {
return make([]*asset.Ticket, 0), nil
}
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should consider using the tx history db for paginating ticket history. At least for spv wallet but maybe for all. No need to do it in this PR though. Sounds like a bit of work.

@JoeGruffins
Copy link
Member Author

@JoeGruffins
Copy link
Member Author

Looking again today I'm not seeing blank results. Removing the cache for now...

https://github.com/decred/dcrdex/compare/ed4c676d50d7467941a233d26a7ba9664d3700f4..916f2490e56b83ac40244c18957bf776fd88cf30

@JoeGruffins
Copy link
Member Author

@buck54321 updated the dcrwallet version to latest

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.

dcr: Allow ticket purchasing for rpc spv wallets.
3 participants