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

Add EIP1024 conform encryption #7

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

ronaldmannak
Copy link
Contributor

This draft PR is related to mew-wallet-ios-tweetnacl PR#3.

I have added encryption to conform to EIP1024.
Question for the maintainers: is the current approach the best way of adding encryption?

@ronaldmannak
Copy link
Contributor Author

NaCl has the option to encrypt and sign a message in a single pass. The signing is is not conform EIP1024 and presents a small risk (since NaCl signs the raw message, and not conform EIP712, there is likely a small risk of unexpectedly signing a transaction). However, I wonder if there are any scenarios where it would make sense to expose this feature in the API.

@ronaldmannak
Copy link
Contributor Author

@Foboz I've updated this repo as well. Encryption now works, as does decryption using the sender's private key. There might be some possible clean up of the code. Lmk what you think.

@Foboz Foboz changed the base branch from main to develop August 19, 2022 02:06
}

extension PrivateKeyEth1 {
public func eth_publicKey() throws -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ronaldmannak can you please keep this method to not break back compatibility, but mark it as 'deprecated'?

/// Create curve25519 public key from Ethereum private key
/// - Returns: public key
public func curve25519PublicKeyData() throws -> Data {
return try TweetNacl.keyPair(fromSecretKey: self.data()).publicKey
Copy link
Collaborator

@Foboz Foboz Sep 2, 2022

Choose a reason for hiding this comment

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

@ronaldmannak in that case I think it's better to have private var curve25519Keypair and work with it in all 4 methods (actually it's better to use public var <name>: <type> { get throws { ... } }), instead of having 2 methods with calls of TweetNacl.keyPair dependencies on each other.

@@ -51,12 +95,72 @@ extension EthEncryptedData {

return message
}


/// Decrypt the message using the sender's private key
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ronaldmannak this looks good to me, but indentations needs to be fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and done

@@ -22,7 +22,8 @@ let package = Package(
.package(url: "https://github.com/Quick/Nimble.git", .upToNextMajor(from: "9.0.0")),
.package(url: "https://github.com/MyEtherWallet/bls-eth-swift.git", .exact("1.0.1")),
.package(url: "https://github.com/attaswift/BigInt.git", from: "5.2.1"),
.package(name: "MEWwalletTweetNacl", url: "https://github.com/MyEtherWallet/mew-wallet-ios-tweetnacl.git", .upToNextMajor(from: "1.0.0"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ronaldmannak can be reverted

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

#2

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.

2 participants