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

feat: make changes to BlockStore trait based on feedback #286

Merged
merged 2 commits into from
Jun 23, 2023

Conversation

appcypher
Copy link
Member

@appcypher appcypher commented Jun 21, 2023

This pull request addresses the following suggestions and feedback made at ipfs thing. Todo item 3 has been implemented in PR #257.

We want to limit wnfs dependency on libipld. Changing the codec type from IpldCodec to u64 lets us replace libipld with libipld_core in wnfs and wnfs-wasm crates. In a way this also removes the limitation of libipld::IpldCodec enum.

We think bytes::Bytes is a much more efficient way of getting bytes to and from a BlockStore (for BlockStores implementation that care about that sort of thing). Previously we used Cow<Vec<u8>> and we have considered Read/Write but bytes::Bytes is just more flexible. This change basically makes block storage and retrieval more efficient and flexible while maintaining no_std compatibility.

This PR implements the following features

  • Replace IPLDCodec with u64.
  • Remove wnfs dependency on libipld.
  • Prefer bytes::Bytes to Vec<u8> in BlockStore interface.

Test plan (required)

  • Testing

    scripts/rs-wnfs test

Closing issues

Fixes #251

@appcypher appcypher requested a review from a team as a code owner June 21, 2023 22:51
@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #286 (fc2f35d) into main (3fb5392) will increase coverage by 0.06%.
The diff coverage is 14.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
+ Coverage   60.41%   60.47%   +0.06%     
==========================================
  Files          40       40              
  Lines        2766     2763       -3     
  Branches      687      690       +3     
==========================================
  Hits         1671     1671              
+ Misses        625      622       -3     
  Partials      470      470              
Impacted Files Coverage Δ
wnfs-common/src/utils.rs 75.86% <0.00%> (-5.62%) ⬇️
wnfs-hamt/src/diff.rs 57.01% <ø> (ø)
wnfs-hamt/src/hamt.rs 20.58% <ø> (ø)
wnfs-hamt/src/merge.rs 59.25% <ø> (ø)
wnfs-hamt/src/node.rs 55.03% <ø> (ø)
wnfs-hamt/src/pointer.rs 30.00% <ø> (ø)
wnfs/examples/private.rs 0.00% <0.00%> (ø)
wnfs/examples/privateref.rs 0.00% <0.00%> (ø)
wnfs/examples/public.rs 0.00% <0.00%> (ø)
wnfs/examples/tiered_blockstores.rs 0.00% <0.00%> (ø)
... and 16 more

@matheus23
Copy link
Member

I'd say this should be a feat PR, since it changes the signature in one of our exposed traits.

@appcypher appcypher changed the title fix(api) : replace IpldCodec with u64 fix(api): replace IpldCodec with u64 Jun 22, 2023
@appcypher appcypher changed the title fix(api): replace IpldCodec with u64 feat(api): replace IpldCodec with u64 Jun 22, 2023
@appcypher appcypher changed the title feat(api): replace IpldCodec with u64 feat: replace IpldCodec with u64 Jun 22, 2023
@appcypher appcypher changed the title feat: replace IpldCodec with u64 feat: make changes to BlockStore trait based on feedback Jun 22, 2023
Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

Nice. Love it.

@appcypher appcypher force-pushed the appcypher/refactor-blockstore branch from d24342e to fc2f35d Compare June 23, 2023 15:52
@appcypher appcypher merged commit 085242d into main Jun 23, 2023
@appcypher appcypher deleted the appcypher/refactor-blockstore branch June 23, 2023 16:03
@github-actions github-actions bot mentioned this pull request Jun 23, 2023
@github-actions github-actions bot mentioned this pull request Sep 1, 2023
@github-actions github-actions bot mentioned this pull request Sep 10, 2023
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.

Feedback refactor: BlockStore restructure
2 participants