From b866d31bb8d18178d2e06ced5e12e1b34af21837 Mon Sep 17 00:00:00 2001
From: Fabio Barone <holistic.computing@gmail.com>
Date: Tue, 6 Nov 2018 05:24:11 -0500
Subject: [PATCH] swarm/network/stream: addressed PR comments

---
 swarm/network/stream/delivery_test.go | 25 +++++++++++++++++--------
 swarm/network/stream/stream.go        |  4 ++--
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/swarm/network/stream/delivery_test.go b/swarm/network/stream/delivery_test.go
index 0bec1f9381..29b4f2f69a 100644
--- a/swarm/network/stream/delivery_test.go
+++ b/swarm/network/stream/delivery_test.go
@@ -38,6 +38,7 @@ import (
 	"github.com/ethereum/go-ethereum/swarm/storage"
 )
 
+//Tests initializing a retrieve request
 func TestStreamerRetrieveRequest(t *testing.T) {
 	regOpts := &RegistryOptions{
 		Retrieval: RetrievalClientOnly,
@@ -64,7 +65,7 @@ func TestStreamerRetrieveRequest(t *testing.T) {
 	err = tester.TestExchanges(p2ptest.Exchange{
 		Label: "RetrieveRequestMsg",
 		Expects: []p2ptest.Expect{
-			{
+			{ //start expecting a subscription for RETRIEVE_REQUEST due to `RetrievalClientOnly`
 				Code: 4,
 				Msg: &SubscribeMsg{
 					Stream:   stream,
@@ -73,7 +74,7 @@ func TestStreamerRetrieveRequest(t *testing.T) {
 				},
 				Peer: node.ID(),
 			},
-			{
+			{ //expect a retrieve request message for the given hash
 				Code: 5,
 				Msg: &RetrieveRequestMsg{
 					Addr:      hash0[:],
@@ -89,10 +90,12 @@ func TestStreamerRetrieveRequest(t *testing.T) {
 	}
 }
 
+//Test requesting a chunk from a peer then issuing a "empty" OfferedHashesMsg (no hashes available yet)
+//Should time out as the peer does not have the chunk (no syncing happened previously)
 func TestStreamerUpstreamRetrieveRequestMsgExchangeWithoutStore(t *testing.T) {
 	tester, streamer, _, teardown, err := newStreamerTester(t, &RegistryOptions{
 		Retrieval: RetrievalEnabled,
-		Syncing:   SyncingDisabled,
+		Syncing:   SyncingDisabled, //do no syncing
 	})
 	defer teardown()
 	if err != nil {
@@ -106,15 +109,17 @@ func TestStreamerUpstreamRetrieveRequestMsgExchangeWithoutStore(t *testing.T) {
 	peer := streamer.getPeer(node.ID())
 
 	stream := NewStream(swarmChunkServerStreamName, "", true)
+	//simulate pre-subscription to RETRIEVE_REQUEST stream on peer
 	peer.handleSubscribeMsg(context.TODO(), &SubscribeMsg{
 		Stream:   stream,
 		History:  nil,
 		Priority: Top,
 	})
 
+	//test the exchange
 	err = tester.TestExchanges(p2ptest.Exchange{
 		Expects: []p2ptest.Expect{
-			{
+			{ //first expect a subscription to the RETRIEVE_REQUEST stream
 				Code: 4,
 				Msg: &SubscribeMsg{
 					Stream:   stream,
@@ -127,7 +132,7 @@ func TestStreamerUpstreamRetrieveRequestMsgExchangeWithoutStore(t *testing.T) {
 	}, p2ptest.Exchange{
 		Label: "RetrieveRequestMsg",
 		Triggers: []p2ptest.Trigger{
-			{
+			{ //then the actual RETRIEVE_REQUEST....
 				Code: 5,
 				Msg: &RetrieveRequestMsg{
 					Addr: chunk.Address()[:],
@@ -136,7 +141,7 @@ func TestStreamerUpstreamRetrieveRequestMsgExchangeWithoutStore(t *testing.T) {
 			},
 		},
 		Expects: []p2ptest.Expect{
-			{
+			{ //to which the peer responds with offered hashes
 				Code: 1,
 				Msg: &OfferedHashesMsg{
 					HandoverProof: nil,
@@ -149,6 +154,8 @@ func TestStreamerUpstreamRetrieveRequestMsgExchangeWithoutStore(t *testing.T) {
 		},
 	})
 
+	//should fail with a timeout as the peer we are requesting
+	//the chunk from does not have the chunk
 	expectedError := `exchange #1 "RetrieveRequestMsg": timed out`
 	if err == nil || err.Error() != expectedError {
 		t.Fatalf("Expected error %v, got %v", expectedError, err)
@@ -285,6 +292,7 @@ func TestStreamerDownstreamChunkDeliveryMsgExchange(t *testing.T) {
 
 	node := tester.Nodes[0]
 
+	//subscribe to custom stream
 	stream := NewStream("foo", "", true)
 	err = streamer.Subscribe(node.ID(), stream, NewRange(5, 8), Top)
 	if err != nil {
@@ -297,7 +305,7 @@ func TestStreamerDownstreamChunkDeliveryMsgExchange(t *testing.T) {
 	err = tester.TestExchanges(p2ptest.Exchange{
 		Label: "Subscribe message",
 		Expects: []p2ptest.Expect{
-			{
+			{ //first expect subscription to the custom stream...
 				Code: 4,
 				Msg: &SubscribeMsg{
 					Stream:   stream,
@@ -311,7 +319,8 @@ func TestStreamerDownstreamChunkDeliveryMsgExchange(t *testing.T) {
 		p2ptest.Exchange{
 			Label: "ChunkDelivery message",
 			Triggers: []p2ptest.Trigger{
-				{
+				{ //...then trigger a chunk delivery for the given chunk from peer in order for
+					//local node to get the chunk delivered
 					Code: 6,
 					Msg: &ChunkDeliveryMsg{
 						Addr:  chunkKey,
diff --git a/swarm/network/stream/stream.go b/swarm/network/stream/stream.go
index 45d653c921..695ff0c502 100644
--- a/swarm/network/stream/stream.go
+++ b/swarm/network/stream/stream.go
@@ -62,12 +62,12 @@ const (
 )
 
 const (
-	//Retrieval disabled
+	//Retrieval disabled. Used mostly for tests to isolate syncing features (i.e. syncing only)
 	RetrievalDisabled RetrievalOption = iota
 	//Only the client side of the retrieve request is registered.
 	//(light nodes do not serve retrieve requests)
 	//once the client is registered, subscription to retrieve request stream is always sent
-	RetrievalClientOn
+	RetrievalClientOnly
 	//Both client and server funcs are registered, subscribe sent automatically
 	RetrievalEnabled
 )