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

Add function 'check_prime' for node's crypto API #82

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Aviii06
Copy link

@Aviii06 Aviii06 commented Aug 29, 2022

This implements one function of node's crypto API.
This is to complete the assignment given for lfx

@juntao
Copy link
Member

juntao commented Apr 5, 2023

flows summarize

Copy link
Member

juntao commented Apr 5, 2023

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.


Commit 1

This patch introduces a new function 'check_prime' to the node's crypto API. Here's a summary of the key changes:

  1. Add 'check_prime' function in 'src/internal_module/crypto.rs'.
    • The function checks if a given integer is prime or not and returns a boolean value. It takes an integer parameter and throws a type error if the parameter is not an integer.
  2. Import and use the new 'check_prime' function in 'example_js/node/main.mjs'.
  3. Add necessary Rollup configurations and dependencies to 'example_js/node/package.json'.
  4. Update the module initialization in 'src/quickjs_sys/mod.rs' to include the new crypto module.
  5. Modify 'src/internal_module/mod.rs' to include the crypto module.
  6. A small change in 'src/internal_module/os.rs' just adds a newline at the end of the file.

Potential problems:

  1. The check_prime function can be optimized further, as it checks for divisibility with even numbers after 2, which isn't necessary.
  2. There's no error handling in case the user tries to run 'check_prime' with a very large number, which could lead to performance issues.
  3. The added dependencies might lead to conflicts, so it's essential to test various use cases with the new function to ensure there aren't any unexpected dependency conflicts in the long run.

@alabulei1
Copy link
Member

Hi @L-jasmine

I think this PR is to apply for the LFX mentorship. If we're not going to merge this PR, please close this PR. Thanks.

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.

None yet

3 participants