ethers-io / ethers.js

Complete Ethereum library and wallet implementation in JavaScript.
https://ethers.org/
MIT License
7.98k stars 1.86k forks source link

Latest `kzg-wasm` Release `0.5.0` Breaks `ethers` API #4841

Open pcaversaccio opened 1 month ago

pcaversaccio commented 1 month ago

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: https://github.com/ethereumjs/kzg-wasm/pull/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

ricmoo commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

@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: https://github.com/ethers-io/ethers.js/commit/e5036e778624fea957dd847f7be1a9f148b8997e#commitcomment-147131361. The types are not happy yet :)

pcaversaccio commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Sure, that should be doable. :)

pcaversaccio commented 1 month ago

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

pcaversaccio commented 1 month ago

Sure, that should be doable. :)

any update here :D?

paulmillr commented 1 month ago

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

It is available in the following import.

ricmoo commented 1 month ago

@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 commented 1 month ago

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