celestiaorg / rsmt2d

Go implementation of two dimensional Reed-Solomon merkle tree data availability scheme.
Apache License 2.0
162 stars 80 forks source link

refactor: rename share to chunk #299

Closed rootulp closed 9 months ago

rootulp commented 9 months ago

Closes https://github.com/celestiaorg/rsmt2d/issues/215

I opted to use chunk instead of share because @musalbas preferred it and chunk already appears in the exported API of this library (e.g. ValidateChunkSize, MaxChunks).

Technically Shares is in the public API too (it's a field of ErrByzantineData) but this PR doesn't introduce any breaking changes b/c IMO it's not worth breaking API over this name change.

codecov[bot] commented 9 months ago

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (d6c118c) 80.89% compared to head (5223ae4) 82.24%. Report is 7 commits behind head on main.

Files Patch % Lines
extendeddatacrossword.go 80.95% 7 Missing and 1 partial :warning:
leopard.go 83.33% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #299 +/- ## ========================================== + Coverage 80.89% 82.24% +1.34% ========================================== Files 8 7 -1 Lines 869 614 -255 ========================================== - Hits 703 505 -198 + Misses 119 66 -53 + Partials 47 43 -4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

musalbas commented 9 months ago

What about celestia-node, celestia-app, and other repos? Cc @renaynay @Wondertan

rootulp commented 9 months ago

celestia-node has ~183 occurrences of share and ~4 for chunk. celestia-app almost always uses shares and not chunk. I had a preference for share but you preferred chunk. See https://github.com/celestiaorg/rsmt2d/issues/262#issuecomment-1668381460.

musalbas commented 9 months ago

If share is overwhelmingly used more than chunk then I'm not against using share

musalbas commented 9 months ago

But in this repo it seems that chunk is used more than share?

rootulp commented 9 months ago

This repo is split. 188 occurrences of chunk and 300 occurrences of share. I still lean slightly towards share because usage in downstream repos but not strongly opinionated on it. Can do whichever reviewers prefer.

musalbas commented 9 months ago

Wonder what Celestia node API uses. I also do think it seems people say share more than chunks

On Wed, 21 Feb 2024, 22:47 Rootul P, @.***> wrote:

This repo is split. 188 occurrences of chunk and 300 occurrences of share. I still lean slightly towards share because usage in downstream repos but not strongly opinionated on it. Can do whichever reviewers prefer.

— Reply to this email directly, view it on GitHub https://github.com/celestiaorg/rsmt2d/pull/299#issuecomment-1958027894, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGOEBP3FTKU6GUANXC47SDYUZTNNAVCNFSM6AAAAABDTZA5W6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJYGAZDOOBZGQ . You are receiving this because you were mentioned.Message ID: @.***>

rootulp commented 9 months ago

celestia-node API uses share: https://node-rpc-docs.celestia.org/?version=v0.13.0#share. I don't see instances of chunk in those docs.

musalbas commented 9 months ago

If most apis and docs use share I'd guess it makes more sense to use share in the code

On Wed, 21 Feb 2024, 22:51 Rootul P, @.***> wrote:

celestia-node API uses share: https://node-rpc-docs.celestia.org/?version=v0.13.0#share. I don't see instances of chunk in those docs.

— Reply to this email directly, view it on GitHub https://github.com/celestiaorg/rsmt2d/pull/299#issuecomment-1958039862, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGOEBICMEB3UUX7SRH6UQ3YUZT5HAVCNFSM6AAAAABDTZA5W6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJYGAZTSOBWGI . You are receiving this because you were mentioned.Message ID: @.***>

rootulp commented 9 months ago

Agreed, can do.