-
Notifications
You must be signed in to change notification settings - Fork 156
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
Dont allow ticket voting address override. #2417
Conversation
Setting a specific voting address was an important part of purchasing a legacy stakepool ticket, however it is no longer necessary and providing these options only serves as a potential footgun which can lead to accidental address reuse. Now a new voting address is derived for every ticket, either from the voting account if one is specified, or from the purchase account if not. Breaking changes: 1. Config `--votingaddress` removed. 1. Param `ticket_address` removed from `PurchaseTickets` gRPC. 1. Param `voting_address` removed from `RunTicketBuyer` gRPC. 1. Param `ticketaddress` removed from `purchaseticket` json-RPC.
It only wrapped one boolean value so doesn't serve any purpose when the boolean can be used directly.
return nil, errors.E(op, errors.Invalid, | ||
"ticket address must either be P2SH or P2PKH") | ||
} | ||
const stakeSubmissionPkScriptSize = txsizes.P2PKHPkScriptSize + 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.
Please double check this line.
My understanding is that in most/all current usage of this func, it is called with an empty req.VotingAddress
which means the second case from the switch should be the one which becomes hard-coded here.
} | ||
// If the user hasn't specified a voting account, derive a voting | ||
// address from the purchasing account. | ||
addrVote, _, err = stakeAddrFunc(op, req.SourceAccount, 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.
Do we need to call signingAddressAtIdx
here?
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 don't think so. It's only called once above, and the address return result is immediately discarded. It appears the only thing it might be doing is persisting the returned address to the database, but this should already be handled by stakeAddrFunc.
This function should probably just go away.
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.
Ah, no, stakeAddrFunc is being called with the internal branch, but signingAddressAtIndex hardcodes the external branch. Seems this is needed for VSP usecases.
Setting a specific voting address was an important part of purchasing a legacy stakepool ticket, however it is no longer necessary and providing these options only serves as a potential footgun which can lead to accidental address reuse.
Now a new voting address is derived for every ticket, either from the voting account if one is specified, or from the purchase account if not.
Breaking changes:
--votingaddress
removed.ticket_address
removed fromPurchaseTickets
gRPC.voting_address
removed fromRunTicketBuyer
gRPC.ticketaddress
removed frompurchaseticket
json-RPC.