added error logs for avs reader/writer clients #115
Conversation
| @@ -119,25 +119,30 @@ func BuildAvsRegistryChainReader( | |||
| ) (*AvsRegistryChainReader, error) { | |||
| contractRegistryCoordinator, err := regcoord.NewContractRegistryCoordinator(registryCoordinatorAddr, ethClient) | |||
| if err != nil { | |||
| logger.Error("Failed to create contractRegistryCoordinator", "err", err) | |||
There was a problem hiding this comment.
should we rather wrap the error with informative text and then return it and not log here? reason it's client responsibility to log errors otherwise we have too many logs?
There was a problem hiding this comment.
Yeah I was thinking so too. But we've been doing it this way everywhere else so wanted to stay consistent. Shall we change it everywhere?
There was a problem hiding this comment.
yea we should maybe do this ? https://pkg.go.dev/errors#Join
for ex: errors.Join(errors.new("Failed to create contractRegistryCoordinator", err))
There was a problem hiding this comment.
I agree with @shrimalmadhur , you should return the wrapped error and the client should handle it.
There was a problem hiding this comment.
yea we should maybe do this ? https://pkg.go.dev/errors#Join
for ex:
errors.Join(errors.new("Failed to create contractRegistryCoordinator", err))
join literally concatenates the error strings with a newline in between, such that printing them prints the 2 errors on two separate lines. Is that fine with you @uri-lightblocks ? I know you didn't like the newlines in avs-sync.
There was a problem hiding this comment.
ok changed here: 856915e
There was a problem hiding this comment.
I prefer to have the error string concatenated without a newline.
There was a problem hiding this comment.
@uri-lightblocks tell me more man. How would you concatenate it? With a space in between? A tilda? A "-"? :)
There was a problem hiding this comment.
@shrimalmadhur you already had a wrapError function which concatenates with a ":". I made it public and changed everywhere to use it in 33eac92
There was a problem hiding this comment.
Let's get this merged. At least now we can easily change it whenever.
some avs teams who are forking our clients and remove some contracts such as the apkRegistry end up with bugs here, and the errors were not informative, so added logging lines for them.