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

Fix Insufficient Balance to Pay Ticket Fee #189

Merged
merged 8 commits into from Jun 18, 2021

Conversation

oshorefueled
Copy link
Contributor

This PR resolves issue #176

It adds some of the methods used in dcrwallet vsp package to reserve some inputs for paying for fees after puchasing a ticket. It adds a vsp package which consists of some helper functions and client code for making requests to the vsp server

@oshorefueled oshorefueled requested a review from beansgum May 2, 2021 15:23
@oshorefueled oshorefueled force-pushed the ticket-fee branch 2 times, most recently from e317ae3 to cc06ea6 Compare May 2, 2021 17:42
vsp.go Outdated
c *w.ConfirmationNotificationsClient

ticketToFeeLock sync.Mutex
feesToTickets map[chainhash.Hash]chainhash.Hash
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is unused, can be removed.

vsp.go Outdated
Comment on lines 14 to 18
"github.com/planetdecred/dcrlibwallet/vsp"

"decred.org/dcrwallet/wallet/txrules"

w "decred.org/dcrwallet/wallet"
"decred.org/dcrwallet/wallet/txsizes"
Copy link
Contributor

Choose a reason for hiding this comment

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

Group imports

vsp.go Outdated
func (r *lockedRand) coinflip() bool {
r.mu.Lock()
defer r.mu.Unlock()
return r.rand.Uint32n(2) == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets remove the added prng.go file and use the random generator through wallet/txauthor/cprng.go to reduce added files

https://github.com/decred/dcrwallet/blob/master/wallet/txauthor/cprng.go#L32

Here's how it's used to randomize output position
https://github.com/decred/dcrwallet/blob/master/wallet/txauthor/author.go#L169

types.go Outdated
Timestamp int64 `json:"timestamp"`
Request json.RawMessage `json:"request"`
Timestamp int64 `json:"timestamp"`
Request []byte `json:"request"`
}

type TicketStatusRequest struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused structs.

  • TicketStatusRequest
  • TicketStatusResponse
  • SetVoteChoicesRequest
  • SetVoteChoicesResponse
  • GetFeeAddressRequest
  • GetFeeAddressResponse

vsp.go Outdated

*vsp.Client
ctx context.Context
c *w.ConfirmationNotificationsClient
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is not used, remove this & other unused variables in this struct.

vsp.go Outdated
Comment on lines 103 to 104
passphraseCopy := make([]byte, len(passphrase))
_ = copy(passphraseCopy, passphrase)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this done?

vsp.go Outdated

// PurchaseStakeTickets purchases the number of tickets passed as an argument and pays the fees for the tickets
func (v *VSP) PurchaseTickets(ticketCount, expiryBlocks int32, passphrase []byte) error {
defer v.w.LockWallet()
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to after successfully unlocking the wallet.

vsp.go Outdated

_, err = v.w.internal.PurchaseTickets(v.ctx, networkBackend, request)
if err != nil {
log.Errorf("PURCHASE TICKET %v", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

This log is not needed since the error is returned, to avoid spamming the logs.

vsp.go Outdated

// verify initial request matches server
if !bytes.Equal(requestBody, payfeeResponse.Request) {
log.Warnf("[PAYFee] server response has differing request: %#v != %#v",
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove spam logs...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this should be considered spam log. There are a number of places where requests are made to the vsp server and they return this error. Logging it with the differences would make it easier to debug and fix. The error message returned "server response contains differing request" is a user friendly message that could easily be used by the caller of the method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but the log is not readable and not necessarily needed unless in development. If the content of the request is needed then it can be added to the returned error.

vsp.go Outdated

// verify initial request matches server
if !bytes.Equal(requestBody, payfeeResponse.Request) {
log.Warnf("[PAYFee] server response has differing request: %#v != %#v",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but the log is not readable and not necessarily needed unless in development. If the content of the request is needed then it can be added to the returned error.

vsp.go Outdated
Comment on lines 242 to 279
log.Warnf("failed to generate pay to addr script for %v: %v", feeInfo.FeeAddress, err)
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.Warnf("failed to generate pay to addr script for %v: %v", feeInfo.FeeAddress, err)
return err
return errors.Errorf("failed to generate pay to addr script for %v: %v", feeInfo.FeeAddress, err)

Also, make this change in other similar places.

methods required to pay tickets fees to a VSPD are made private and called
from a single exported method.

wallet unlocking is removed from createTicketFeeTx and payVSPFee methods
and called only once before the methods are called.
- the issue with insufficient balance when trying to pay fees for tickets has been
  fixed on the ticketbuyer feature on dcrwallet. Fee payment methods used by
  dcrwallet are replicated and integrated into dcrlibwallet
- update fee processing method to requirements defined on dcrwallet 1.6.0
- remove unused struct field and type declarations
- Replace prng code with exising output randomization method from
  the txauthor package.
- remove spam logs from code
@beansgum beansgum merged commit 9bf46bd into planetdecred:master Jun 18, 2021
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.

None yet

2 participants