Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EigenDA client using handshake based authentication #551

Merged
merged 13 commits into from
May 23, 2024

Conversation

teddyknox
Copy link
Contributor

@teddyknox teddyknox commented May 9, 2024

Why are these changes needed?

To provide a client for accessing EigenDA with the following features:

  1. BN254 31-byte encoding (dispersal and retrieval)
  2. Length prefix encoding to return exact blob length that was dispersed (dispersal and retrieval)
  3. IFFT encoding to enable location specific point opening for fraud/validity proofs. (dispersal and retrieval)
  4. Automatically waiting for finalization (abstracting over DisperseBlob() and GetBlobStatus()).
  5. Implement ECDSA signature based authentication.

This client will unify logic across many integrations and make EigenDA integrations easier to maintain.

This change also unifies the api/ package into the root module, which can be done because the original reason for separating api into its own module no longer applies; we have upgraded certain dependencies within the root module that were conflicting with Optimism's set of dependencies before.

My testing strategy here has been based on testnet integration tests, since otherwise testing this client would require either mocking out the disperser, spinning up an entire inabox environment, or some middle ground between the two. For now, given time constraints, it is fine to verify that the client works against testnet.

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@teddyknox teddyknox requested review from mooselumph and ian-shim May 9, 2024 11:29
@teddyknox teddyknox force-pushed the client branch 8 times, most recently from 4066683 to 79613a9 Compare May 9, 2024 16:37
@teddyknox teddyknox requested a review from anupsv May 9, 2024 18:07
api/client/client.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
@teddyknox teddyknox requested review from anupsv, epociask and afkbyte May 9, 2024 20:46
@mooselumph
Copy link
Collaborator

What is the intended relationship between this client and the client in the clients folder? This is creating a lot of duplication. Moreover, the client there is integrated into several unit tests throughout the codebase.

This change also unifies the api/ package into the root module, which can be done because the original reason for separating api into its own module no longer applies; we have upgraded certain dependencies within the root module that were conflicting with Optimism's set of dependencies before.

Do we anticipate this remaining to be the case?

If we are using the same module, then why should this code be contained in the api folder instead of the prexisting clients folder?

Copy link

@epociask epociask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some small comments - otherwise looks really good

api/client/client.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
api/client/utils_test.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of client implementation makes sense, and code lgtm. But I feel like it should belong on the integrator's codebase.
Is this client tailored for a specific integration or all integrations?
If it's tailored for a specific integration, it should belong in the integrator's codebase utilizing the generic disperser client.
If your judgement is that all integrations will use this client, we should update the existing client.

What do you think?

api/client/client.go Outdated Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
core/auth/signer.go Show resolved Hide resolved
core/auth/signer.go Show resolved Hide resolved
@teddyknox
Copy link
Contributor Author

teddyknox commented May 10, 2024

Thanks for all the feedback on this PR. I wasn't aware of the existing EigenDA client in the clients/ directory as pointed out by @mooselumph but I'm open to combining them. They seem to be operating on two different levels, where this one is higher-level and handles encoding and status polling, and the existing client seems lower-level, and offering polish over the GRPC endpoints and handshake-based authentication.

Since the existing client is being used for traffic generation and integration tests which depend on its lower level semantics, it doesn't seem viable to fold that client into this one.

Conversely, it does seem possible to factor out aspects of this client which could be handled by the lower-level client, but would require some duplication of supporting code, e.g. separate config objects etc. This is the path I will take in pending follow ups to this PR.

Based on @epociask's requests I think it does make sense to mock out the apiserver for the purpose of testing failure cases of this client, that work is also pending. For now though I've updated the branch with another draft of refactors that respond to the comments above.

@teddyknox
Copy link
Contributor Author

Ok I've updated this client to depend on the existing low level EigenDA client. The only thing I'd change about the low level client at this point is that the low level client reconnects with the server with every request. This seems like overkill, since GRPC connections are virtual and can abstract over whether there is a real TCP connection going at any point in time.

@teddyknox
Copy link
Contributor Author

In my latest push I've further updated this PR to add tests.

api/clients/cert.go Outdated Show resolved Hide resolved
api/clients/eigenda_client.go Show resolved Hide resolved
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, one comment on validation

api/clients/config.go Outdated Show resolved Hide resolved
api/clients/config.go Outdated Show resolved Hide resolved
api/clients/config.go Outdated Show resolved Hide resolved

func (c *EigenDAClientConfig) Check() error {
if c.StatusQueryRetryInterval == 0 {
c.StatusQueryRetryInterval = 5 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to rename the method name to make it clear to the caller that this method modifies the content of the receiver (or return a new config instead of modifying the receiver in place).

@teddyknox teddyknox merged commit 003c358 into Layr-Labs:master May 23, 2024
5 checks passed
@teddyknox teddyknox deleted the client branch May 23, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants