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.

Sign up for GitHub

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

Jump to bottom

feat(domainobjs) - code and tests refactoring #767

Merged
merged 19 commits into from
Dec 4, 2023
Merged

feat(domainobjs) - code and tests refactoring #767

merged 19 commits into from
Dec 4, 2023

Conversation

ctrlc03
Copy link
Collaborator

@ctrlc03 ctrlc03 commented Oct 23, 2023 •
edited
Loading

depends on #749 (that should be merged first)

Refactoring of the domainobjs package:

  • separate classes into their own files
  • separate tests
  • typedoc
  • BigInt -> bigint
  • optimizedmt d.ts declaration

@ctrlc03 ctrlc03 linked an issue Oct 23, 2023 that may be closed by this pull request
refactor domainobjs #768
Closed
5 tasks
@ctrlc03 ctrlc03 self-assigned this Oct 23, 2023
@ctrlc03 ctrlc03 force-pushed the refactoring/domainobjs branch from c1a9b0c to 379f801 Compare November 2, 2023 09:33
@ctrlc03 ctrlc03 marked this pull request as ready for review November 2, 2023 10:05
@ctrlc03 ctrlc03 mentioned this pull request Nov 7, 2023
Closed
8 tasks
@ctrlc03 ctrlc03 force-pushed the refactoring/domainobjs branch from 242161f to 5de2102 Compare November 14, 2023 22:14
0xmad
0xmad reviewed Nov 28, 2023
View reviewed changes
Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

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

@ctrlc03 thanks, just left some suggestions and improvements.

domainobjs/ts/__tests__/artifacts/test_vk.json Outdated Show resolved Hide resolved
domainobjs/ts/__tests__/ballot.test.ts Outdated Show resolved Hide resolved
domainobjs/ts/__tests__/commands.test.ts Outdated Show resolved Hide resolved
domainobjs/ts/ballot.ts Outdated Show resolved Hide resolved
domainobjs/ts/ballot.ts Outdated Show resolved Hide resolved
domainobjs/ts/publicKey.ts Outdated Show resolved Hide resolved
domainobjs/ts/publicKey.ts Outdated Show resolved Hide resolved
domainobjs/ts/publicKey.ts Outdated Show resolved Hide resolved
domainobjs/ts/stateLeaf.ts Outdated Show resolved Hide resolved
domainobjs/ts/verifyingKey.ts Outdated Show resolved Hide resolved
@ctrlc03 ctrlc03 force-pushed the refactoring/domainobjs branch from f7332ef to c3eb832 Compare November 29, 2023 10:33
@ctrlc03 ctrlc03 requested a review from 0xmad November 29, 2023 12:31
0xmad
0xmad reviewed Nov 29, 2023
View reviewed changes
Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

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

@ctrlc03 thanks, just some missing comments.

domainobjs/ts/commands.ts Outdated Show resolved Hide resolved
domainobjs/ts/commands.ts Outdated Show resolved Hide resolved
domainobjs/ts/message.ts Outdated Show resolved Hide resolved
domainobjs/ts/privateKey.ts Outdated Show resolved Hide resolved
domainobjs/ts/__tests__/commands.test.ts Show resolved Hide resolved
domainobjs/ts/keyPair.ts Outdated Show resolved Hide resolved
0xmad
0xmad previously approved these changes Nov 29, 2023
View reviewed changes
Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

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

@ctrlc03 thanks!

@ctrlc03
Copy link
Collaborator Author

ctrlc03 commented Nov 29, 2023

@ctrlc03 thanks!

thanks to you for the review and great suggestions, much appreciated

samajammin
samajammin reviewed Nov 29, 2023
View reviewed changes
domainobjs/ts/types.ts Outdated Show resolved Hide resolved
samajammin
samajammin reviewed Nov 29, 2023
View reviewed changes
domainobjs/ts/ballot.ts Outdated Show resolved Hide resolved
samajammin
samajammin requested changes Nov 29, 2023
View reviewed changes
Copy link
Member

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

Nice work! A few questions & suggestions

@ctrlc03 ctrlc03 force-pushed the refactoring/domainobjs branch from 4a67e82 to 109d840 Compare December 1, 2023 13:42
@ctrlc03 ctrlc03 requested a review from samajammin December 1, 2023 13:43
0xmad
0xmad reviewed Dec 1, 2023
View reviewed changes
Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

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

@ctrlc03 thanks, just few comments about code style and imports

cli/ts/genLocalState.ts Outdated Show resolved Hide resolved
domainobjs/ts/ballot.ts Show resolved Hide resolved
domainobjs/ts/commands.ts Outdated Show resolved Hide resolved
domainobjs/ts/commands.ts Outdated Show resolved Hide resolved
domainobjs/ts/commands.ts Outdated Show resolved Hide resolved
domainobjs/ts/keyPair.ts Outdated Show resolved Hide resolved
domainobjs/ts/publicKey.ts Outdated Show resolved Hide resolved
domainobjs/ts/verifyingKey.ts Outdated Show resolved Hide resolved
@ctrlc03 ctrlc03 force-pushed the refactoring/domainobjs branch from 91f59e9 to a7a9b1a Compare December 1, 2023 17:19
@ctrlc03 ctrlc03 requested a review from 0xmad December 1, 2023 17:34
0xmad
0xmad reviewed Dec 1, 2023
View reviewed changes
Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

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

@ctrlc03 seems changes haven't been applied. Is it something related to rebasing?

domainobjs/ts/ballot.ts Show resolved Hide resolved
domainobjs/ts/verifyingKey.ts Outdated Show resolved Hide resolved
@ctrlc03
Copy link
Collaborator Author

ctrlc03 commented Dec 2, 2023

@ctrlc03 seems changes haven't been applied. Is it something related to rebasing?

Yes, very sorry about that, I incorporated changes via the ui, then rebased locally and lost those 😅

ctrlc03 and others added 18 commits December 4, 2023 09:55
@ctrlc03
feat(domainobjs) - code and tests refactoring
97d537b
@ctrlc03
fix(domain-objs) - fix build errors due to type mismatch
817b371
@ctrlc03
fix(domainobjs) - add package-lock
55f4c98
@ctrlc03
refactoring(domainobjs) - ensure dependencies are correct and add opt…
f899eaa 
…imizedmt types
@ctrlc03
refactoring(domainobjs) - ensure package-locks are up to date
599ec9d
@ctrlc03
refactoring(domainobjs) - rebase and ensure recent dev changes are in…
0f22652 
…tegrated correctly, fix syntax and typedoc, make Command an abstract class
@ctrlc03 @0xmad
refactoring(domainobjs) - apply suggestions from code review
50ef2d9 
Co-authored-by: Anton <[email protected]>
@ctrlc03
refactoring(domainobjs) - implement PR review comments into the code
e0f887f
@ctrlc03
refactoring(domainobjs) - revert a change in the asArray method of Ba…
b48a8d5 
…llot, and add one extra test case
@ctrlc03
refactoring(domainobjs) - simplify ballot asArray code and ensure cor…
0b76e0a 
…rectness
@ctrlc03 @0xmad
refactoring(domainobjs) - add PR review suggestions
2804200 
Co-authored-by: Anton <[email protected]>
@ctrlc03
refactoring(domainobjs) - implement further suggestions into the code…
d92b958 
…, and add more tests to verify these changes
@ctrlc03
refactoring(domainobjs) - incorporate PR review comments, and run pre…
0866948 
…ttier
@ctrlc03
refactoring(domainobjs) - fix argument duplication in genProof
bf9331d
@ctrlc03
refactoring(domainobjs) - rebase to dev and ensure tests run
c66b2ae
@ctrlc03
refactoring(domainobjs) - run prettier
9a72adf
@ctrlc03 @0xmad
refactoring (domainobjs) - apply suggestions from code review
2549730 
Co-authored-by: Anton <[email protected]>
@ctrlc03
refactoring(domainobjs) - amend documentation references of EVM key t…
d6039cd 
…o Ethereum key and run prettier
@ctrlc03 ctrlc03 force-pushed the refactoring/domainobjs branch from 0cbcb83 to d6039cd Compare December 4, 2023 09:55
@ctrlc03 ctrlc03 requested a review from 0xmad December 4, 2023 09:55
0xmad
0xmad approved these changes Dec 4, 2023
View reviewed changes
Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

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

@ctrlc03 thanks!

samajammin
samajammin approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

Good stuff, thanks for making those updates!

@samajammin samajammin merged commit 76b57a9 into dev Dec 4, 2023
10 checks passed
@samajammin samajammin deleted the refactoring/domainobjs branch December 4, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baumstern baumstern baumstern left review comments

@0xmad 0xmad 0xmad approved these changes

@samajammin samajammin samajammin approved these changes

Assignees

@ctrlc03 ctrlc03

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

refactor domainobjs
4 participants
@ctrlc03 @baumstern @samajammin @0xmad