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

chore: upgrade Hyperdrive to 11.5.3, fix blob-store tests, fix hyperdrive blob namespace #151

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

sethvincent
Copy link
Contributor

This pr upgrades the hyperdrive package to latest and fixes a couple small errors:

Closes #147

Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

one comment that would be helpful to get some clarification on, but otherwise lgtm

@@ -266,7 +266,7 @@ class PretendCorestore {
)
} else if (opts.name === 'db') {
return this.#coreManager.getWriterCore('blobIndex').core
} else if (opts.name === 'blobs') {
} else if (opts.name.includes('blobs')) {
Copy link
Member

Choose a reason for hiding this comment

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

this seems like a reasonable change but not sure how this works in practice. wanted to highlight in order to get clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The blobs core in a hyperdrive is now indexed with ${this.db.core.id}/blobs rather than just blobs:

https://github.com/holepunchto/hyperdrive/pull/355/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346L160-R160

We don't know the id of that core at this stage so we check to see if blobs is in the core name.

It feels a little imprecise to use includes there, but if we somehow have other cores with blobs in their name we likely have other problems, too.

Copy link
Member

Choose a reason for hiding this comment

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

We don't actually name any cores, so this will be fine. It's just a way of making sure we return the right core when hyperdrive asks for it. For existing hyperdrives it always reads the blob core key from the blob index anyway. In an ideal world the hyperdrive constructor would accept the blob core and the hyperbee core, but this is a necessary workaround for now.

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Can you create a follow up issue to resolve the todo comments in the code around wait options?

@sethvincent sethvincent merged commit ea36782 into main Aug 8, 2023
6 checks passed
@sethvincent sethvincent deleted the upgrade-hyperdrive-11.5.3 branch August 9, 2023 22:44
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.

Update to [email protected]
3 participants