-
Notifications
You must be signed in to change notification settings - Fork 115
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
rpc: add SetPendingTransferProofCourierAddr endpoint to fix erroneous courier addresses #1097
base: main
Are you sure you want to change the base?
Changes from all commits
313bd2c
1df76fe
7520292
c4ff3c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,7 +107,12 @@ const ( | |
proofTypeReceive = tapdevrpc.ProofTransferType_PROOF_TRANSFER_TYPE_RECEIVE | ||
) | ||
|
||
// nolint: lll | ||
type ( | ||
// Shorthand gRPC types. | ||
updateCourierAddrReq = taprpc.SetPendingTransferProofCourierAddrRequest | ||
updateCourierAddrResp = taprpc.SetPendingTransferProofCourierAddrResponse | ||
|
||
// cacheableTimestamp is a wrapper around a uint32 that can be used as a | ||
// value in an LRU cache. | ||
cacheableTimestamp uint32 | ||
|
@@ -1357,6 +1362,114 @@ func (r *rpcServer) ListTransfers(ctx context.Context, | |
return resp, nil | ||
} | ||
|
||
// SetPendingTransferProofCourierAddr sets the transfer output proof courier | ||
// address for a pending (undelivered proof) transfer. The anchoring transaction | ||
// of the target transfer must have already been confirmed on-chain. | ||
func (r *rpcServer) SetPendingTransferProofCourierAddr(ctx context.Context, | ||
req *updateCourierAddrReq) (*updateCourierAddrResp, error) { | ||
|
||
// Unmarshal the anchor tx hash. | ||
if len(req.AnchorTxid) == 0 { | ||
return nil, fmt.Errorf("anchor txid must be set") | ||
} | ||
|
||
anchorTxHash, err := chainhash.NewHashFromStr(req.AnchorTxid) | ||
if err != nil { | ||
return nil, fmt.Errorf("invalid anchor tx hash: %w", err) | ||
} | ||
|
||
// Validate the requested new proof courier address. | ||
_, err = proof.ParseCourierAddress(req.NewProofCourierAddr) | ||
if err != nil { | ||
return nil, fmt.Errorf("invalid proof courier address: %w", err) | ||
} | ||
|
||
// Unmarshal the transfer output script key public key. | ||
outputScriptKey, err := parseUserKey(req.TransferOutputScriptPubKey) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the pubkey is a safety net to make sure the user really specified the right one? |
||
if err != nil { | ||
return nil, fmt.Errorf("invalid script pub key: %w", err) | ||
} | ||
|
||
// Query for pending transfers only. | ||
pendingParcels, err := r.cfg.AssetStore.QueryParcels( | ||
ctx, anchorTxHash, true, | ||
) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to query parcels: %w", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The user will run into this if they specify a completed transfer. Perhaps we should mention this in the error message, so something like "parcel not found in pending parcels"? |
||
} | ||
|
||
// Identify the target parcel and modify the proof courier address. | ||
var targetParcel *tapfreighter.OutboundParcel | ||
for idxParcel := range pendingParcels { | ||
pendingParcel := pendingParcels[idxParcel] | ||
|
||
// The anchoring transaction of target parcel must have already | ||
// been confirmed on-chain. | ||
if pendingParcel.AnchorTxBlockHash.IsNone() { | ||
continue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this matter? Maybe we want to update before we reached a confirmation? |
||
} | ||
|
||
// Investigate every output in the parcel. | ||
for idxOut := range pendingParcel.Outputs { | ||
output := pendingParcel.Outputs[idxOut] | ||
|
||
// Skip outputs that don't match the target output. | ||
if !output.ScriptKey.PubKey.IsEqual(outputScriptKey) || | ||
output.Position != req.TransferOutputPosition { | ||
|
||
continue | ||
} | ||
|
||
// At this point we've matched on the correct transfer | ||
// output. We will now perform a sanity check to ensure | ||
// check whether a proof should be delivered for this | ||
// output. | ||
shouldDeliverProof, err := output.ShouldDeliverProof() | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to determine "+ | ||
"if proof should be delivered for "+ | ||
"the given transfer output: %w", err) | ||
} | ||
|
||
if !shouldDeliverProof { | ||
return nil, fmt.Errorf("target transfer " + | ||
"output has been identified in a " + | ||
"pending transfer but a proof should " + | ||
"not be delivered for this output") | ||
} | ||
|
||
// Modify the proof courier address of the transfer | ||
// output. | ||
pendingParcel.Outputs[idxOut].ProofCourierAddr = | ||
[]byte(req.NewProofCourierAddr) | ||
|
||
targetParcel = pendingParcel | ||
break | ||
} | ||
|
||
// Break if we've found the target parcel. | ||
if targetParcel != nil { | ||
break | ||
} | ||
} | ||
|
||
// Return an error if the target parcel was not found. | ||
if targetParcel == nil { | ||
return nil, fmt.Errorf("target pending transfer output not " + | ||
"found") | ||
} | ||
|
||
// Request delivery of the updated parcel. | ||
pendingParcel := tapfreighter.NewPendingParcel(targetParcel) | ||
_, err = r.cfg.ChainPorter.RequestShipment(pendingParcel) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this be handled properly and not lead to duplicates? Shouldn't we add the possibility to update an existing parcel instead? |
||
if err != nil { | ||
return nil, fmt.Errorf("error requesting delivery of "+ | ||
"modified pending parcel: %w", err) | ||
} | ||
|
||
return &updateCourierAddrResp{}, nil | ||
} | ||
|
||
// QueryAddrs queries the set of Taproot Asset addresses stored in the database. | ||
func (r *rpcServer) QueryAddrs(ctx context.Context, | ||
req *taprpc.QueryAddrRequest) (*taprpc.QueryAddrResponse, error) { | ||
|
@@ -3382,6 +3495,7 @@ func marshalOutboundParcel( | |
OutputType: rpcOutType, | ||
AssetVersion: assetVersion, | ||
ProofDeliveryStatus: proofDeliveryStatus, | ||
Position: out.Position, | ||
} | ||
} | ||
|
||
|
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 we should go with just
UpdateTransferOutput
and only allow the proof courier address to be updated for now. And we'll just return an error if the selected transfer output isn't pending.IMO we don't have to put the whole documentation of a method into its name ;-)