Adding nil error check to connection status for ETH1 nodes #10186
Conversation
michaelneuder
changed the title
| if err == nil { | ||
| errStrs = append(errStrs, "") |
There was a problem hiding this comment.
What do we need an empty string for? Why not just skip this item?
There was a problem hiding this comment.
If we leave the empty string out, it might be confusing in the case that some of the connections have errors and others do not. For example, let's say we have connections conn1, conn2, conn3 and errors only on the first and third connection. Then the error strings could either be err1, "", err2 or err1 err2 (if we don't use the empty strings). It seems like there might be some ambiguity in the second case, because err2 could either map to conn2 or conn3. WDYT?
There was a problem hiding this comment.
This makes perfect sense given that ETH1ConnectionErrors returns a value for each connection. Thanks!
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
Fixes the bug reported in #10165
Which issues(s) does this PR fix?
#10165
Other notes for review
Added a nil check for the error in connection status. I added to the unit test to verify the nil error case returns an empty string.