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.

Sign up for GitHub

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

Jump to bottom

Added README files in v2 proto directories. #812

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added README files in v2 proto directories. #812

wants to merge 3 commits into from

Conversation

cody-littley
Copy link
Contributor

Why are these changes needed?

Adds README files in directories containing API not yet in use.

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.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@cody-littley
Added README files in v2 proto directories.
2f299ad 
Signed-off-by: Cody Littley <cody@eigenlabs.org>
@cody-littley cody-littley self-assigned this Oct 16, 2024
samlaf
samlaf reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for this. would personally like to have a README in api/proto/README.md to describe v1 vs v2. Like an entrypoint README, that could link to the others.

ian-shim
ian-shim reviewed Oct 16, 2024
View reviewed changes
api/proto/disperser/v2/README.md Outdated

# What is "v2"?

The EigenDA v2 Architecture is a fundamental redesign of the disperser, and by extension systems that interact
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ... redesign of the protocol...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wording updated

api/proto/disperser/v2/README.md Outdated
# What is "v2"?

The EigenDA v2 Architecture is a fundamental redesign of the disperser, and by extension systems that interact
with the disperser. The v2 Architecture is an important stepping stone towards reducing centralization and for
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The v2 architecture improves robustness, efficiency, and compatibility with upcoming features such as permissionless disperser and data availability sampling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wording updated

@cody-littley
Copy link
Contributor Author

@samlaf

LGTM! thanks for this. would personally like to have a README in api/proto/README.md to describe v1 vs v2. Like an entrypoint README, that could link to the others.

I added a readme to the proto directory. Not sure if it makes sense to link it to all of the other ones. The README files are basically all just copies of one another. They are currently spread around in multiple locations to make it more likely that somebody will think to look inside it when they are peeking into the v2 files.

@cody-littley
Made suggested changes.
35d9400 
Signed-off-by: Cody Littley <cody@eigenlabs.org>
ian-shim
ian-shim approved these changes Oct 17, 2024
View reviewed changes
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.

I feel like having one README at api/proto is sufficient, but I'll leave it up to you

api/proto/relay/README.md Outdated
@@ -0,0 +1,15 @@
# Should I use the v2 API?
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no indication api/proto/relay is part of v2. Maybe this README should be specific to relay or say something like relay is only applicable with v2 architecture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note that the code in the relay directory is experimental.

@cody-littley
Made suggested changes.
17c9f4b 
Signed-off-by: Cody Littley <cody@eigenlabs.org>
@cody-littley
Copy link
Contributor Author

I feel like having one README at api/proto is sufficient, but I'll leave it up to you

I've collapsed these README files into a single file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samlaf samlaf samlaf left review comments

@ian-shim ian-shim ian-shim approved these changes

@mooselumph mooselumph Awaiting requested review from mooselumph

Assignees

@cody-littley cody-littley

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cody-littley @samlaf @ian-shim