api: Expose reference-vector helpers - #53
BenWestgate wants to merge 2 commits into
Conversation
Promote the module-level checksum and u5 conversion interfaces needed by reference-vector authors while keeping the package-level API narrow. Document and test the supported vector workflow and justify the remaining private cross-module couplings.\n\nValidation: 866 normal and 866 optimized tests; mypy; Ruff; production size budget.\n\nfixes #49
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review (Claude), posted at the maintainer's request.
ACK 7311db8
- Pure renames plus
checksum_for_body_lengthfactored out of_from_partswith the same logic. I ran the api.md example; it parses as aSecret.__all__is 24. - These names become a supported v1 surface. That's the point of #49, but it's a commitment worth accepting on purpose.
- Sequencing: it conflicts with #42 and #57 (mechanical renames only).
- Nit: the new docstrings have a blank line after them, unlike the neighbouring functions.
|
Merge-order note from #42 review: #42 introduces |
|
Review follow-up: the blank-line docstring nit is valid but non-functional. I’m leaving the current one-commit API diff intact until #42 and #57 land because #53 already has mechanical rename conflicts with both; remove the extra blank lines in that single conflict-refresh commit rather than creating another pre-conflict churn commit. Human review order: #42 and #57 before #53. |
|
Second merge-order/API-boundary note from #13 review: after #13 lands, deduplicate the two ASCII-only case-fold implementations by adding one private |
|
Review-submission follow-up: ACK stands. The blank-line-after-docstring nit is style-only; handle it when this branch is refreshed after #13/#42 so the conflict resolution stays one mechanical API-boundary pass. That same refresh should (1) centralize private |
|
Release-gate sequencing note: keep this after #13, #42, and #57 because the current conflicts are mechanical renames. One concrete helper-boundary cleanup should be folded into this pass: #13 currently has equivalent ASCII-only lowercase helpers in |
The newly supported chars_to_u5 interface must not case-fold Unicode into valid Bech32 symbols. Lower only ASCII characters so lookalikes such as the Kelvin sign remain invalid, matching the normalization boundary enforced elsewhere.
|
Agent API-boundary review at current head |
Closes #49.
Promote the module-level checksum specifications, u5 conversion helpers, and checksum-selection helpers required to construct and verify codex32 reference vectors without underscore-prefixed imports. These are supported at their owning modules; this PR does not add them to package-level
codex32.__all__.docs/developer/api.mddocuments the supported vector workflow and explicitly justifies the remaining private cross-module imports as correction-engine, GF/profile, wallet/Core, or CLI implementation couplings. Internal benchmarks may continue to use implementation-private names when they are explicitly testing internals.A production-import audit on the reviewed head finds 48 distinct private
(module, symbol)pairs across 64 same-package import occurrences (47 unique symbol spellings). They are confined to the correction engine, profile/artifact construction, wallet/Core, and CLI implementation boundaries documented here. Renaming those internals would only remove Python’s private-name signal or publish construction/search hooks; it would not improve the supported vector API. No additional pre-v1 rename is warranted.The documented external vector-construction example runs using only
bech32_encode,chars_to_u5, andchecksum_for_body_length; no underscore-prefixed import is required.Review follow-up
316ea11makes the newly supportedchars_to_u5()API ASCII-case-insensitive without Unicode case folding, so a lookalike such as Kelvin signKremains invalid instead of becoming Bech32k. That follow-up adds the direct regression and introduces private_ascii_lower, which is also the intended single helper to absorb #13’s duplicated ASCII-only folding during the final prerequisite refresh.Package-level API accounting: current
reviewability-v1actually has 24 names incodex32.__all__although its developer guide still says 25. #64 will remove the obsolete publiccore_descriptorsexport, making the frozen v1 package-level count 23. This PR's module-level vector helpers do not change that count.Local verification at reviewed head
316ea11:python -O;git diff --check: clean;/usr/local/bin/codex32script. GitHub CI installs the package in its environment and is the authoritative check for that case.This PR intentionally receives its final mechanical refresh only after the overlapping prerequisite work has settled: #42/#57/#46, #13, #33, and #64. #7/#51 are already in the base and #45 is already merged into #42's branch. The overlap audit shows those PRs touch one or more of
bech32.py,correction.py,indel.py,_cli_input.py,cli.py,generation.py,wallet.py,docs/developer/api.md, or their tests. Waiting for that complete set avoids repeatedly invalidating human review with mechanical restacks.The final resolution must keep the supported module-level vector names, mixed-case/diagnostic behavior, #7 dependency-removal/Core fixtures, 83-character HRP boundary, #64 private Core descriptor boundary, and centralized ASCII-only lowercasing in
bech32._ascii_lower. After that refresh, rerun the vector/API tests and full CI, recheck review threads, and rewrite/squash the Codex-authored follow-up under the responsible human author before merge.Disclosure: AI tools were used while implementing and checking the review follow-up, per
docs/developer/AI_POLICY.md.