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

Use @spartan-hc/holo-hash for managing holo-hashes #259

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

mjbrisebois
Copy link
Contributor

Here is a quick example of how we could start using a library to manage holo-hashes while preserving legacy support.

Most of the helper functions in utils could be replaced by the native methods on HoloHash instances.

@mjbrisebois mjbrisebois requested a review from jost-s June 27, 2024 21:55
@mjbrisebois
Copy link
Contributor Author

@jost-s do you see value in this?

Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

Yes, I do see value in this. Looks good to me

}

export * from "@spartan-hc/holo-hash";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This causes ambiguity because the HoloHash classes have the same name as all the types.ts exports. How should we export the class implementations without conflict?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should ship this client with only one type for each type. So I wouldn't want to export a legacy type and the new type under an alias. Therefore I'd replace the existing type with the spartan-hc one.

What do you think about this, @ThetaSinner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supporting both HoloHash and Uint8Array input for most functions is important so that users are not forced to wrap all input in new HoloHash( input ). But I don't know enough about typescript to do that. Would it work to extend all the types with an "or-pipe"?

Example

export type AgentPubKey = Uint8Array | HoloHashLib.AgentPubKey;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, that doesn't solve the duplicate export issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to let you know this is still on my radar.

* @returns The signing key pair and an agent pub key based on the public key.
*
* @public
*/
export const generateSigningKeyPair: (
agentPubKey?: AgentPubKey
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the purpose of forcing the same location bytes? Imposing location bytes on the end of random bytes makes the DHT location check fail.

@@ -1,77 +1,77 @@
import { range } from "lodash-es";
import { randomByteArray } from "../api/zome-call-signing.js";
import { DnaHash, ActionHash, AgentPubKey, EntryHash } from "../types.js";
import { dhtLocationFrom32 } from "./hash-parts.js";

async function fakeValidHash<T extends Uint8Array>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this function is not exported, it is safe to assume it was only used by this file. Correct?

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.

None yet

2 participants