Skip to content

Comments

Small fixes and extra checks#253

Merged
popenta merged 2 commits intofeat/nextfrom
fixes
Jun 11, 2025
Merged

Small fixes and extra checks#253
popenta merged 2 commits intofeat/nextfrom
fixes

Conversation

@popenta
Copy link
Collaborator

@popenta popenta commented Jun 6, 2025

Added a few extra checks when working with mnemonics:

  • ensure mnemonic is valid when deriving secret keys
  • ensure the index is not negative when deriving secret keys

When instantiating a ValidatorSecretKey from a hex string, ensure there are no whitespaces.

@popenta popenta self-assigned this Jun 6, 2025
@github-actions
Copy link

github-actions bot commented Jun 6, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  multiversx_sdk/wallet
  core.py
  errors.py
  mnemonic.py
  user_keys.py 80-81
  validator_keys.py
Project Total  

This report was generated by python-coverage-comment-action

# https://github.com/trezor/python-mnemonic/blob/master/mnemonic/mnemonic.py
# https://ethereum.stackexchange.com/a/72871/59887
def mnemonic_to_bip39seed(mnemonic: str, passphrase: str = ""):
if not Mnemonic(BIP39_LANGUAGE).check(mnemonic):
Copy link
Contributor

Choose a reason for hiding this comment

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

In our Mnemonic class, we already have this check:


class Mnemonic:
    def __init__(self, text: str) -> None:
        text = text.strip()
        self.assert_text_is_valid(text)
        self.text = text

    @classmethod
    def assert_text_is_valid(cls, text: str) -> None:
        if not cls.is_text_valid(text):
            raise InvalidMnemonicError()

However, seems fine, now you also add the check at a lower level.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a unit test for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also fix the link in the comment, it does not work anymore.

https://github.com/trezor/python-mnemonic/blob/master/src/mnemonic/mnemonic.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a text and fixed the link.

@popenta popenta merged commit 9a6401c into feat/next Jun 11, 2025
7 checks passed
@popenta popenta deleted the fixes branch June 11, 2025 05:03
@andreibancioiu andreibancioiu requested a review from Copilot June 19, 2025 08:19
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces small fixes and extra validations across the wallet modules. Key changes include:

  • Trimming whitespace in hex-string conversions for validator and user secret keys.
  • Adding new exception handling for negative address index and invalid mnemonics.
  • Enhancing test coverage by including cases for address index errors and mnemonic validation.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
multiversx_sdk/wallet/validator_keys.py Added .strip() to hex string before conversion
multiversx_sdk/wallet/user_keys.py Added logging and refined exception handling in the verify method
multiversx_sdk/wallet/mnemonic_test.py Updated tests to validate negative address index and invalid mnemonics
multiversx_sdk/wallet/mnemonic.py Integrated address index validation with new error raised
multiversx_sdk/wallet/errors.py Introduced InvalidAddressIndexError with a custom message
multiversx_sdk/wallet/core.py Added a mnemonic check to raise error on invalid mnemonics

Comment on lines +78 to +81
logger.info(str(e))
return False
except Exception as e:
logger.info(str(e))
Copy link

Copilot AI Jun 19, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider logging CryptoError exceptions at a higher severity level (e.g., error) rather than info since these represent failed verifications.

Suggested change
logger.info(str(e))
return False
except Exception as e:
logger.info(str(e))
logger.error(str(e))
return False
except Exception as e:
logger.error(str(e))

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants