refactor(shwap): Extract eds interface #3452
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
walldiss
requested review from
renaynay,
Wondertan,
vgonkivs,
distractedm1nd and
ramin
as code owners
cristaloleg
reviewed
May 31, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## shwap #3452 +/- ##
========================================
Coverage ? 43.26%
========================================
Files ? 286
Lines ? 16433
Branches ? 0
========================================
Hits ? 7110
Misses ? 8492
Partials ? 831 ☔ View full report in Codecov by Sentry. |
Wondertan
reviewed
May 31, 2024
|
Drafting until we migrate some funcs over EDS interface |
Wondertan
reviewed
May 31, 2024
|
I've applied review suggestions in 46daf13 and listed what changed in PR description in Additional refactoring section. |
cristaloleg
reviewed
Jun 3, 2024
ramin
reviewed
Jun 3, 2024
There was a problem hiding this comment.
had a very superficial look / review, the interfaces and methods are starting to make sense to me as a layman and someone that might need to use them 😸
One question: is share/new_eds meant to survive? I wonder if having it organized more like
share/eds/v2 or something would be better than new_eds?
|
To @ramin:
Not going to survive. Eventually (in the late shwap PRs) It will land into eds pkg. For now it is not there, because existing eds pkg contains a lot of code and new code would get lost there. Existing eds pkg code will be removed entirely and will be replaced with content of new_eds |
extract switch simplify AxisHalf
Wondertan
reviewed
Jun 4, 2024
Wondertan
reviewed
Jun 5, 2024
Wondertan
reviewed
Jun 5, 2024
walldiss
added a commit
that referenced
this pull request
Jun 10, 2024
- Reintroduce file interface as eds interface. Change aims to allow
usage of EDS interface outside of storage package and to be high level
interface of EDS methods.
- Renames of eds interface methods to align with returned shwap types
names
- Share() -> Sample
- Data -> Row data
- Extracts New<shwap_type_name>FromEDS functions to eds file methods
- moves associated tests to eds pkg
**Additional refactoring:**
- **Change Interface Name**: Realized that 'EDS' is a terrible name for
an interface. Renamed `eds.EDS` to `eds.Accessor` to more accurately
reflect its functionality rather than its internal content.
- **Separate Closer**: Extracted `Closer` from `Accessor`. Now it is
available in a new composite interface `AccessorCloser`.
- **Rename InMem**: Renamed `InMem` to `rsmt2d` to better align with its
usage.
- **Decouple NamespacedData**: Separated `NamespacedData` from the
`rsmt2d` implementation. It is now a standalone function.
- **Update EDS Method**: Replaced the `EDS()` method with `Flattened`,
similar to `rsmt2d`. Considered introducing two separate methods,
`Flattened` and `FlattenedODS`, with the latter to be potentially added
later. Proposed to park this suggestion in an issue for future
consideration.
walldiss
added a commit
that referenced
this pull request
Jun 10, 2024
- Reintroduce file interface as eds interface. Change aims to allow
usage of EDS interface outside of storage package and to be high level
interface of EDS methods.
- Renames of eds interface methods to align with returned shwap types
names
- Share() -> Sample
- Data -> Row data
- Extracts New<shwap_type_name>FromEDS functions to eds file methods
- moves associated tests to eds pkg
**Additional refactoring:**
- **Change Interface Name**: Realized that 'EDS' is a terrible name for
an interface. Renamed `eds.EDS` to `eds.Accessor` to more accurately
reflect its functionality rather than its internal content.
- **Separate Closer**: Extracted `Closer` from `Accessor`. Now it is
available in a new composite interface `AccessorCloser`.
- **Rename InMem**: Renamed `InMem` to `rsmt2d` to better align with its
usage.
- **Decouple NamespacedData**: Separated `NamespacedData` from the
`rsmt2d` implementation. It is now a standalone function.
- **Update EDS Method**: Replaced the `EDS()` method with `Flattened`,
similar to `rsmt2d`. Considered introducing two separate methods,
`Flattened` and `FlattenedODS`, with the latter to be potentially added
later. Proposed to park this suggestion in an issue for future
consideration.
walldiss
added a commit
to walldiss/celestia-node
that referenced
this pull request
Jul 6, 2024
- Reintroduce file interface as eds interface. Change aims to allow
usage of EDS interface outside of storage package and to be high level
interface of EDS methods.
- Renames of eds interface methods to align with returned shwap types
names
- Share() -> Sample
- Data -> Row data
- Extracts New<shwap_type_name>FromEDS functions to eds file methods
- moves associated tests to eds pkg
**Additional refactoring:**
- **Change Interface Name**: Realized that 'EDS' is a terrible name for
an interface. Renamed `eds.EDS` to `eds.Accessor` to more accurately
reflect its functionality rather than its internal content.
- **Separate Closer**: Extracted `Closer` from `Accessor`. Now it is
available in a new composite interface `AccessorCloser`.
- **Rename InMem**: Renamed `InMem` to `rsmt2d` to better align with its
usage.
- **Decouple NamespacedData**: Separated `NamespacedData` from the
`rsmt2d` implementation. It is now a standalone function.
- **Update EDS Method**: Replaced the `EDS()` method with `Flattened`,
similar to `rsmt2d`. Considered introducing two separate methods,
`Flattened` and `FlattenedODS`, with the latter to be potentially added
later. Proposed to park this suggestion in an issue for future
consideration.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Additional refactoring:
Change Interface Name: Realized that 'EDS' is a terrible name for an interface. Renamed
eds.EDStoeds.Accessorto more accurately reflect its functionality rather than its internal content.Separate Closer: Extracted
CloserfromAccessor. Now it is available in a new composite interfaceAccessorCloser.Rename InMem: Renamed
InMemtorsmt2dto better align with its usage.Decouple NamespacedData: Separated
NamespacedDatafrom thersmt2dimplementation. It is now a standalone function.Update EDS Method: Replaced the
EDS()method withFlattened, similar torsmt2d. Considered introducing two separate methods,FlattenedandFlattenedODS, with the latter to be potentially added later. Proposed to park this suggestion in an issue for future consideration.