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

build: rebuilt wasm tutorial with next.js #5

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

Conversation

ethan-crypto
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@currenthandle currenthandle left a comment

Choose a reason for hiding this comment

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

Looks good dude. Left you some comment of a that will make this more maintainable

<div className='App'>
<GenPK
files={{
model: files['model_ser_pk'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should be able to access this the normal way if you feel it's more readable files.model_ser_pk

id='genVkButton'
onClick={async () => {
if (Object.values(files).every((file) => file instanceof File)) {
const result = await handleGenVkButton(
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is result? a vk? maybe a more descriptive name

<FileDownload
fileName='vk.key'
buffer={buffer}
handleDownloadCompleted={function (): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

arrow function probably cleaner here. Usually better not to add explicit return types and let TS infer it for you.

onClick={async () => {
if (Object.values(files).every((file) => file instanceof File)) {
const result = await handleGenVkButton(
files as { [key: string]: File },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually want to avoid type casting with as

<button
id='genHashButton'
onClick={async () => {
const result = await handleGenHashButton(message as File) // 'as' cast should be safe b/c of disabled button
Copy link
Collaborator

Choose a reason for hiding this comment

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

message: File | null correct? instead of doing message as File. One line above do

if (!message) {
  return 
}

then typescript will infer this for you and it will safe at runtime

app/Verify.tsx Show resolved Hide resolved
Comment on lines +40 to +49
filesNames.forEach((fileName) => {
fetch(`/data/${fileName}`)
.then((res) => res.blob())
.then((blob) =>
setFiles((prevFiles) => ({
...prevFiles,
[fileName]: new File([blob], fileName),
})),
)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're already in an async function. Would be more readable to use await here

}, [])

useEffect(() => {
if (files['test.onnx'] && files['kzg'] && files['settings.json']) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

helper function

Comment on lines +99 to +103
files['test.witness.json'] &&
files['test.provekey'] &&
files['test.onnx'] &&
files['settings.json'] &&
files['kzg']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Def could use a helper function

Comment on lines +129 to +132
files['test.proof'] &&
files['test.key'] &&
files['settings.json'] &&
files['kzg']
Copy link
Collaborator

Choose a reason for hiding this comment

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

helper function

@ethan-crypto ethan-crypto removed the request for review from jasonmorton August 10, 2023 19:32
Comment on lines +6 to +10
import GenPK from './GenPk'
import GenVk from './GenVk'
import GenProof from './GenProof'
import Verify from './Verify'
import Hash from './Hash'
Copy link
Collaborator

Choose a reason for hiding this comment

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

There components should live in /components

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.

2 participants