Skip to content

fix: unify encryption across platforms - #3470

Open
MrCyjaneK wants to merge 2 commits into
devfrom
cyjan-unify-encryption
Open

fix: unify encryption across platforms#3470
MrCyjaneK wants to merge 2 commits into
devfrom
cyjan-unify-encryption

Conversation

@MrCyjaneK

Copy link
Copy Markdown
Collaborator

Issue Number (if Applicable): Fixes #

Description

Please include a summary of the changes and which issue is fixed / feature is added.

Pull Request - Checklist

  • Initial Manual Tests Passed
  • Double check modified code and verify it with the feature/task requirements
  • Format code
  • Look for code duplication
  • Clear naming for variables and methods
  • Manual tests in accessibility mode (TalkBack on Android) passed

@MrCyjaneK
MrCyjaneK force-pushed the cyjan-unify-encryption branch 2 times, most recently from 37a5d63 to 055685a Compare August 3, 2026 07:12
@MrCyjaneK
MrCyjaneK marked this pull request as ready for review August 3, 2026 13:02
@MrCyjaneK
MrCyjaneK force-pushed the cyjan-unify-encryption branch from 055685a to 0b044d1 Compare August 5, 2026 10:46
@MrCyjaneK
MrCyjaneK force-pushed the cyjan-unify-encryption branch from 0b044d1 to 0e6ad10 Compare August 13, 2026 11:02

@OmarHatem28 OmarHatem28 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also can we add some tests to this migration, add cases for correct migration and for failed migration, and for writing multiple times together, and reading multiple times together, etc...

Comment thread pubspec_base.yaml Outdated
Comment on lines +55 to +60
await _writeXChaCha20(
path: path,
password: password,
data: data,
highEntropyPassphrase: useHighEntropy,
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should wrap this in a try/catch, so a failure here won't stop wallet from opening/read from completing

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

it shouldn't fail at all and if it does it better do so loudly

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it will not fail loudly, it will just fail to open the wallet silently, and the user will end up with a wallet that can't be opened, and since it passed our tests, so we don't need it to fail loudly, we need it to work, and for any reason the user might have, we shouldn't block him from opening his wallet, we can add another way to check if the migration worked tho, but it will be a bit of a headache

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

True, but it can only fail if

  1. secure random is unavailable (impossible)
  2. file wasn't written (out of space / wrong permissions)

and even if it fails a fix is a matter of reopening the app after solving the problem. Unless I'm missing something

Comment thread cw_core/lib/utils/file.dart Outdated
@MrCyjaneK
MrCyjaneK force-pushed the cyjan-unify-encryption branch from 0e6ad10 to ab46f8f Compare September 10, 2026 04:27
@github-actions

Copy link
Copy Markdown

changes in dependencies

changed git refs:
  cake_backup
    url   https://github.com/cake-tech/cake_backup.git -> https://github.com/cake-tech/cake_backup.git
    ref   3aba867dcab6737f6707782f5db15d71f303db38 -> b5d86a2a21e1c186cd0baa4df1f3a4f3f9413056
    path  - -> -
     lib/backup.dart       | 90 ++++++++++++++++++++++++++++++++++++++-------------
     test/backup_test.dart | 50 +++++++++++++++++++++++++---
     2 files changed, 114 insertions(+), 26 deletions(-)

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