Add CreatePayment, ReceivePayment API methods to go-nitro RPC client
#1370
Conversation
reusing payments.Voucher as the data format
This requires some thought. Are engine sideEffects generally presumed to be benign? Does this change open attack vectors? The same immediate benefit could be had by opening a vouchers-only pipe, but that feels more like api-creep than this general method.
This is beefier than other request methods in order to return a sensible synchronous response (amount received).
This is safer, since incoming messages are not "generally assumed" to be safe.
so that async creation request can be identified
so that async creation request can be identified
rather than cleaning directly after setup
temporary. Needed because notifs now contain voucher data - harder to specify in advance
manually resolving some merge conflict
✅ Deploy Preview for nitrodocs canceled.
|
client_test/rpc_test.go
Outdated
| @@ -445,7 +514,7 @@ func checkNotifications[T channelInfo](t *testing.T, client string, required []T | |||
| } | |||
| if len(unexpectedNotifications) > 0 { | |||
| logUnexpected() | |||
| t.FailNow() | |||
| // t.FailNow() // todo: re-enable this with a change to the notif check mechanism (or consider scrapping it) | |||
There was a problem hiding this comment.
When are we going todo this?
There was a problem hiding this comment.
Do we still need the // todo comment?
| // if true, go-nitro will not send the payment to the counterparty, instead | ||
| // returning it to the calling client for sending. |
There was a problem hiding this comment.
Nit: We should be careful to ensure this wording matches the behaviour well -- here it sounds like the voucher won't be returned unless Withhold = true.
Arguably, we could remove this godoc to remove the need worry about keeping it in sync with behaviour. The important place to have that godoc will be around the API itself (already covered I think).
| return processRequest(rs, requestData, func(v serde.ReceivePaymentRequest) (query.PaymentChannelPaymentReceipt, error) { | ||
| pc, err := rs.client.GetPaymentChannel(v.ChannelId) | ||
| if err != nil { | ||
| return query.PaymentChannelPaymentReceipt{ | ||
| Status: query.PRSchannelNotFound, | ||
| }, err | ||
| } | ||
|
|
||
| me := rs.client.Address | ||
|
|
||
| if !bytes.Equal(me.Bytes(), pc.Balance.Payee.Bytes()) { | ||
| return query.PaymentChannelPaymentReceipt{ | ||
| ID: v.ChannelId, | ||
| Status: query.PRSmisaddressed, | ||
| }, err | ||
| } | ||
|
|
||
| signer, err := v.RecoverSigner() | ||
|
|
||
| if !bytes.Equal(signer.Bytes(), pc.Balance.Payer.Bytes()) || err != nil { | ||
| return query.PaymentChannelPaymentReceipt{ | ||
| ID: v.ChannelId, | ||
| Status: query.PRSincorrectSigner, | ||
| }, err | ||
| } | ||
|
|
||
| amountReceived := big.NewInt(0).Sub(v.Amount, (*big.Int)(pc.Balance.PaidSoFar)) |
There was a problem hiding this comment.
This feels like maybe too much responsibility for the rpc server. Can't we just push in the message and let the engine handle all of this logic?
There was a problem hiding this comment.
I think in fact the voucher manager should be doing as much of the heavy lifting as possible.
There was a problem hiding this comment.
I generally agree. Note: the engine, via the voucher manager, does handle all of this logic. The rpc server duplicates some of it in order to synchronously return something that the caller can use to make decisions with.
Maybe its better to locally watch the update chan and return a result?
| // return an *optimistic* appraisal of amount received. Engine processing could feasibly | ||
| // fail, but above checks are pretty comprehensive | ||
| return query.PaymentChannelPaymentReceipt{ | ||
| ID: v.ChannelId, | ||
| AmountReceived: (*hexutil.Big)(amountReceived), | ||
| Status: query.PRSreceived, | ||
| }, nil | ||
| }) |
There was a problem hiding this comment.
Not sure I understand the implications of this. Can you explain a bit more?
| } | ||
| } | ||
|
|
||
| return vChan, nil |
There was a problem hiding this comment.
I think we can reduce indirection here, see 02697ad
| // of the given payment channel by the given amount. | ||
| // | ||
| // The returned voucher must be sent to the payee. | ||
| func (c *Client) CreatePayment(channelId types.Destination, amount *big.Int) (<-chan payments.Voucher, error) { |
There was a problem hiding this comment.
Somewhat out of scope: are we happy with sprinkling "payment" widely through our API and codebase? "Voucher" has fewer connotations and we get to define what it means. But I feel like "payment" has the opportunity to mislead, given the replace-by-incentive nature of vouchers?
The godoc you have here is good, I think elsewhere existing godoc is not as clear about what calling Pay actually means.
There was a problem hiding this comment.
A good question that has two alternative answers. The case for removing Payment verbiage is as you described - a new terminology carries less baggage.
On the other hand, pay and payment are universally understood. Vouchers are implementation details that we could consider obscuring from the API entirely.
Agreed on the godoc, although we may suffer the curse of too much knowledge - an outsider wouldn't look at a function like Pay(amount) and wonder whether this function bumps total spend up to amount or by amount. (I did wonder this, and dug through code to find out). Obvious regular usage is that it bumps total spend by amount.
| func (c *Client) CreatePayment(channelId types.Destination, amount *big.Int) (<-chan payments.Voucher, error) { | ||
| vChan := make(chan payments.Voucher, 1) | ||
| id := rand.Uint64() | ||
| c.engine.PaymentRequestsFromAPI <- engine.PaymentRequest{ChannelId: channelId, Amount: amount, Withhold: true, PaymentID: id} |
There was a problem hiding this comment.
Instead of routing the payment request to the engine I wonder if CreatePayment could call VoucherManager.Pay directly. That would mean the node has to have a reference to the VoucherManager (and the voucher manager would have to be concurrency-safe) but this would avoid some of the pain of routing through the engine and adding a Withold flag.
Instead we'd just call VoucherManager.Pay to generate a new Voucher and return that. Since we'd avoid the engine we'd avoid sending the voucher over the message service.
Toward #1326.
PR introduces CreatePayment and ReceivePayment methods to the go-nitro rpc client, plumbing (go-nitro rpc-server & go nitro Client) to serve these requests, as well as a test for their correctness.
Some modest refactors along the way as well.