Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

Add encryption conform EIP1024 #3

Merged
merged 11 commits into from
Aug 19, 2022
Merged

Conversation

ronaldmannak
Copy link
Contributor

This is a draft PR

Encryption is needed to have MEWwalletKit conform to EIP1024. This is a first draft with bugs (the new unit test testEncode() crashes at two different places seemingly random (unknown object release and an out-of-bounds index).

Questions for the maintainers:

  • Is the current approach the right one?
  • What is the best way to debug the code calling NaCl?

@Foboz
Copy link
Collaborator

Foboz commented Jul 8, 2022

Hey @ronaldmannak,

Is the current approach the right one?

Everything should be fine, since it's aligned with decryption, except that I don't think that we need to have public visibility of key sizes

crashes at two different places seemingly random

It's mostly because of wrong buffer sizes (encryption buffer should have 32 extra bytes, while encrypted data - 16). Let me double-check that and I'll comment what needs to be fixed

P.S. Also, FYI, we're moving repos to a new location https://github.com/mewwallet/, but it's ok to finish this PR here (since it's not "deprecated" yet) and I'll merge it into mewwallet one

@Foboz
Copy link
Collaborator

Foboz commented Jul 8, 2022

In addition to MyEtherWallet/mew-wallet-ios-kit#7 and key sizes

I think it better to have helper function, like generateNonce inside of twetnacl package, and do not think about that in mew-wallet-kit

@ronaldmannak
Copy link
Contributor Author

Thanks @Foboz. I've updated the code. The unit test encrypts w/o error, but then throws an internal error code upon decryption. It looks like encryption is still not correct. Did I update everything correctly?

@ronaldmannak
Copy link
Contributor Author

@Foboz Thanks for the second review, I appreciate it. The code now seems to work and is ready for review. I've added new unit tests as well. I will update the MEWwalletKit API next.

@ronaldmannak ronaldmannak marked this pull request as ready for review July 14, 2022 04:50
@Foboz
Copy link
Collaborator

Foboz commented Jul 20, 2022

Hi @ronaldmannak, thanks for the update. Looks great! However, do you think that we really need to have keys size in public? If so, I would prefer to update that part before merge

@ronaldmannak
Copy link
Contributor Author

@Foboz My bad, I forgot to change those back. Done.

@Foboz
Copy link
Collaborator

Foboz commented Aug 19, 2022

Hey @ronaldmannak, sorry for being silent. I'm making last checks and will merge

@Foboz Foboz merged commit 21c7187 into MyEtherWallet:main Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants