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

rpc: add SetPendingTransferProofCourierAddr endpoint to fix erroneous courier addresses #1097

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions perms/perms.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ var (
Entity: "assets",
Action: "read",
}},
"/taprpc.TaprootAssets/SetPendingTransferProofCourierAddr": {{
Copy link
Member

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 ;-)

Entity: "assets",
Action: "write",
}},
"/taprpc.TaprootAssets/QueryAddrs": {{
Entity: "addresses",
Action: "read",
Expand Down
113 changes: 113 additions & 0 deletions rpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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?
I assume it has something to do with how we insert a new parcel, see comment below.

}

// 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)
Copy link
Member

Choose a reason for hiding this comment

The 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) {
Expand Down
Loading
Loading