Replace Assertions with Conditional Error Raising #100
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cACK, i will do some tests to it. |
jdlcdl
reviewed
Jul 29, 2025
src/embit/bip85.py
Outdated
| assert num_bytes <= 64 | ||
| assert num_bytes >= 16 | ||
| if num_bytes > 64: | ||
| raise ValueError("Number of bytes must be less than 64") |
There was a problem hiding this comment.
"... must not exceed 64"
or if not 16 <= num_bytes <= 64: raise ValueError("Number of bytes must be between 16 and 64") ?
|
I've carefully reviewed these changes. I didn't catch any logic errors in this pr, seems to be doing the same thing as before (except specific exceptions rather than AssertionError (maybe some projects catch these?)) I noted one wording change, where "less than 64" might better be "less than or equal to 64". |
In order to test the replace assertions with conditional error raising, specifically, in bip85 code, was added a test for `derive_entropy` method as well the failure cases described in `derive_mnemonic` and `derive_hex`.
Collaborator
Author
|
Commits from this PR were merged to |
Collaborator
Author
|
The last commit (which I had forgotten to add when opening the PR) is the most important, as the removed assert would disrupt the descriptor parsing logic when Embit is run with the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some consider
assertsuitable only for debugging because, as when a Python script is executed with the-O(optimize) flag, allassertstatements are stripped from the code at runtime.This is particularly relevant in Android environments, where Buildozer, by default, runs with the
-Oflag enabled. As a result, important validation checks implemented with assert would be skipped entirely.We have already been using these changes in the Krux Android app for some time, and we believe adopting this approach more broadly would be beneficial.