Enable creating InstrumentedClient from existing #117
Conversation
| @@ -104,7 +104,7 @@ func BuildAll( | |||
|
|
|||
| } | |||
|
|
|||
| func (config *BuildAllConfig) buildElClients( | |||
| func (config *BuildAllConfig) BuildElClients( | |||
There was a problem hiding this comment.
Why do you need this method exposed? Personally find it a bit weird given that the BuildAllConfig takes in AVS contract addresses. I'd prefer we expose a BuildElClients that takes in a core eigenlayer contract address, such as the delegation manager address.
Also if you're already putting together the BuildAllConfig, might as well just build everything, even if you just need the EL clients.
There was a problem hiding this comment.
Its connected to InstrumentedClient. The reason to expose this is because BuildAll build clients using eth.NewClient which in our case just creates duplicate clients. I would like to be able to pass wrapped eth.Client(InstrumentedClient) to achieve results of BuildAll but with my own client instances
There was a problem hiding this comment.
I see, makes sense to expose these I guess for people to use them, although I'd still prefer we clear the TODO on top of Clients struct, and make a different BuildALL for people who want instrumented client and another for people who don't want metrics. Perhaps easiest is only to expose the building blocks like this, but my main concern was just that the config struct is then a bit confusing. I'm fine with merging these changes for now if it helps, but if you end up creating other useful high-level constructors, please create new PRs with those. :)
There was a problem hiding this comment.
Sure) Will resolve conflicts in a few days.
taco-paco
force-pushed
the
feat/instrumented-client
branch
from
5ced414
to
0764f3f
Compare
Motivation
Make some methods accessible for third-party use to avoid creating duplicate instances. Make new constructor method for Instrumented client for 3-party integration
Solution
Open questions