-
Notifications
You must be signed in to change notification settings - Fork 91
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
Conversation
cdfa4ea
to
ea5b703
Compare
I guess the rpcwallet also needs to be a |
a6713e3
to
b7484aa
Compare
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 | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
client/asset/dcr/rpcwallet.go
Outdated
w.ticketCache.Lock() | ||
defer w.ticketCache.Unlock() | ||
if w.ticketCache.stamp.Add(ticketCacheExpiry).After(time.Now()) { | ||
return w.ticketCache.tickets, nil | ||
} |
There was a problem hiding this comment.
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.
This looks ok for purchasing tickets but viewing them afterwards seems to have a lot of problems still. |
client/asset/dcr/rpcwallet.go
Outdated
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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use time.Since
if errors.Is(err, oldSPVWalletErr) { | ||
return nil, nil | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this 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?
if errors.Is(err, oldSPVWalletErr) { | ||
return nil, nil | ||
} |
There was a problem hiding this comment.
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.
client/asset/dcr/rpcwallet.go
Outdated
// Calling w.rpcClient.GetTickets too close together will return no | ||
// tickets for the second call. A ticket cache alleviates this. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure...
There was a problem hiding this comment.
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.
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 | ||
} |
There was a problem hiding this comment.
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.
b7484aa
to
ed4c676
Compare
Configuring beta wallet with the vsp https://github.com/decred/dcrdex/compare/b7484aa8719d11b1c97285d7410a58c035b7971a..ed4c676d50d7467941a233d26a7ba9664d3700f4 |
ed4c676
to
916f249
Compare
Looking again today I'm not seeing blank results. Removing the cache for now... |
65739da
to
a8d7dbf
Compare
@buck54321 updated the dcrwallet version to latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments illustrated here.
a8d7dbf
to
ced0579
Compare
Just rebased. |
ced0579
to
e4a9b00
Compare
Removed the interface methods and updated vspd version in the harness https://github.com/decred/dcrdex/compare/ced0579ac849baf5dd086af4df2ca36d796ea7a3..e4a9b00143d071d040ac282750e4031696aeabc4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just needs a rebase.
e4a9b00
to
cbba655
Compare
cbba655
to
720df64
Compare
Rebased and updated to 4.1.1 |
closes #2611
Testers must update their system's
vspd
to latest or it will panic in the harness.