feat: Proxy client #33
Conversation
| } | ||
|
|
||
| // client is the implementation of ProxyClient | ||
| type client struct { |
There was a problem hiding this comment.
- Why not expose the concrete type?
- Naming-wise, I think "eigenda" should either be in the package name or in the struct/interface name.
There was a problem hiding this comment.
Is this client specific to OP stack or general?
| httpClient *http.Client | ||
| } | ||
|
|
||
| func New(cfg *Config) ProxyClient { |
There was a problem hiding this comment.
i.e. why not return a client type? (may need to make it a public type)
| return err | ||
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { |
There was a problem hiding this comment.
Would be nice to add a protocol for including error messages from the response body in the error output here.
|
|
||
| // ProxyClient is an interface for communicating with the EigenDA proxy server | ||
| type ProxyClient interface { | ||
| Health() error |
There was a problem hiding this comment.
Is this part of the OP plasma interface? I figured this endpoint was for docker healthchecks.
| } | ||
|
|
||
| // encode prefix bytes | ||
| b = eigenda.GenericPrefix(eigenda.Commitment(b).Encode()) |
There was a problem hiding this comment.
nit: not terribly ergonomic to have Commitment(b) do nothing to b without a corresponding call to .Encode(). What if it looked more like
type genericPrefix String
type commitment String
b = eigenda.NewGenericPrefix(eigenda.NewCommitment(b))
where these are functions and not type casting operators and b
There was a problem hiding this comment.
Update:
I'm now seeing that GenericPrefix() is already a function, but Commitment() is not. So my suggestion would be to make Commitment() into a simple function so the Encode() part can be dropped.
client/client.go
Outdated
| return nil, fmt.Errorf("read certificate is of invalid length: %d", len(b)) | ||
| } | ||
|
|
||
| var blob *common.Certificate |
There was a problem hiding this comment.
nit: rename blob var to cert
common/common.go
Outdated
| @@ -93,3 +110,26 @@ func ParseBytesAmount(s string) (uint64, error) { | |||
| return 0, fmt.Errorf("unsupported unit: %s", unit) | |||
| } | |||
| } | |||
|
|
|||
| func SubsetExists[P comparable](arr, sub []P) bool { | |||
There was a problem hiding this comment.
where is this being used?
| wait.For(ts.ctx, time.Second*1, func() (bool, error) { | ||
| err := daClient.Health() | ||
| if err != nil { | ||
| return false, nil | ||
| } | ||
|
|
||
| return true, nil | ||
| }) |
There was a problem hiding this comment.
I believe that createTestSuite() is already waiting for the server to come up.
eigenda-proxy/server/server.go
Line 114 in f270ca1
There was a problem hiding this comment.
this just does a time wait and verifies that no error is generated within 10 ms. Feel like doing the await gurantees client <-> server communication is ready versus instantiation the server construct
| @@ -165,3 +221,55 @@ func TestE2EPutGetLogicForMemoryStore(t *testing.T) { | |||
| require.NoError(t, err) | |||
| require.Equal(t, testPreimage, preimage) | |||
| } | |||
|
|
|||
| func TestMemStoreWithProxyClient(t *testing.T) { | |||
There was a problem hiding this comment.
nit: Theoretically it should be possible to dry up these tests with a loop + test configuration array. Not necessary to do manually but maybe chatgpt can do.
There was a problem hiding this comment.
So the idea here is that we have different versions of the proxy conform to the effective plasma interface of different versions of optimism, but then we maintain this client to hold a certain idealized interface constant through those changes. And this client can be used in Orbit, ZK Sync, CDK. Do I have that right?
Approving with comments.
Closes: