Skip to content

fix COSMOS#198

Open
aliel wants to merge 4 commits intomainfrom
aliel-fix-cosmos
Open

fix COSMOS#198
aliel wants to merge 4 commits intomainfrom
aliel-fix-cosmos

Conversation

@aliel
Copy link
Member

@aliel aliel commented Jan 22, 2025

No description provided.

@aliel aliel requested a review from nesitor January 22, 2025 19:15
@github-actions github-actions bot added the BLACK This PR has critical implications and must be reviewed by a senior engineer. label Jan 22, 2025
@github-actions
Copy link

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:

  1. New Chain Support: The addition of the CSDK chain support involves changes to multiple files, including the main SDK logic and specific chain-related files. This includes:

    • Adding the CSDKAccount class and related methods.
    • Modifying existing chain handling logic to include the new CSDK chain.
    • Introducing new dependencies and changes to the verification process.
  2. Verification Logic: The changes to the verification logic are significant and include:

    • Modifying the way public keys and signatures are handled and encoded/decoded.
    • Adding new methods for verifying signatures, which could affect the overall security and integrity of the system.
    • Introducing exceptions for invalid signatures, which indicates a higher risk of bugs or security vulnerabilities.
  3. Tests: The addition of new tests is a positive step, but the complexity of the changes requires comprehensive testing to ensure reliability. The new tests cover various aspects of the new chain support and verification logic, which is crucial for maintaining the quality of the codebase.

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.

@nesitor nesitor self-requested a review January 22, 2025 19:48
nesitor
nesitor previously approved these changes Jan 22, 2025
@aliel aliel force-pushed the aliel-fix-cosmos branch 4 times, most recently from b887e98 to ba9ad6e Compare January 23, 2025 11:43
@hoh hoh force-pushed the aliel-fix-cosmos branch from 68f0eec to f5d46ad Compare March 5, 2025 10:49
@nesitor nesitor self-requested a review October 8, 2025 11:48
nesitor
nesitor previously approved these changes Oct 8, 2025
Copy link

@foxpatch-aleph foxpatch-aleph left a comment

Choose a reason for hiding this comment

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

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 e

Note 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.

Copy link

@foxpatch-aleph foxpatch-aleph left a comment

Choose a reason for hiding this comment

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

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].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BLACK This PR has critical implications and must be reviewed by a senior engineer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants