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: fr32-sha2-256-trunc254-padded-binary-tree #19

Merged
merged 10 commits into from
Jul 19, 2023
Merged

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Jul 19, 2023

No description provided.

Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

✨ the inline comments here are great to be able to go through what is happening! this is great

const root = parseLink(data.out.cid).multihash.digest
const height = Piece.PaddedSize.toHeight(BigInt(data.out.paddedSize))

{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are using brackets here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just creating scopes so I don't have to come up with three different names for digest. It also kind of provides some separation, but if this is confusing I can remove them and call digest1, digest2.

I might actually generate separate test functions instead.

src/multihash.js Outdated
export const MAX_HEIGHT = 255

/**
* Given that max size of the
Copy link
Contributor

Choose a reason for hiding this comment

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

can we complete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops not sure what happened here

src/multihash.js Outdated Show resolved Hide resolved
Comment on lines +157 to +160
if (asMultihash) {
output.set(PREFIX, byteOffset)
byteOffset += PREFIX.length
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would have a new function to do so. Thoughts?
Otherwise, the byteOffset being optional is also tricky as it is required to use third param. Perhaps having an object for options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean have a separate function that encodes multihash prefix ? I mostly wanted to align it with StreamingHasher interface implemented by blake3-multihash

That said you are right interface is bit clunky. I have created multiformats/js-multiformats#260 so we could uplift the incremental hasher into multiformats. How about we iterate on the interface there and then we can reflect consensus here in a followup.

@Gozala Gozala merged commit 0c7b499 into main Jul 19, 2023
vasco-santos pushed a commit that referenced this pull request Jul 25, 2023
🤖 I have created a release *beep* *boop*
---


##
[2.2.0](v2.1.0...v2.2.0)
(2023-07-21)


### Features

* fr32-sha2-256-trunc254-padded-binary-tree
([412bf18](412bf18))
* fr32-sha2-256-trunc254-padded-binary-tree
([#19](#19))
([0c7b499](0c7b499))
* padded size from height calc function
([dba6cd5](dba6cd5))
* padded size from height calc function
([#18](#18))
([47971cd](47971cd))


### Bug Fixes

* export interface for data structure with link and height
([263656e](263656e))
* export interface for data structure with link and height
([#17](#17))
([a1006c0](a1006c0))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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