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

Frankie/milestone#signature validation #445

Merged
merged 11 commits into from
Jan 7, 2025

Conversation

frankiebee
Copy link
Collaborator

closes #438
closes #434
closes #442

return signatureVerify(message, signature, address).isValid
}

throw new Error(`unsupported hashType: ${hashType}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nitpick, what if we move this error handling to the above block of if-statements that throw errors if data is missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

not a blocker for merging

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a fair point. i worry about having functions that potentially do nothing so when in doubt throw errors. but can see if their is a nice way to move that check up some how that still results in a sensible return. ie: return: Promise<true/false>/Error

* @returns {string} The string without the '0x' prefix.
*/

export function addHexPrefix (str: string): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

this fn isnt just adding a hex prefix, it also checks to see if the string already is a hex string with prefix, what do you think about changing the fn name to be more descriptive like checkOrAddHexPrefix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

checkOrAddHexPrefix seems like unnecessary verbosity

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how ever the docs are wrong 😅 so ill fix that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i dont want to ad a 0x if one is already their that would make it invalid so i'm doing what is required of a function that would add a hex prefix with out invalidating the string

rh0delta

This comment was marked as outdated.

rh0delta
rh0delta previously approved these changes Jan 6, 2025
Copy link
Contributor

@rh0delta rh0delta left a comment

Choose a reason for hiding this comment

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

couple minor questions/suggestions, other than that looks good

@frankiebee frankiebee merged commit 742868a into main Jan 7, 2025
3 checks passed
@frankiebee frankiebee deleted the frankie/milestone#signature-validation branch January 7, 2025 18:19
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants