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

refactor: euint256 and code generation #414

Open
wants to merge 8 commits into
base: new-types
Choose a base branch
from

Conversation

PacificYield
Copy link
Contributor

@PacificYield PacificYield commented Mar 18, 2025

This PR:

  • refactors/simplifies the codegen processes for TFHE.sol and Impl.sol for greater control over operator/type matching
  • reduces the number of operators available for euint256
  • changes casting from cleartext for euintXX such as asEuint8(uint8 value) (instead of asEuint8(uint256 value))
  • fixes a few issues (e.g., missing ACL view functions for ebytesXX)
  • adds new types that may be implemented in the future (to match tfhe-rs)
  • separates the codegen/testgen processes
  • updates supportedTypes in TFHEExecutorNoEvents.sol to prevent security risks for deprecated types (euint4 for all operators and euint256 for several operators).

⚠️ ⚠️ ⚠️ If approved, it will be merged into the new-types (feature) branch.

There are a few TODOs that need to be addressed in future PRs (autogenerate tests for ebytesXX, adding underflow/overflow tests, adding tests for ACL view functions + potentially for isInitialized)

@PacificYield PacificYield self-assigned this Mar 18, 2025
@cla-bot cla-bot bot added the cla-signed label Mar 18, 2025
@PacificYield PacificYield force-pushed the refactor-euint256 branch 6 times, most recently from d45a3d7 to 3fe07cb Compare March 21, 2025 15:47
@PacificYield PacificYield changed the title refactor: euint256 and code generation (WIP) refactor: euint256 and code generation Mar 21, 2025
@PacificYield PacificYield force-pushed the refactor-euint256 branch 7 times, most recently from 3d20ed1 to cdd8b1c Compare March 26, 2025 15:33
@PacificYield PacificYield marked this pull request as ready for review March 27, 2025 08:26
@PacificYield PacificYield requested a review from a team as a code owner March 27, 2025 08:26
@jatZama
Copy link
Member

jatZama commented Mar 27, 2025

You forgot to update TFHEExecutorNoEvents since you said partially deprecates euint256 you should start by this for security. As a reminder, anyone could use their own TFHE.sol library, source of truth for reverts on invalid operation is the TFHEExecutor contract. Ping me when you implemented this major change.

@jatZama
Copy link
Member

jatZama commented Mar 27, 2025

For instance, as of now, there is a security issue since you forgot to remove the (1 << 8) term from this sum: https://github.com/zama-ai/httpz-backend/blob/main/contracts/contracts/TFHEExecutorNoEvents.sol#L153
Same for all other operators which are deprecated for euint256, etc.

@PacificYield PacificYield force-pushed the refactor-euint256 branch 2 times, most recently from ebe1481 to 5d21648 Compare March 27, 2025 13:33
*/
export const SUPPORTED_BITS: number[] = [8, 16, 32, 64, 128, 256];
export function validateFHETypes(fheTypes: FheType[]): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this function useful?

@PacificYield PacificYield force-pushed the refactor-euint256 branch 4 times, most recently from bf7ed04 to 61dfb10 Compare March 31, 2025 07:28
{
type: 'Euint160',
value: 7,
supportedOperators: [],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused why you didn't put the supportedOperators here and put it inside in the alias (Eaddress)? this structure seems confusing.

@jatZama
Copy link
Member

jatZama commented Mar 31, 2025

The FHEGas tracking utility function is broken since the FHEGas Consumed in transfer is NaN. I would like you to please modify the tests in the EncryptedERC20.FHEGas.ts with 2 additional asserts on the real value expected for the FHEGas consumed both in a transfer and transferFrom, this way we would be able to detect more easily this kind of regression in future updates.

The biggest flaw I think currently, is the types.ts new file which is confusing, for instance, I would only put the native types there from tfhe-rs in the FheTypes first struct, with all the supported operators, and with same names as in tfhe-rs, see https://github.com/zama-ai/tfhe-rs/blob/1e588034320553bd79dbb5e9959ae001d070a6db/tfhe/src/high_level_api/mod.rs#L162 meaning, Euint160 should be renamed Uint160 to match what is inside tfhe-rs and contain the supported operators instead of an empty array currently (dunno why those where transferred only to the alias type which should not be even here in the first place, but potentially in another struct specific for the aliases). In my opinion you should also rename the field "type" to "tfhe-rs-type" (or maybe"native-type") to be less confusing.
Then, you could put all aliases in a second struct FheAliases which would just map native tfhe-rs types to aliases, i.e here you could define ebool as an alias for Bool, and euint8 as an alias of the native Uint8, as well as eaddress as an alias for the native Uint160, etc. In my mind, FheAliases could be exactly in sync with the Solidity custom types, for increased readability, maybe this struct could be renamed to SolidityCustomTypes or SolidityCustomAliases even. Those aliases could also contain another separate field supported operators, but this one will be used only for the front-end (i.e TFHE.sol), meaning for eg, even if euint160 and eaddress are truly indeed equivalent from the point of vue of TFHEExecutor.sol contract, maybe in the future, we would like to expose different TFHE.sol methods for those different Solidity aliases.

In summary for types.ts : the backend related code (i.e TFHEExecutor, FHEGasLimit contracts, etc) should be using the native types, while the front-end library (TFHE.sol) should be based on the solidity aliases. I hope this makes sense.

Remember: this makes sense, because we decided that "equivalent" types such as ebytes20, eaddress and euint160 would be 100% equivalent for the backend (for eg inside TFHEExecutor), ie in theory we could make a legal operation between an euint160 and an eaddress at some point, without reverting, and for decryption in the callback, in the far future we will let the dapp developer handle the reverted endianness of potential ebytesXX types via a specific utility function (see this issue #409)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants