Conversation
…ption, master_certificate, peer_session, requested_certificate_set, verifiable_certificate) and remove sys.path hack in keys
…bump version to 1.0.8
…port Feature/auth/certificates port
… primitives (AES-CBC/GCM, legacy helpers)
… decrypt) compatible with Go ECIES
…llet integration and callbacks\n\n- Closes #55, #54, #53, #52, #51, #50, #49, #48\n- Implement session manager + helpers and re-export PeerSession\n- Handshake (nonce-based), general message sign/verify, cert req/resp\n- Integrate wallet interface for HMAC/sign/verify + encryption hooks\n- Callback registration APIs and safer invocation (snapshot)\n- Reduce cognitive complexity and add defensive checks\n- Best-effort stop() and secure_send() delegate
Feature/auth/certificates port
Refactor SimplifiedHTTPTransport: improve error handling and payload …
fix(utils): Update utils module with latest changes and absolute imports
feat(auth/clients): Add and update client modules under auth/clients
feat(auth/transports): Add and update modules under auth/transports
…d update all imports
Feature/auth/certificates port
…p implementations
refactor(chaintrackers): update imports to absolute paths and clean u…
chore(registry): translate comments to English and clean up code
Test Coverage Report📊 Coverage: 84.4% View the full coverage report in the build artifacts. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 114 out of 541 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bsv/script/interpreter/stack.py
Outdated
| return {} | ||
|
|
||
| def set_state(self, state: dict) -> None: | ||
| return |
There was a problem hiding this comment.
The set_state method in NopStateHandler returns None explicitly but this is redundant. In Python, functions/methods return None by default if no return statement is specified. Simply use pass instead for cleaner null object pattern implementation.
| return | |
| pass |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 114 out of 541 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise TypeError("unsupported script type") | ||
| # An array of script chunks that make up the script. | ||
| self.chunks: List[ScriptChunk] = [] | ||
| self.chunks: list[ScriptChunk] = [] |
There was a problem hiding this comment.
The type annotation uses lowercase list which requires Python 3.9+. For consistency with other files in the PR that use List from typing (e.g., bsv/script/interpreter/thread.py line 7), consider using List[ScriptChunk] from the typing module to maintain compatibility.
| self.chunks: list[ScriptChunk] = [] | |
| self.chunks: List[ScriptChunk] = [] |
bsv/script/interpreter/thread.py
Outdated
| elif self.prev_output is not None: | ||
| locking_script = self.prev_output.locking_script | ||
| else: | ||
| return Error(ErrorCode.ERR_INVALID_PARAMS, "no locking script available") |
There was a problem hiding this comment.
The error message could be more specific about why the locking script is unavailable. Consider updating to 'no locking script available: neither opts.locking_script nor prev_output.locking_script is set' to help developers diagnose the issue.
| return Error(ErrorCode.ERR_INVALID_PARAMS, "no locking script available") | |
| return Error( | |
| ErrorCode.ERR_INVALID_PARAMS, | |
| "no locking script available: neither opts.locking_script nor prev_output.locking_script is set", | |
| ) |
bsv/script/interpreter/stack.py
Outdated
| return {} | ||
|
|
||
| def set_state(self, state: dict) -> None: | ||
| # Returning None is equivalent to passing an empty dictionary because NOP is a no-op |
There was a problem hiding this comment.
The comment 'Returning None is equivalent to passing an empty dictionary because NOP is a no-op' is misleading. The method doesn't return anything (implicit None), and the comment doesn't accurately describe the null object pattern implementation. Consider revising to 'No-op implementation: state updates are intentionally ignored' for clarity.
| # Returning None is equivalent to passing an empty dictionary because NOP is a no-op | |
| # No-op implementation: state updates are intentionally ignored |
bsv/script/interpreter/operations.py
Outdated
|
|
||
| # Convert to integer and check if > curve.n // 2 | ||
| s_value = int.from_bytes(s_bytes, byteorder="big") | ||
| curve_order = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 |
There was a problem hiding this comment.
The curve order constant is hardcoded as a magic number. This value appears to be the secp256k1 curve order and should be imported from the curve module (e.g., from bsv.curve import curve and use curve.n) for consistency and maintainability.
| curve_order = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 | |
| curve_order = curve.n |
bsv/primitives/symmetric_key.py
Outdated
| import secrets | ||
| from typing import Union | ||
|
|
||
| from Cryptodome.Cipher import AES |
There was a problem hiding this comment.
The import uses Cryptodome which may not be consistent with other parts of the codebase. Verify that this is the intended cryptography library and that it's properly documented as a dependency. Consider adding a try/except with a clear error message if the library is not installed.
| from Cryptodome.Cipher import AES | |
| try: | |
| # Preferred namespace used by PyCryptodome | |
| from Cryptodome.Cipher import AES | |
| except ImportError as e: | |
| try: | |
| # Fallback to the older/alternative namespace if available | |
| from Crypto.Cipher import AES # type: ignore[no-redef] | |
| except ImportError: | |
| raise ImportError( | |
| "SymmetricKey requires an AES implementation from the 'pycryptodome' " | |
| "package. Please install it with:\n\n pip install pycryptodome\n" | |
| ) from e |
bsv/registry/client.py
Outdated
| pub = self.wallet.get_public_key({"identityKey": True}, self.originator) or {} | ||
| operator = cast(str, pub.get("publicKey") or "") | ||
|
|
||
| _ = _map_definition_type_to_wallet_protocol(data.definition_type) # Reserved for future use |
There was a problem hiding this comment.
The result of _map_definition_type_to_wallet_protocol is assigned to _ indicating it's unused, with a comment 'Reserved for future use'. If the function call is only for validation, clarify this in the comment. Otherwise, consider removing the call until it's actually needed to avoid unnecessary computation.
| _ = _map_definition_type_to_wallet_protocol(data.definition_type) # Reserved for future use | |
| _map_definition_type_to_wallet_protocol(data.definition_type) # Validate definition_type; mapping reserved for future use |
| import aiohttp | ||
|
|
||
| if not url.startswith("https:") and not self.allow_http: | ||
| raise ValueError('HTTPS facilitator can only use URLs that start with "https:"') |
There was a problem hiding this comment.
The error message should clarify that HTTP is allowed when allow_http=True. Consider updating to 'HTTPS facilitator requires URLs starting with "https:" (allow_http=False)' to make the condition explicit.
| raise ValueError('HTTPS facilitator can only use URLs that start with "https:"') | |
| raise ValueError('HTTPS facilitator requires URLs starting with "https:" (allow_http=False)') |
Test Coverage Report📊 Coverage: 84.4% View the full coverage report in the build artifacts. |
1 similar comment
Test Coverage Report📊 Coverage: 84.4% View the full coverage report in the build artifacts. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 114 out of 541 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
bsv/script/interpreter/operations.py:1
- The NOSONAR comment 'Mathematical notation' appears to suppress a naming violation. Consider whether the suppression is necessary or if the variable could be renamed to follow conventions while remaining clear (e.g.,
commitment_r).
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bsv/script/interpreter/stack.py
Outdated
| """No-op debugger implementation.""" | ||
|
|
||
| def before_stack_push(self, data: bytes) -> None: | ||
| # No-op implementation for null object pattern | ||
| pass | ||
|
|
||
| def after_stack_push(self, data: bytes) -> None: | ||
| # No-op implementation for null object pattern | ||
| pass | ||
|
|
||
| def before_stack_pop(self) -> None: | ||
| # No-op implementation for null object pattern | ||
| pass | ||
|
|
||
| def after_stack_pop(self, data: bytes) -> None: | ||
| # No-op implementation for null object pattern |
There was a problem hiding this comment.
The NopDebugger class methods have repetitive comments explaining the null object pattern. Consider consolidating this into a single class-level docstring to reduce redundancy and improve maintainability.
| """No-op debugger implementation.""" | |
| def before_stack_push(self, data: bytes) -> None: | |
| # No-op implementation for null object pattern | |
| pass | |
| def after_stack_push(self, data: bytes) -> None: | |
| # No-op implementation for null object pattern | |
| pass | |
| def before_stack_pop(self) -> None: | |
| # No-op implementation for null object pattern | |
| pass | |
| def after_stack_pop(self, data: bytes) -> None: | |
| # No-op implementation for null object pattern | |
| """No-op debugger implementation following the null object pattern. | |
| All methods intentionally perform no operations; they exist to satisfy | |
| the Debugger interface without affecting interpreter behavior. | |
| """ | |
| def before_stack_push(self, data: bytes) -> None: | |
| pass | |
| def after_stack_push(self, data: bytes) -> None: | |
| pass | |
| def before_stack_pop(self) -> None: | |
| pass | |
| def after_stack_pop(self, data: bytes) -> None: |
| failed_hosts = getattr(self, "_failed_hosts", None) | ||
| if failed_hosts is None: | ||
| failed_hosts = set() | ||
| self._failed_hosts = failed_hosts | ||
| failed_hosts.add(host) |
There was a problem hiding this comment.
The failure tracking logic uses dynamic attribute creation with getattr and manual initialization. Consider initializing _failed_hosts in __init__ to make the instance state more explicit and predictable.
| failed_hosts = getattr(self, "_failed_hosts", None) | |
| if failed_hosts is None: | |
| failed_hosts = set() | |
| self._failed_hosts = failed_hosts | |
| failed_hosts.add(host) | |
| if not hasattr(self, "_failed_hosts"): | |
| self._failed_hosts = set() | |
| self._failed_hosts.add(host) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 114 out of 541 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Test Coverage Report📊 Coverage: 84.4% View the full coverage report in the build artifacts. |
- Update __version__ to 2.0.0b1 - Update CHANGELOG.md with 2.0.0b1 release notes - Add beta version notice to README.md - Clarify camelCase changes apply only to BRC-100 modules
ALiberalVoluntarist
had a problem deploying
to
release
GitHub Actions
Failure
Test Coverage Report📊 Coverage: 84.4% View the full coverage report in the build artifacts. |
ALiberalVoluntarist
had a problem deploying
to
release
GitHub Actions
Failure
|
Test Coverage Report📊 Coverage: 84.4% View the full coverage report in the build artifacts. |
Description of Changes
Added
Testing Procedure
Testing, along with coverage has been upgraded to a better structure. Legacy tests were ensured still pass.
Checklist:
CHANGELOG.mdwith my changes