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

docs: add nat-spec doc comments #255

Merged
merged 6 commits into from
Jan 22, 2025
Merged

docs: add nat-spec doc comments #255

merged 6 commits into from
Jan 22, 2025

Conversation

ly0va
Copy link
Member

@ly0va ly0va commented Jan 20, 2025

Description

Add doc comments, clean up unused errors and interface methods

@jcsec-security
Copy link
Collaborator

jcsec-security commented Jan 21, 2025 •
edited
Loading

@cpb8010 Could you review WebAuthValidator.sol and expand the details you would consider of interest? In particular, it would be great to add some bits to the parsing and transaction validation logic (OZ report > Overview > Production readiness > 4th bullet point)

jcsec-security
jcsec-security requested changes Jan 21, 2025
View reviewed changes
Copy link
Collaborator

@jcsec-security jcsec-security left a comment

Choose a reason for hiding this comment

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

The function rawVerify of WebAuthValidator is missing docstrings. It is a straightforward fn, but I think it is worth adding it for the sake of completeness and have all non-internal fns commented.

https://github.com/matter-labs/zksync-sso-clave-contracts/blob/lyova-comments-and-cleanup/src/validators/WebAuthValidator.sol#L169-L176

src/validators/WebAuthValidator.sol Show resolved Hide resolved
ly0va and others added 3 commits January 21, 2025 14:54
@ly0va
docs: weird -> unusual
02ea728
@ly0va
feat: update timestamp asserter locator with mainnet address
13605fa
@cpb8010
chore: update webauthn docs
de549e4 
Provide some extra information that's less obvious from reading the code
unless you are already familiar with webauthn
@cpb8010 cpb8010 force-pushed the lyova-comments-and-cleanup branch from a5e3586 to de549e4 Compare January 21, 2025 18:45
@cpb8010
Copy link
Contributor

cpb8010 commented Jan 21, 2025

@jcsec-security Let me know if that's enough documentation for an internal testing function. I can add more inline

jcsec-security
jcsec-security approved these changes Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@jcsec-security jcsec-security left a comment

Choose a reason for hiding this comment

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

LGTM

cpb8010
cpb8010 approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@cpb8010 cpb8010 left a comment

Choose a reason for hiding this comment

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

👍

@ly0va ly0va merged commit fc0af34 into main Jan 22, 2025
3 checks passed
@ly0va ly0va deleted the lyova-comments-and-cleanup branch January 22, 2025 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpb8010 cpb8010 cpb8010 approved these changes

@jcsec-security jcsec-security jcsec-security approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ly0va @jcsec-security @cpb8010