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

Typesafe library #99

Closed
wants to merge 15 commits into from
Closed

Typesafe library #99

wants to merge 15 commits into from

Conversation

dapplion
Copy link
Contributor

@dapplion dapplion commented May 1, 2021

  • Use same Typescript and lint options as Lodestar monorepo
  • Use same Typescript version as Lodestar monorepo

Those two changes have surfaced many issues which I've resolved as many as possiible. @wemeetagain I don't know how to handle the function calls that you do with null such as

if (!this.hasVariableSerializedLength() && length !== this.struct_getSerializedLength(null)) {

please take a look and find a solution accepted by the Typescript compiler.


  1. This PR also enforces correct types for ContainerType fields and ArrayType element type. See this test as an example of what compiles and what doesn't now
    https://github.com/ChainSafe/ssz/blob/36b30f8916/test/unit/eth2Types.test.ts

  2. Replaced the toHexString and fromHexString for simpler alternatives that are

  • toHexString: x12 faster (now 0.4 µs / op, 32 bytes input)
  • fromHexString: x2 faster (now 2.4 µs / op, 32 bytes input)

@dapplion dapplion changed the title Typesafe composite types Typesafe library May 1, 2021
@wemeetagain
Copy link
Member

struct_getSerializedLength is called with null in cases where the type is "fixed-length" and not dependent on the value.
I think @3xtr4t3rr3str14l ran into this same issue in #96

The issue is that the signature is different between fixed-length and variable-length types.
One naive approach could be to change the signature to struct_getSerializedLength(value?: T): number.
And just throw an error at runtime if the value isn't provided for variable-length types.

A different approach could be to cast null/undefined to T in the fixed-length type case.

@wemeetagain
Copy link
Member

change the signature to struct_getSerializedLength(value?: T): number.

Seems we already did similar here in #110 https://github.com/ChainSafe/ssz/pull/110/files#diff-f7dbd702062a823eb1050e5721c8802d7d613821a719d69da838bc0760cef64bR594-R596

Pushed a few commits on top to fix conflicts and type errors

@dapplion
Copy link
Contributor Author

conflicts

@dapplion
Copy link
Contributor Author

@wemeetagain Can you take a look?

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

🚀 LGTM, we should link against lodestar and ensure there's no critical errors

@dapplion dapplion mentioned this pull request Aug 17, 2021
@dapplion
Copy link
Contributor Author

PR got fucked with conflicts 😅 @wemeetagain do you think it's worth it to pursue this?

@dapplion
Copy link
Contributor Author

Implemented with #223

@dapplion dapplion closed this Apr 13, 2022
@dapplion dapplion deleted the dapplion/type-container branch April 13, 2022 09:42
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

3 participants