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: Switch from Namefilter to Name Accumulators #247

Merged
merged 67 commits into from
Jul 7, 2023

Conversation

matheus23
Copy link
Member

Work on name accumulators (spec WIP: wnfs-wg/spec#65).

@matheus23 matheus23 self-assigned this May 2, 2023
@matheus23 matheus23 changed the title Matheus23/name accumulators Name Accumulators May 3, 2023
@matheus23 matheus23 changed the title Name Accumulators feat: Name Accumulators May 3, 2023
@matheus23 matheus23 changed the title feat: Name Accumulators feat: Switch from Namefilter to Name Accumulators Jul 6, 2023
@appcypher
Copy link
Member

appcypher commented Jul 6, 2023

This is a big and important change. Great work @matheus23! This should 🤞🏾 round up work on format stabilisation. The accumulator implementation looks good to me so I don't have much comment there. Just bits of nits here and there.

Now that we have *Forest::load/store helper functions, some lines like the one below need update:

let forest_cid = store.put_async_serializable(forest).await?;

Update to CryptoRngCore:

pub use rand_core::RngCore;

@matheus23 matheus23 mentioned this pull request Jul 6, 2023
6 tasks
@matheus23
Copy link
Member Author

@appcypher I decided to remove the hash algorithm generic type from the HamtForest. However, this means that we can no longer do tests with a MockHasher.
But I decided to go ahead and remove that test. It's quite a long test (hard to maintain), and I'd expect most of the code it covers would also be covered by the proptests in wnfs-hamt.

Copy link
Member

@appcypher appcypher left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@matheus23 matheus23 merged commit 7026a37 into main Jul 7, 2023
@matheus23 matheus23 deleted the matheus23/name-accumulators branch July 7, 2023 15:42
@github-actions github-actions bot mentioned this pull request Jul 7, 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.

2 participants