Separate out client v2 package #986
Conversation
ian-shim
force-pushed
the
clients-v2-package
branch
from
7f28131 to
2ae40a7
Compare
disperser/controller/dispatcher.go
Outdated
| "github.com/Layr-Labs/eigenda/api/clients" | ||
| "github.com/prometheus/client_golang/prometheus" | ||
|
|
||
| clients "github.com/Layr-Labs/eigenda/api/clients/v2" |
There was a problem hiding this comment.
how about using "github.com/Layr-Labs/eigenda/api/v2/clients" instead? This way we don't need this renaming on every import.
There was a problem hiding this comment.
Hmm this would prob be a much bigger refactor though right?
Would have been nice from the getgo if the entire api was versioned, but I see that every single package inside api has its own v2 packages. Is there a lot of code sharing between the v1 and v2 packages? If not, perhaps we could consider versioning the entire api?
There was a problem hiding this comment.
I don't think this would be a big refactor to do just within the api package.
Yes, it would be more difficult to do this in other packages because there's a lot of sharing
There was a problem hiding this comment.
I think it would be very nice to do, but don't have enough context on everything in the api package, and whether this refactor would make sense with the rest of the code in this repo.
api/clients/v2/disperser_client.go
Outdated
| @@ -1,4 +1,4 @@ | |||
| package clients | |||
| package v2 | |||
There was a problem hiding this comment.
How about keeping the package name clients here?
Its typical for golang modules to use an "invisible" version suffix (this is actually part of how golang's algo does version management). See for eg this urfave example: https://github.com/urfave/cli/blob/main/examples/example-cli/example-cli.go (imported as cli/v3, but module is just used as cli., not v3. which would be awkward).
I'm not sure if its typical to do the same thing for packages, but I feel like it would be cleaner?
There was a problem hiding this comment.
To do this, don't you need to version the entire module (like here in your example)?
There was a problem hiding this comment.
No I'm just saying leave the directory structure the same (api/clients/v2/.*.go), but name the package clients (package can be named different than the directory).
| type DisperserClientConfig struct { | ||
| Hostname string | ||
| Port string | ||
| UseSecureGrpcFlag bool | ||
| } | ||
|
|
||
| type DisperserClientV2 interface { | ||
| type DisperserClient interface { |
There was a problem hiding this comment.
If we keep the package name clients, then we could simplify these to clients.DisperserConfig and clients.Disperser.
ian-shim
force-pushed
the
clients-v2-package
branch
from
2ae40a7 to
d06410f
Compare
There was a problem hiding this comment.
Approving so we can move on with our lives, although let's keep in mind a future move to versioning the entire api package (api/v2/...).
I'd also still vote for clients.Disperser instead of clients.DisperserClient but I'm fine if people like the more verbose version.
ian-shim
force-pushed
the
clients-v2-package
branch
from
d06410f to
d76d6d4
Compare
Why are these changes needed?
Instead of suffixing
v2, create a separatev2package underclientsChecks