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

Feat/new types #296

Closed
wants to merge 12 commits into from
Closed

Feat/new types #296

wants to merge 12 commits into from

Conversation

immortal-tofu
Copy link
Collaborator

@immortal-tofu immortal-tofu commented Feb 28, 2024

  • Use native ebool from fhEVM instead of euint8
  • Implement euint4 type and operations

chore:

  • Implement overload generator to create test with new types

@cla-bot cla-bot bot added the cla-signed label Feb 28, 2024
@immortal-tofu immortal-tofu marked this pull request as ready for review February 28, 2024 09:49
codegen/overloadTests.ts Outdated Show resolved Hide resolved
export const SUPPORTED_FUNCTIONS: SupportedFunctions = {
add: {
supportedBits: SUPPORTED_BITS,
safeMin: true,
Copy link
Member

Choose a reason for hiding this comment

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

not sure to understand why you would need the safeMin to be true here, if I understand this would mean that you force the output to be of type the lowest bitwidth of inputs, but this is not needed here, because you can indeed sum a euint8 with a euint64 and result would be a euint64

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, but the codegen expects a uint8 result, otherwise it fails. I think this is a mistake in the testgen.ts but I don't want to touch it now.

Copy link
Member

Choose a reason for hiding this comment

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

weird

encryptedTests.push(safeEval(test.evalTest, smallest - 4, smallest, lhs, rhs, test.safeMin));
}
encryptedTests.push(safeEval(test.evalTest, smallest, smallest, lhs, rhs, test.safeMin));
encryptedTests.push(safeEval(test.evalTest, smallest, smallest - 4, lhs, rhs, test.safeMin));
Copy link
Member

Choose a reason for hiding this comment

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

just noticed you were using safeEval for every test you pushed, but shouldnt we also test the wrapping behavior works correctly when overflowing? so also NOT using safeEval sometimes would make sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Overflowing check is already done on tfhe-rs lib, I don't think we need to go into this type of details.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this is not so important indeed, but sometimes in rare cases we actually need to use the wrapping behavior , like what is done in the Uniswap V2 contract for the price oracle (only example I know of to be honest where overflow is desired) https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L76

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's test in the contract in this case.

@jatZama jatZama self-requested a review March 1, 2024 11:44
codegen/overloads.json Outdated Show resolved Hide resolved
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