-
Notifications
You must be signed in to change notification settings - Fork 0
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
refactor: TFHEExecutor #141
Conversation
790dc79
to
da2f9fa
Compare
I think we shouldn't commit types folder |
Agreed. I wanted to remain consistent. Should we remove the types altogether instead? |
I think It has been commited by error, so yeah you can add it to gitignore |
da2f9fa
to
1bbdb04
Compare
1bbdb04
to
cfb5bb0
Compare
I removed all. |
@@ -2,6 +2,7 @@ | |||
|
|||
pragma solidity ^0.8.24; | |||
|
|||
import "@openzeppelin/contracts/utils/Strings.sol"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add this import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it is my mistake. I wanted to restrict the scope of the import but I didn't finish. I adjusted to be as such:
import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
import {TFHEExecutor} from "../contracts/TFHEExecutor.sol";
cfb5bb0
to
fea3f8c
Compare
@@ -286,7 +285,7 @@ contract TFHEExecutor is UUPSUpgradeable, Ownable2StepUpgradeable { | |||
result = binaryOp(Operators.fheRotl, lhs, rhs, scalar, lhsType); | |||
} | |||
|
|||
function fheRotr(uint256 lhs, uint256 rhs, bytes1 scalarByte) external returns (uint256 result) { | |||
function fheRotr(uint256 lhs, uint256 rhs, bytes1 scalarByte) public virtual returns (uint256 result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this one didn't have the virtual
fea3f8c
to
a0f4b42
Compare
a0f4b42
to
004d9fd
Compare
4fa88d5
to
d9fbba9
Compare
pragma solidity ^0.8.24; | ||
|
||
import "./ACL.sol"; | ||
import "./FHEPayment.sol"; | ||
import "../addresses/ACLAddress.sol"; | ||
import "../addresses/FHEPaymentAddress.sol"; | ||
import "../addresses/InputVerifierAddress.sol"; | ||
import "@openzeppelin/contracts/utils/Strings.sol"; | ||
import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this kind of import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to be strict for imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it sums up quite well the benefits.
https://ethereum.stackexchange.com/questions/117100/why-do-many-solidity-projects-prefer-importing-specific-names-over-whole-modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance, the UUPSUpgradeable
was imported from the import of ACL before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
This PR removes the codegen for
TFHEExecutor.sol
andTFHEExecutor.events.sol
; it replaces the codegen logic through inheritance and the use ofsuper()
.All files in
types/
that are auto-generated are removed.