privacy-scaling-explorations / zk-kit

A monorepo of reusable libraries for zero-knowledge technologies.
https://zkkit.pse.dev/
MIT License
256 stars 53 forks source link

TS `insert()` and `update()` doesn't check if the new leaf exists already #243

Closed 0xbok closed 1 month ago

0xbok commented 1 month ago

In Solidity version of LeanIMT, existing leaves cannot be inserted or updated, but TS version allows it.

https://github.com/privacy-scaling-explorations/zk-kit/blob/950dc5bd5f8f53069c12b56581ed63faa5382515/packages/imt/src/lean-imt/lean-imt.ts#L116

https://github.com/privacy-scaling-explorations/zk-kit/blob/950dc5bd5f8f53069c12b56581ed63faa5382515/packages/imt/src/lean-imt/lean-imt.ts#L156

https://github.com/privacy-scaling-explorations/zk-kit/blob/950dc5bd5f8f53069c12b56581ed63faa5382515/packages/imt/src/lean-imt/lean-imt.ts#L199

https://github.com/privacy-scaling-explorations/zk-kit/blob/950dc5bd5f8f53069c12b56581ed63faa5382515/packages/imt.sol/contracts/internal/InternalLeanIMT.sol#L40-L46

aakansha1234 commented 1 month ago

Is it fine if I take up this issue?

cedoor commented 1 month ago

Is it fine if I take up this issue?

Absolutely!

aakansha1234 commented 1 month ago

I have added the check in insert and insertMany: https://github.com/aakansha1234/zk-kit/commit/9fb3be8da30508cf0c2f663c99a94724f8f2bc2a

I have a question for update function - If we are doing a duplicate check on new leaf in update function, we will not be able to delete leaves as 0 will already be in existing leaves. How to resolve this?

cedoor commented 1 month ago

Hey @aakansha1234, I'd keep the update method without any check here. Sounds like a check that the extensions of this class need to add (e.g. Group).

@0xbok What do you think?

cedoor commented 1 month ago

After discussing this with @0xbok, we decided to not check anything in this TS implementation.

Documentation needs to be added to the Solidity LeanIMT library tho, in order to make it clear that it is different from the TS one.