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

Mistyped client call responses #266

Conversation

mattyg
Copy link
Member

@mattyg mattyg commented Jul 31, 2024

Failing test demonstrating #265

@mjbrisebois mjbrisebois requested a review from jost-s August 1, 2024 00:16
@mattyg
Copy link
Member Author

mattyg commented Aug 1, 2024

Heads up this is just a demonstration with one client call -- appInfo. The same issue needs to be resolved for all other client calls that take in hash types in their input type or have hash types in their responses.

For responses we have a simple solution: transform the response type and cast any Uint8Arrays representing hashes into a HoloHash instance.

For requests I don't know what the best solution is: do we want to require the input type to actually be HoloHash instances? Or do we want another set of types for when hashes are used in inputs, or type them all Uint8Array?

@mjbrisebois
Copy link
Contributor

I think all inputs should accept Uint8Array and then it can be transformed to the expected hash type like the example in utils/hash-parts.ts.

As for responses, I don't have a strong opinion. However, I would lean towards the client not automatically transforming the payloads.

@jost-s
Copy link
Contributor

jost-s commented Aug 1, 2024

I agree with both, inputs should accept Uint8Array and responses remain unaltered.
Perhaps inputs should actually accept both Uint8Array and HoloHash where applicable.

Would it be helpful to introduce another type which is Uint8Array | HoloHash? Actually I realize I need to discuss this in the office hours call next week. This change has wider implications than I saw.

@mattyg
Copy link
Member Author

mattyg commented Aug 1, 2024

In that case why dont we just revert to using the original types? We can still use the @spartan-hc/holo-hash utility functiom implementations under-the-hood

@jost-s
Copy link
Contributor

jost-s commented Aug 26, 2024

Reverted the spartan-hc changes.

@jost-s jost-s closed this Aug 26, 2024
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.

3 participants