From 47efaece60c70d9b80ade6300c72128a48bd1c6c Mon Sep 17 00:00:00 2001
From: whyrusleeping <why@ipfs.io>
Date: Tue, 13 Aug 2019 11:47:40 -0700
Subject: [PATCH] address review feedback

---
 node/impl/full.go | 14 ++++++++------
 paych/paych.go    |  8 ++++++--
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/node/impl/full.go b/node/impl/full.go
index ceb00d2b4..607171680 100644
--- a/node/impl/full.go
+++ b/node/impl/full.go
@@ -545,6 +545,11 @@ func (a *FullNodeAPI) PaychVoucherAdd(ctx context.Context, ch address.Address, s
 	return a.PaychMgr.AddVoucher(ctx, ch, sv)
 }
 
+// PaychVoucherCreate creates a new signed voucher on the given payment channel
+// with the given lane and amount.  The value passed in is exactly the value
+// that will be used to create the voucher, so if previous vouchers exist, the
+// actual additional value of this voucher will only be the difference between
+// the two.
 func (a *FullNodeAPI) PaychVoucherCreate(ctx context.Context, pch address.Address, amt types.BigInt, lane uint64) (*types.SignedVoucher, error) {
 	ci, err := a.PaychMgr.GetChannelInfo(pch)
 	if err != nil {
@@ -556,11 +561,6 @@ func (a *FullNodeAPI) PaychVoucherCreate(ctx context.Context, pch address.Addres
 		return nil, err
 	}
 
-	// REVIEW ME: The amount passed into this method is the absolute value on
-	// the channel.  I think that a common usecase here might be 'i want to pay
-	// person X an additional 4 FIL'.  If they just pass '4' for the amount,
-	// and the channel already has sent some amount, they wont actually be
-	// sending four additional filecoin.
 	sv := &types.SignedVoucher{
 		Lane:   lane,
 		Nonce:  nonce,
@@ -579,7 +579,9 @@ func (a *FullNodeAPI) PaychVoucherCreate(ctx context.Context, pch address.Addres
 
 	sv.Signature = sig
 
-	// REVIEWME: Should we immediately add this to the voucher store? or leave that to the caller?
+	if err := a.PaychMgr.AddVoucher(ctx, pch, sv); err != nil {
+		return nil, xerrors.Errorf("failed to persist voucher: %w", err)
+	}
 
 	return sv, nil
 }
diff --git a/paych/paych.go b/paych/paych.go
index b95e1e32d..43b75a8c7 100644
--- a/paych/paych.go
+++ b/paych/paych.go
@@ -72,6 +72,10 @@ func (pm *Manager) CheckVoucherValid(ctx context.Context, ch address.Address, sv
 	if err != nil {
 		return err
 	}
+
+	// TODO: technically, either party may create and sign a voucher.
+	// However, for now, we only accept them from the channel creator.
+	// More complex handling logic can be added later
 	if err := sv.Signature.Verify(pca.From, vb); err != nil {
 		return err
 	}
@@ -79,10 +83,10 @@ func (pm *Manager) CheckVoucherValid(ctx context.Context, ch address.Address, sv
 	sendAmount := sv.Amount
 
 	// now check the lane state
+	// TODO: should check against vouchers in our local store too
+	// there might be something conflicting
 	ls, ok := pca.LaneStates[fmt.Sprint(sv.Lane)]
 	if !ok {
-		// TODO: should check against vouchers in our local store too
-		// there might be something conflicting
 	} else {
 		if ls.Closed {
 			return fmt.Errorf("voucher is on a closed lane")