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

Add fee processment status and process it later #1866

Merged
merged 11 commits into from
Oct 19, 2020

Conversation

vctt94
Copy link
Member

@vctt94 vctt94 commented Oct 7, 2020

This PR aims to store the fee tx processment status. This is needed because we need to know when it fails to process, so we can retry it, in the future.

Right now if a vsp fail to process a vsp ticket, dcrawllet will continue the execution and will never process it again.

This PR creates a new field on the VSPTicket struct: FeeTxStatus, and store when the vsp ticket fee payment process has failed.

I am adding checks for when dcrwallet has it initial load and add two new grpc requests so I can deal with it on decrediton:
SyncVSPFailedTickets and FailedVSPTicketsProcess.

FailedVSPTicketsProcess gets all vsp tickets which errored. It has an empty request, which can be used for knowing if decrediton has unregistered vsp tickets.

SyncVSPFailedTickets Will sync them and because of that it needs a vsphost and vsppubkey into the request.

@vctt94 vctt94 force-pushed the vsp-fee-sttus branch 2 times, most recently from 27ceeb6 to 6509db9 Compare October 9, 2020 20:43
@vctt94 vctt94 changed the title [wip] udb/vsp: Add fee processment status udb/vsp: Add fee processment status Oct 9, 2020
@vctt94 vctt94 changed the title udb/vsp: Add fee processment status Add fee processment status and process it later Oct 9, 2020
@vctt94 vctt94 force-pushed the vsp-fee-sttus branch 3 times, most recently from 69c44da to f3f1e2d Compare October 15, 2020 14:25
dcrwallet.go Outdated
@@ -242,6 +243,17 @@ func run(ctx context.Context) error {
passphrase = startPromptPass(ctx, w)
}

// Process VSP tickets that failed to be processed.
Copy link
Member

Choose a reason for hiding this comment

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

I feel iffy about putting all of this into the application startup code. At least for decrediton's purposes, it would be fine to only report status and react to it over RPC.

@vctt94 vctt94 force-pushed the vsp-fee-sttus branch 5 times, most recently from 9c515d6 to 95fe39d Compare October 19, 2020 15:18
rpc/api.proto Outdated
message SyncVSPTicketsResponse {}

message GetVSPTicketsByFeeStatusRequest {
int32 fee_status = 1;
Copy link
Member

Choose a reason for hiding this comment

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

protobuf has enums, use that for these states, instead of plain integers.

const (
// VSP_FEE_PROCESS_STARTED represents the state which process has being
// called but fee still not paid.
VSP_FEE_PROCESS_STARTED = 0
Copy link
Member

Choose a reason for hiding this comment

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

SCREAMING_CAPS is not go style, UseNamesLikeThis.

These constants should also use a defined type, and the constants should be declared using iota.

wallet/udb/vsp.go Show resolved Hide resolved
wallet/wallet.go Outdated
// informed fee status.
func (w *Wallet) GetVSPTicketsByFeeStatus(ctx context.Context, feeStatus int) ([]chainhash.Hash, error) {
const op errors.Op = "wallet.GetVSPTicketsByFeeStatus"
tickets := make(map[chainhash.Hash]*udb.VSPTicket)
Copy link
Member

Choose a reason for hiding this comment

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

don't make if you are going to overwrite this anyways. just declare the variable.

wallet/wallet.go Outdated
// UpdateVSPTicket updates the vsp ticket for the informed tickethash.
func (w *Wallet) UpdateVSPTicket(ctx context.Context, ticketHash *chainhash.Hash, vspTicket udb.VSPTicket) error {
var err error
w.lockedOutpointMu.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

this mutex acquire is not needed here.

wallet/wallet.go Outdated
// SetPublished sets the informed hash as true or false.
func (w *Wallet) SetPublished(ctx context.Context, hash *chainhash.Hash, published bool) error {
var err error
w.lockedOutpointMu.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

nor here

wallet/wallet.go Outdated

func (w *Wallet) UpdateVspTicketFeeToPaid(ctx context.Context, ticketHash *chainhash.Hash, feeHash *chainhash.Hash) error {
var err error
w.lockedOutpointMu.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

nor here

wallet/wallet.go Show resolved Hide resolved
@jrick jrick force-pushed the vsp-fee-sttus branch 2 times, most recently from 2ae6193 to ab3ac3f Compare October 19, 2020 18:01
return nil, err
}
}
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.

this should return &pb.SyncVSPTicketsResponse{}, not nil

Comment on lines 19 to 25
VSPFeeProccesStarted = 0
// VSPFeeProcessPaid represents the state where the process has being
// paid, but not published.
VSPFeeProcessPaid = 1
// TODO change fee status to published, once fee tx is published.
VSPFeeProcessPublished = 2
VSPFeeProcessErrored = 3
Copy link
Member

@jrick jrick Oct 19, 2020

Choose a reason for hiding this comment

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

Use iota and a type

type VSPFeeStatus int

const (
	// VSPFeeProccesStarted represents the state which process has being
	// called but fee still not paid.
	VSPFeeProccesStarted VSPFeeStatus = iota

	// VSPFeeProcessPaid represents the state where the process has being
	// paid, but not published.
	VSPFeeProcessPaid

	// TODO change fee status to published, once fee tx is published.
	VSPFeeProcessPublished

	VSPFeeProcessErrored

also please take care of that TODO, remove it if that's fixed.

const (
// VSPFeeProccesStarted represents the state which process has being
// called but fee still not paid.
VSPFeeProccesStarted = 0
Copy link
Member

Choose a reason for hiding this comment

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

This is a typo, should be VSPFeeProcessStarted

rpc/api.proto Show resolved Hide resolved
rpc/api.proto Outdated
VSP_FEE_PROCESS_PAID = 1;
VSP_FEE_PROCESS_ERRORED = 2;
}
FeeStatus feeStatus = 3;
Copy link
Member

Choose a reason for hiding this comment

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

fee_status

Comment on lines 1459 to 1461
if err != nil {
return nil, errors.E(op, err)
}
Copy link
Member

Choose a reason for hiding this comment

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

i might have introduced this when i rebased, but it should be removed.

@jrick jrick merged commit fdc4bf4 into decred:master Oct 19, 2020
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