Skip to content

Conversation

@fboucquez
Copy link
Contributor

@fboucquez fboucquez commented Dec 2, 2024

verifyEcc could be expensive, taking up to 500 ms to finish on some devices. This PR allows the client code to avoid running the verification on boot with a flag. Devs can run the verification on unit tests to verify if the provided ECC is correct.

This is for V6, we are currently running this version

@fboucquez fboucquez changed the title feat: initEccLib skip verification feat: initEccLib skip verification (v6) Dec 2, 2024
@junderw
Copy link
Member

junderw commented Dec 2, 2024

I would like to see it be a bit more self-documenting.

Someone could add a single line "true" diff and most people won't blink an eye.

Instead of a boolean, maybe the type should be skipVerification?: { DANGER_DO_NOT_VERIFY_ECCLIB: boolean; }

@fboucquez
Copy link
Contributor Author

fboucquez commented Dec 2, 2024

Hi @junderw , what do you think about this one? ups, I'll improve

@fboucquez
Copy link
Contributor Author

Thanks @junderw , what do you think now?

@samholmes
Copy link

How about opt?: { DANGER_DO_NOT_VERIFY_ECCLIB: boolean; } (naming it opt for options and that pattern is flexible for these sort of function flag anti-patterns.

@fboucquez
Copy link
Contributor Author

How about opt?: { DANGER_DO_NOT_VERIFY_ECCLIB: boolean; } (naming it opt for options and that pattern is flexible for these sort of function flag anti-patterns.

Updated

Copy link

@kewde kewde left a comment

Choose a reason for hiding this comment

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

utACK

@junderw
Copy link
Member

junderw commented Dec 3, 2024

Bump the patch version (both package and package-lock) and add something to CHANGELOG.

@junderw junderw merged commit d90fb51 into bitcoinjs:v6 Dec 3, 2024
11 checks passed
@junderw
Copy link
Member

junderw commented Dec 3, 2024

Published.

@fboucquez
Copy link
Contributor Author

Nice! thanks @junderw. I'll update v7

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.

4 participants