From 7aecdc148d16e3f53cc6c3e36ecdeceec0b6ebe5 Mon Sep 17 00:00:00 2001
From: Marco Munizaga <git@marcopolo.io>
Date: Wed, 29 Jun 2022 21:18:16 -0700
Subject: [PATCH] PR nits

---
 allowlist.go      | 24 ++++++++++++++----------
 allowlist_test.go |  4 ++--
 rcmgr_test.go     |  2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/allowlist.go b/allowlist.go
index 9e919c5..6a4fb1c 100644
--- a/allowlist.go
+++ b/allowlist.go
@@ -104,12 +104,12 @@ func toIPNet(ma multiaddr.Multiaddr) (*net.IPNet, peer.ID, error) {
 // e.g. /ip4/1.2.3.4/p2p/QmFoo, /ip4/1.2.3.4, and /ip4/1.2.3.0/ipcidr/24 are valid.
 // /p2p/QmFoo is not valid.
 func (al *Allowlist) Add(ma multiaddr.Multiaddr) error {
-	al.mu.Lock()
-	defer al.mu.Unlock()
 	ipnet, allowedPeer, err := toIPNet(ma)
 	if err != nil {
 		return err
 	}
+	al.mu.Lock()
+	defer al.mu.Unlock()
 
 	if allowedPeer != peer.ID("") {
 		// We have a peerID constraint
@@ -124,16 +124,16 @@ func (al *Allowlist) Add(ma multiaddr.Multiaddr) error {
 }
 
 func (al *Allowlist) Remove(ma multiaddr.Multiaddr) error {
-	al.mu.Lock()
-	defer al.mu.Unlock()
-
 	ipnet, allowedPeer, err := toIPNet(ma)
 	if err != nil {
 		return err
 	}
+	al.mu.Lock()
+	defer al.mu.Unlock()
+
 	ipNetList := al.allowedNetworks
 
-	if allowedPeer != peer.ID("") {
+	if allowedPeer != "" {
 		// We have a peerID constraint
 		ipNetList = al.allowedPeerByNetwork[allowedPeer]
 	}
@@ -149,10 +149,14 @@ func (al *Allowlist) Remove(ma multiaddr.Multiaddr) error {
 			if i == len(ipNetList)-1 {
 				// Trim this element from the end
 				ipNetList = ipNetList[:i]
+				// We only remove one thing
+				break
 			} else {
 				// swap remove
 				ipNetList[i] = ipNetList[len(ipNetList)-1]
 				ipNetList = ipNetList[:len(ipNetList)-1]
+				// We only remove one thing
+				break
 			}
 		}
 	}
@@ -167,12 +171,12 @@ func (al *Allowlist) Remove(ma multiaddr.Multiaddr) error {
 }
 
 func (al *Allowlist) Allowed(ma multiaddr.Multiaddr) bool {
-	al.mu.RLock()
-	defer al.mu.RUnlock()
 	ip, err := manet.ToIP(ma)
 	if err != nil {
 		return false
 	}
+	al.mu.RLock()
+	defer al.mu.RUnlock()
 
 	for _, network := range al.allowedNetworks {
 		if network.Contains(ip) {
@@ -192,12 +196,12 @@ func (al *Allowlist) Allowed(ma multiaddr.Multiaddr) bool {
 }
 
 func (al *Allowlist) AllowedPeerAndMultiaddr(peerID peer.ID, ma multiaddr.Multiaddr) bool {
-	al.mu.RLock()
-	defer al.mu.RUnlock()
 	ip, err := manet.ToIP(ma)
 	if err != nil {
 		return false
 	}
+	al.mu.RLock()
+	defer al.mu.RUnlock()
 
 	for _, network := range al.allowedNetworks {
 		if network.Contains(ip) {
diff --git a/allowlist_test.go b/allowlist_test.go
index 713e5b8..7f5267e 100644
--- a/allowlist_test.go
+++ b/allowlist_test.go
@@ -38,8 +38,8 @@ func TestAllowedWithPeer(t *testing.T) {
 
 	peerA := test.RandPeerIDFatal(t)
 	peerB := test.RandPeerIDFatal(t)
-	multiaddrA, _ := multiaddr.NewMultiaddr("/ip4/1.2.3.4/tcp/1234")
-	multiaddrB, _ := multiaddr.NewMultiaddr("/ip4/2.2.3.4/tcp/1234")
+	multiaddrA := multiaddr.StringCast("/ip4/1.2.3.4/tcp/1234")
+	multiaddrB := multiaddr.StringCast("/ip4/2.2.3.4/tcp/1234")
 
 	testcases := []testcase{
 		{
diff --git a/rcmgr_test.go b/rcmgr_test.go
index 1656a0d..fdf8dbf 100644
--- a/rcmgr_test.go
+++ b/rcmgr_test.go
@@ -10,7 +10,7 @@ import (
 	"github.com/multiformats/go-multiaddr"
 )
 
-var dummyMA, _ = multiaddr.NewMultiaddr("/ip4/1.2.3.4/tcp/1234")
+var dummyMA = multiaddr.StringCast("/ip4/1.2.3.4/tcp/1234")
 
 func TestResourceManager(t *testing.T) {
 	peerA := peer.ID("A")