Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Message signing and verifying #343

Merged
merged 8 commits into from
Oct 10, 2023
Merged

Message signing and verifying #343

merged 8 commits into from
Oct 10, 2023

Conversation

popenta
Copy link
Contributor

@popenta popenta commented Oct 2, 2023

In this PR I've added two new sub-commands for the wallet command group:

  • mxpy wallet sign-message
  • mxpy wallet verify-message

mxpy wallet sign-message

This command is used to sign a message. It requires the --message argument and you'll also need to pass in a wallet that will be used for signing the message.

When running mxpy wallet sign-message --message test --pem alice.pem the output will look something like this:

{
            "address": "erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th",
            "message": "test",
            "signature": "0x7aff43cd6e3d880a65033bf0a1b16274854fd7dfa9fe5faa7fa9a665ee851afd4c449310f5f1697d348e42d1819eaef69080e33e7652d7393521ed50d7427a0e"
}

mxpy wallet verify-message

This command is used for verifying a previously signed message. It requires the --address argument which is the bech32 address of the signer, the --message argument which represents the signed message(in readable format) and the --signature argument which is the message signature hex encoded.

To verify the message signed above we can run the following command:

mxpy wallet verify-message \
--address erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th \
--message test \
--signature 0x7aff43cd6e3d880a65033bf0a1b16274854fd7dfa9fe5faa7fa9a665ee851afd4c449310f5f1697d348e42d1819eaef69080e33e7652d7393521ed50d7427a0e

The output will look like this:

The message "test" was signed by erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th

In case the message was not signed by someone with that address the output will be:

The message "not signed" was NOT signed by erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th

@popenta popenta self-assigned this Oct 2, 2023
@popenta popenta marked this pull request as draft October 2, 2023 11:54
@popenta popenta changed the title Message signing Message signing and verifying Oct 3, 2023
Base automatically changed from contract-template to feat/next October 3, 2023 10:37
@popenta popenta marked this pull request as ready for review October 3, 2023 10:39
@andreibancioiu andreibancioiu self-requested a review October 4, 2023 08:27
multiversx_sdk_cli/tests/test_cli_wallet.py Show resolved Hide resolved
multiversx_sdk_cli/cli_wallet.py Outdated Show resolved Hide resolved
multiversx_sdk_cli/cli_wallet.py Outdated Show resolved Hide resolved
is_signed = verifier.verify(verifiable_message)

if is_signed:
show_message(f"""The message "{message}" was signed by {bech32_address}""")
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a prefix for these messages, such as "success" and "failed" (as in "verification failed". E.g:

SUCCESS: The messsage ...

Optional, we should ask for additional opinion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added prefixes, we can still ask for some other opinions, maybe wait to see what the second reviewer thinks about this.


def sign(self) -> None:
hex_signature = self.account.sign_message(self.message.encode())
self.signature = bytes.fromhex(hex_signature)
Copy link
Contributor

Choose a reason for hiding this comment

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

The "signable" message now becomes a "signed message".

Alternatively, could be SignedMessage(message, account) and also do self.signature = ... in the constructor. Not an actual improvement, though.

Or have this file renamed to sign_verify.py and have a sign_message(message, account) return a simple DTO called SignedMessage (also defined in this file), that has the to_dictionary() method, as well. Then, some parts of the verify logic can be moved here, too (for symmetry).

Optional, can also stay as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, great suggestion!

andreibancioiu
andreibancioiu previously approved these changes Oct 6, 2023
@ssd04 ssd04 self-requested a review October 10, 2023 12:11
Comment on lines 97 to 98
"Verify a previously message"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Verify a previously message"
)
"Verify a previously signed message"
)

?

@popenta popenta merged commit 71a006e into feat/next Oct 10, 2023
9 checks passed
@popenta popenta deleted the message-signing branch October 10, 2023 13:53
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