-
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
multi: gui staking #2482
multi: gui staking #2482
Conversation
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.
Working well, just noticed some issues. No matter how many tickets I try to buy, it always only buys one ticket. After I buy a ticket, and the block has not yet been mined, if I try to buy another one it will say "insufficient funds", but the wallet page shows I have plenty of funds.
client/asset/dcr/spv.go
Outdated
endBlock = wallet.NewBlockIdentifierFromHeight(upperHeight) | ||
} | ||
if lowerHeight == lowerHeightAutomatic { | ||
bn := upperHeight - int32(p.TicketExpiry+uint32(p.TicketMaturity)-requiredConfs) |
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.
What's the reason for subtracting requiredConfs
and why is it 6 + 2
?
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 see this in dcrwallet, and I probably copied it from there, but unsure what it is about. Unless it wants the block when the ticket purchasing started? If the mined block it should be revoked after p.TicketExpiry+p.TicketMaturity?
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 used in ForUnspentUnexpiredTickets
without comment, but yes, I agree it seems unneeded. I guess I'll just remove it.
I've got a
I can try to loop and see if that works. |
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.
Looks great! It looks like the simnet vspd cannot handle the vote ids. Should we use a newer vspd version or is that not the issue?
Will try this out on testnet.
client/asset/dcr/spv.go
Outdated
// DRAFT NOTE: Why not 0? We count zero-conf as available, so this | ||
// doesn't match. | ||
MinConf: 1, |
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 0 is fine.
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.
Changed to zero. For SPV wallets, new outputs aren't even seen until they have 1 confirmation, so any zero confirmation outputs seen by an spv wallet would be our own e.g. change outputs.
client/asset/dcr/spv.go
Outdated
endBlock = wallet.NewBlockIdentifierFromHeight(upperHeight) | ||
} | ||
if lowerHeight == lowerHeightAutomatic { | ||
bn := upperHeight - int32(p.TicketExpiry+uint32(p.TicketMaturity)-requiredConfs) |
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 see this in dcrwallet, and I probably copied it from there, but unsure what it is about. Unless it wants the block when the ticket purchasing started? If the mined block it should be revoked after p.TicketExpiry+p.TicketMaturity?
Seems it's not currently possible / allowed to change the VSP? |
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.
Working on testnet! But as Wisdom said cannot change the vsp in the UI after choosing one.
It also seems it will show the ticket price as it syncs, so will have wrong info that could be way off during initial sync. Maybe should disable purchasing if not synced anyway? |
What are you seeing, @JoeGruffins? I'm on vspd version |
Fixed in 35c2516 |
Yes. Will leave this for followup. |
I think #2492 will be a big help for us with remaining hurdles for staking, and with other new wallet features. I don't plan on waiting for that here. Once this PR is merged, we can start addressing remaining issues with separate, targeted efforts. |
oh. doh. It's my installed version is the problem. Let me update and try again. |
|
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.
My vspd install was the problem. Looks great.
Enables the following staking related features through the client GUI
Implementation is minimal. My goal is to get a basic framework for staking on the front end. We can get all the bells and whistles added later.
There are still some unresolved questions around staking in general, marked with
DRAFT NOTE:
, though I am not opening this PR in draft because it's ready for review.Tickets statuses are not updated on the front end. The front end has no tip change notification. If there was a tip change notification and we requested a new
TicketStakingStatus
every block, merging that info with the existing front-end state will require some finesse. e.g. if a user is looking at the paginated ticket table, we can't just delete the current history underneath of them, as it would be inefficient and it would cause errors when the user tried to navigate. Leaving this as a project for later.dcrwallet's
PurchaseTickets
sometimes reduces the number of tickets purchased if there some utxo math it doesn't like. This is obviously terrible for UX, so a solution is needed. See draft notes.