rpc: add SetPendingTransferProofCourierAddr endpoint to fix erroneous courier addresses #1097
Conversation
This commit renames the ChainPorter.transferReceiverProof method for clarity, reflecting that multiple proofs may be transferred. The documentation has also been updated accordingly.
This commit introduces the `position` field to the `TransferOutput` proto message. This field serves as a unique index, enabling precise identification of an output within a given transfer.
This commit adds a new RPC endpoint which can be used to override the proof courier address for a pending asset transfer output.
|
I'm looking for early feedback on the general direction of this PR. This PR is in draft for the following reasons:
|
Pull Request Test Coverage Report for Build 10490103886Details
💛 - Coveralls |
| @@ -40,6 +40,10 @@ var ( | |||
| Entity: "assets", | |||
| Action: "read", | |||
| }}, | |||
| "/taprpc.TaprootAssets/SetPendingTransferProofCourierAddr": {{ | |||
There was a problem hiding this comment.
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 ;-)
| message SetPendingTransferProofCourierAddrRequest { | ||
| // The new proof courier address to use for the target transfer output proof | ||
| // delivery. | ||
| string new_proof_courier_addr = 1; |
There was a problem hiding this comment.
Maybe to make it more clear we can only update this field (for now?), we could put that into a new sub-message.
So something like this:
message UpdateTransferOutput {
// The hexadecimal encoded transaction ID (txid) of the anchor transaction
// for the target transfer.
string anchor_txid = 1;
// Script public key of the target transfer output.
bytes transfer_output_script_pub_key = 2;
// The position of the output in the transfer output list.
uint64 transfer_output_position = 3;
TransferOutputModify modify_params = 4;
}
message TransferOutputModify {
string proof_courier_addr = 1;
There was a problem hiding this comment.
With the current definition I think this is the only reasonable field to change rn, but I agree that using a sub-message is a worthwhile change.
| } | ||
|
|
||
| // Unmarshal the transfer output script key public key. | ||
| outputScriptKey, err := parseUserKey(req.TransferOutputScriptPubKey) |
There was a problem hiding this comment.
So the pubkey is a safety net to make sure the user really specified the right one?
| ctx, anchorTxHash, true, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to query parcels: %w", err) |
There was a problem hiding this comment.
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"?
| // 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.
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.
|
|
||
| // Request delivery of the updated parcel. | ||
| pendingParcel := tapfreighter.NewPendingParcel(targetParcel) | ||
| _, err = r.cfg.ChainPorter.RequestShipment(pendingParcel) |
There was a problem hiding this comment.
Will this be handled properly and not lead to duplicates? Shouldn't we add the possibility to update an existing parcel instead?
|
|
||
| // The hexadecimal encoded transaction ID (txid) of the anchor transaction | ||
| // for the target transfer. | ||
| string anchor_txid = 2; |
There was a problem hiding this comment.
I think this should be the full outpoint, not just the TXID? Otherwise we aren't uniquely identifying the transfer output.
The transfer output list is per-transfer, but IIUC there is no requirement of exactly 1 transfer per TX (can I combine multiple transfers into one TX?)
So the suggested field would be anchor_outpoint, then in the RPC call implementation you could add a filter on pendingParcel.Outputs for outputs with a matching Anchor.OutPoint.
There was a problem hiding this comment.
As a more concrete example, If there are two transfers with the same TXID, the result of QueryParcels will contain the outputs of both transfers.
If I only use outputScriptKey or TransferOutputPosition to specify the TransferOutput, that could match outputs from both transfers, when we want to match exactly one TransferOutput.
This PR introduces a new RPC endpoint, SetPendingTransferProofCourierAddr, which allows overriding the proof courier address for a pending asset transfer output. This enhancement directly addresses the issue of correcting erroneous courier addresses during asset transfers as described in issue #1082.