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

Latest kzg-wasm Release 0.5.0 Breaks ethers API #4841

Open
pcaversaccio opened this issue Sep 21, 2024 · 14 comments
Open

Latest kzg-wasm Release 0.5.0 Breaks ethers API #4841

pcaversaccio opened this issue Sep 21, 2024 · 14 comments
Assignees
Labels
investigate Under investigation and may be a bug. v6 Issues regarding v6

Comments

@pcaversaccio
Copy link

Ethers Version

6.13.2

Search Terms

kzg-wasm

Describe the Problem

kzg-wasm released their latest 0.5.0 version with the major changeset coming from this PR: ethereumjs/kzg-wasm#17. You can see how the renamed e.g. blobToKzgCommitmentWasm to blobToKZGCommitmentWasm and thus if you do something like that tx.kzg = kzg;, there will be a TypeScript compilation error:

return new TSError(diagnosticText, diagnosticCodes, diagnostics);
           ^
TSError: ⨯ Unable to compile TypeScript:
scripts/sign-eip4844.ts:66:5 - error TS2739: Type '{ loadTrustedSetup: (trustedSetup?: TrustedSetup | undefined) => number; freeTrustedSetup: () => void; blobToKZGCommitment: (blob: string) => string; computeBlobKZGProof: (blob: string, commitment: string) => string; verifyBlobKZGProofBatch: (blobs: string[], commitments: string[], proofs: string[]) => boolean; veri...' is missing the following properties from type 'KzgLibrary': blobToKzgCommitment, computeBlobKzgProof

66     tx.kzg = kzg;

Code Snippet

You can use my repo here: https://github.com/pcaversaccio/raw-tx

1. `pnpm install`
2. `pnpm upgrade --latest` (will upgrade `kzg-wasm` to version `0.5.0`)
3. `pnpm generate:eip4844`

Contract ABI

No response

Errors

No response

Environment

No response

Environment (Other)

No response

@pcaversaccio pcaversaccio added investigate Under investigation and may be a bug. v6 Issues regarding v6 labels Sep 21, 2024
@pcaversaccio pcaversaccio changed the title Latest kzg-wasm Release 0.5.0 Breaks ethers Latest kzg-wasm Release 0.5.0 Breaks ethers API Sep 21, 2024
@ricmoo
Copy link
Member

ricmoo commented Sep 21, 2024

Aiya. I will look into what makes the most sense to do; I suppose just allow that field to be either and pick the one at runtime that exists?

@pcaversaccio
Copy link
Author

Aiya. I will look into what makes the most sense to do; I suppose just allow that field to be either and pick the one at runtime that exists?

Sounds reasonable to me.

@ricmoo
Copy link
Member

ricmoo commented Sep 23, 2024

@pcaversaccio

Can you please check out the above commit? I've made changes that should fix it (in a backwards compatible way), but it was far more involved than expected. I've tested on 0.3.1 and 0.5.0, but would love extra eyes on it as it changes some signatures and would also love for you to try it out in your TypeScript environment to make sure the types are all happy.

@pcaversaccio
Copy link
Author

@pcaversaccio

Can you please check out the above commit? I've made changes that should fix it (in a backwards compatible way), but it was far more involved than expected. I've tested on 0.3.1 and 0.5.0, but would love extra eyes on it as it changes some signatures and would also love for you to try it out in your TypeScript environment to make sure the types are all happy.

Left a review here: e5036e7#commitcomment-147131361. The types are not happy yet :)

@pcaversaccio
Copy link
Author

@pcaversaccio
Can you please check out the above commit? I've made changes that should fix it (in a backwards compatible way), but it was far more involved than expected. I've tested on 0.3.1 and 0.5.0, but would love extra eyes on it as it changes some signatures and would also love for you to try it out in your TypeScript environment to make sure the types are all happy.

Left a review here: e5036e7#commitcomment-147131361. The types are not happy yet :)

Somehow my GitHub UI is broken and I can't reply in the commit so will do here. So there is still the TypeScript compilation error:

image

this is the line: https://github.com/pcaversaccio/raw-tx/blob/main/scripts/sign-eip4844.ts#L66

The underlying type issue is due to: Transaction.kzg: ethers.KzgLibrary | null

image

@pcaversaccio
Copy link
Author

I'm wondering if I'm testing it correctly. Installing locally via pnpm can be done via pnpm add ethers@github:ethers-io/ethers.js#wip-v6.14 correct? (Sorry I'm usually a Python guy...)

@ricmoo
Copy link
Member

ricmoo commented Sep 25, 2024

It looks like maybe it didn’t pick up the change?

I’ve never used pnpm (I only use npm), but I would expect that behaves the same.

I think I know the issue; the dist and lib files don’t get built on branches, so while the Ethers TypeScript source is updated, the generated JavaScript is still the old code.

If you clone the library, change to that branch and then npm install && npm run build it should do all that for you and then you can test locally. But I can also build a throw-away branch wip-v6.14-build for you’d when I get home.

@pcaversaccio
Copy link
Author

Can you maybe do a release candidate like v6.14.0-alpha.1 so I can simply install it from the npm registry? That would be easiest for me tbh

@ricmoo
Copy link
Member

ricmoo commented Sep 25, 2024

Sure, that should be doable. :)

@pcaversaccio
Copy link
Author

Awesome - so ping me here after the release and I will test again!

@pcaversaccio
Copy link
Author

Sure, that should be doable. :)

any update here :D?

@paulmillr
Copy link

I have made pure JS KZG, which is now used in ethereumjs by default.

It is available in the following import.

@ricmoo
Copy link
Member

ricmoo commented Oct 17, 2024

@paulmillr thanks! I’m adding a page in the documents to show how to use ethers with your pure-JS implementation too. I’ve also heard it is substantially faster than the WASM version. :)

@ricmoo
Copy link
Member

ricmoo commented Oct 17, 2024

(and am adding support along with the other change to accept the API function names you use)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Under investigation and may be a bug. v6 Issues regarding v6
Projects
None yet
Development

No branches or pull requests

3 participants