Conversation
|
The changes in this PR introduce several new features and modifications to existing logic, which could potentially introduce bugs or require deep understanding of the project architecture. Here are some of the key changes and their potential impact:
Considering the potential impact on the codebase and the need for deep understanding of the project architecture, this PR should be reviewed by experienced developers with extensive knowledge of the Aleph.im network and its underlying technologies. The 'BLACK' label is appropriate for this PR, indicating a high level of complexity and potential risk. |
b887e98 to
ba9ad6e
Compare
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The PR correctly fixes the Cosmos signing pipeline: get_public_key() now returns a hex string (.hex()) instead of incorrectly calling .decode() on raw bytes, and sign_message properly decodes the hex back to bytes before base64-encoding for the signature payload. The verify_signature implementation is functional, get_fallback_account gains the path parameter for consistency with other chains, and Chain.CSDK is wired into the account registry. One real concern: the except Exception in verify_signature is too broad and will mask unrelated errors (e.g. type mismatches inside vk.verify) as BadSignatureError. A docstring typo and a slightly misleading test are also worth fixing.
src/aleph/sdk/chains/cosmos.py (line 122): except Exception as e is too broad. The only expected exception from vk.verify() is ecdsa.BadSignatureError. Catching all exceptions and re-raising as BadSignatureError would obscure genuinely unexpected errors (e.g. internal assertion failures, type errors). Prefer:
except ecdsa.BadSignatureError as e:
raise BadSignatureError from eNote that VerifyingKey.from_string (outside the try block) already raises MalformedPointError for bad keys, so those propagate correctly.
src/aleph/sdk/chains/cosmos.py (line 103): Stray ! at the end of the docstring: "Raises:\n BadSignatureError: If the signature is invalid.!" — remove the trailing !.
tests/unit/test_chain_cosmos.py (line 57): get_verification_buffer(message) is called after sign_message has already added "signature" to message. get_verification_buffer only reads chain/sender/type/item_hash so it still works, but the test is signing a raw verification buffer (CSDK\n<addr>\nPOST\nSomeHash) that is different from what sign_message actually signs (the JSON-wrapped Cosmos amino format from get_verification_string). This mismatch makes the test a bit misleading — consider adding a comment clarifying that this tests the low-level sign_raw/verify_signature path rather than the on-chain Cosmos signing path, or restructure to not call sign_message beforehand.
tests/unit/test_chain_cosmos.py (line 82): The bad-signature test corrupts the signature by prepending "1" to the base64 string, which changes the base64-decoded length and likely causes a decode/padding error rather than an actual signature mismatch. The test passes because except Exception catches everything, but it doesn't exercise the core ECDSA bad-signature code path. A stronger test would flip a few bits inside the decoded signature bytes and re-encode, ensuring the signature is valid base64 but cryptographically invalid.
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This PR correctly fixes the Cosmos SDK account integration: the get_public_key() fix (returning hex instead of calling .decode() on raw bytes) and the corresponding sign_message() fix are both correct and necessary. The verify_signature() implementation is functional and consistent with how sign_raw encodes its output (base64). The get_fallback_account signature now matches the pattern used by other chains. The tests cover the main happy-path and bad-signature cases. Two minor issues worth noting: an overly broad except Exception that could swallow unrelated errors, and a docstring typo.
src/aleph/sdk/chains/cosmos.py (line 122): except Exception as e: raise BadSignatureError from e is too broad. vk.verify() already raises BadSignatureError on an invalid signature — catching all exceptions here will also swallow unrelated errors (e.g. internal ecdsa assertion errors or unexpected exceptions), making debugging harder by misrepresenting their nature as a bad signature. Prefer except BadSignatureError: raise to let other exceptions propagate naturally, or just remove the try/except entirely since vk.verify() already raises the right exception type.
src/aleph/sdk/chains/cosmos.py (line 103): Docstring typo: invalid.! should be invalid.
tests/unit/test_chain_cosmos.py (line 61): The bytes branch of verify_signature's signature parameter is not tested (i.e. passing raw signature bytes directly rather than a base64 string). Minor gap, but worth noting since the function explicitly accepts Union[bytes, str].
No description provided.