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

Fhevmjs fixes to work with local node #70

Merged
merged 5 commits into from
Jun 17, 2024
Merged

Conversation

david-zk
Copy link
Contributor

Tested with coprocessor, we need following adjustments:

  1. types byte array must be made of one byte numbers, not more than 255, bit values like 2048 are too high, added encoding for fhevmjs type to node type enum
  2. added 0x in front of input hex string to be sent as input payload

Added draft test with getting public key and encoding inputs, I think we might need to setup running container of the service to test the integration.

@immortal-tofu @jatZama

Copy link

cla-bot bot commented Jun 17, 2024

Thank you for your pull request. We require contributors to sign our Contributor License Agreement / Terms and Conditions, and we don't seem to have the users @david-zk on file. In order for us to review and merge your code, please sign:

  • For individual contribution: our CLA
  • for Bounty submission, if you are an individual: our T&C
  • for Bounty submission, if you are a company: our T&C
    to get yourself added.

If you already signed one of this document, just wait to be added to the bot config.

src/sdk/encrypt.ts Outdated Show resolved Hide resolved
Copy link

cla-bot bot commented Jun 17, 2024

Thank you for your pull request. We require contributors to sign our Contributor License Agreement / Terms and Conditions, and we don't seem to have the users @david-zk on file. In order for us to review and merge your code, please sign:

  • For individual contribution: our CLA
  • for Bounty submission, if you are an individual: our T&C
  • for Bounty submission, if you are a company: our T&C
    to get yourself added.

If you already signed one of this document, just wait to be added to the bot config.

data.set(ciphertext, data.length + 1);
data.set([bits.length], 0);
bits.forEach((value, index) => {
data.set([ENCRYPTION_TYPES[value] & 0xff], 1 + index);
Copy link

Choose a reason for hiding this comment

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

Why is the "& 0xff" needed here?

Copy link

Choose a reason for hiding this comment

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

ENCRYPTION_TYPES[value] should never exceed 255 from definition already

Copy link
Contributor Author

@david-zk david-zk Jun 17, 2024

Choose a reason for hiding this comment

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

The value in that map is of type number which has a big range. I'm not sure how to best approach this, ideally this should be a compile time error, if someone adds a larger number things will "work" but write bad values.

Copy link

cla-bot bot commented Jun 17, 2024

Thank you for your pull request. We require contributors to sign our Contributor License Agreement / Terms and Conditions, and we don't seem to have the users @david-zk on file. In order for us to review and merge your code, please sign:

  • For individual contribution: our CLA
  • for Bounty submission, if you are an individual: our T&C
  • for Bounty submission, if you are a company: our T&C
    to get yourself added.

If you already signed one of this document, just wait to be added to the bot config.

@immortal-tofu
Copy link
Collaborator

@david-zk when I said comment tests it was to make the CI pass :D Sorry if I wasn't clear enough :)

Copy link

cla-bot bot commented Jun 17, 2024

Thank you for your pull request. We require contributors to sign our Contributor License Agreement / Terms and Conditions, and we don't seem to have the users @david-zk on file. In order for us to review and merge your code, please sign:

  • For individual contribution: our CLA
  • for Bounty submission, if you are an individual: our T&C
  • for Bounty submission, if you are a company: our T&C
    to get yourself added.

If you already signed one of this document, just wait to be added to the bot config.

Copy link

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
87.28% (+0.35% 🔼)
247/283
🟢 Branches
80.88% (-1.21% 🔻)
55/68
🟢 Functions
87.93% (-1.54% 🔻)
51/58
🟢 Lines
88.98% (+0.05% 🔼)
218/245
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢 sdk/encrypt.ts
85.45% (+1.53% 🔼)
80%
90.91% (-4.33% 🔻)
88.46% (+0.84% 🔼)
🟢 ethCall.ts
86.67% (-2.62% 🔻)
57.14% (-9.52% 🔻)
100%
83.33% (-3.03% 🔻)

Test suite run success

26 tests passing in 6 suites.

Report generated by 🧪jest coverage report action from 0213255

@immortal-tofu immortal-tofu merged commit fc7c6e1 into main Jun 17, 2024
4 of 5 checks passed
@immortal-tofu immortal-tofu deleted the fhevmjs-coproc-fixes branch December 5, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants