fix: Various Typescript type checker errors #583
Conversation
Also improved a few bits of config Signed-off-by: Silas Davis <[email protected]>
| @@ -149,7 +149,7 @@ contract ACL is UUPSUpgradeable, Ownable2StepUpgradeable { | |||
| emit AllowedForDecryption(handlesList); | |||
| } | |||
|
|
|||
| function isAllowedForDecryption(uint256 handle) public virtual returns (bool) { | |||
| function isAllowedForDecryption(uint256 handle) public view virtual returns (bool) { | |||
There was a problem hiding this comment.
This needs to be checked, but the tests certainly assume this is a view function and it doesn't seem very useful if it's not.
There was a problem hiding this comment.
good catch, will put this one in a separate commit
| @@ -99,9 +107,9 @@ const getAlreadyFulfilledDecryptions = async (): Promise<[bigint]> => { | |||
| topics: eventDecryptionResult, | |||
| }; | |||
| const pastResults = await ethers.provider.getLogs(filterDecryptionResult); | |||
| results = results.concat(pastResults.map((result) => ifaceResultCallback.parseLog(result).args[0])); | |||
There was a problem hiding this comment.
past results has type Log, whereas results nominally has a type of requestIds so this looks like a logical error from what is intended.
In the new version we merge a type-coherent list of (null | LogDescription) then later filter down and pull out .args[0]
| @@ -118,8 +126,12 @@ const fulfillAllPastRequestsIds = async (mocked: boolean) => { | |||
| const pastRequests = await ethers.provider.getLogs(filterDecryption); | |||
| for (const request of pastRequests) { | |||
| const event = ifaceEventDecryption.parseLog(request); | |||
There was a problem hiding this comment.
This can be rewritten with strongly typed event via typechain, just need to dig that up.
poppyseedDev
requested review from
jatZama and
leventdem
and removed request for
jatZama
|
Thank you for your work. But since many of those files will be refactored and moved by the end of this week in another repo, I will for the moment just take into account your remark about the missing |
|
Could you give an idea of the destination structure, will this refactor/new repo replace Will you still be using hardhat/ethers or some other framework for deployment? |
fhevm will mainly contain the TFHE.sol library (and GatewayContract and library for now). The core contracts (ACL, TFHEExecutor, KMSVerifier, InputVerifier, FHEPayment) will be moved to another repo and imported inside fhevm. Deployment will still use hardhat, but we plan to add a Foundry template soon. |
Got it, I'll leave this open but feel free to close when appropriate for you guys. |
|
|
I am being mentioned by Inco there are typechecker errors. Should I open a new issue for that @immortal-tofu ? |
|
Yes definitely, we should increase the quality of code of our repositories. EDIT: #649 |
Also improved a few bits of config.
This is a WIP since I was passing by anyway, all of the type checker issue can be eliminated with a bit more effort. I think a lot of what remains can be solved by making use of the Typechain types.
Let me know if this effort is likely to get merged and I might try and finish it off (although I may need to defer on some tests where I'm unsure if I may end up making logical changes inadvertently).