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

fix: new wallets only created with english mnemonics #5064

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

MuckT
Copy link
Collaborator

@MuckT MuckT commented Mar 8, 2024

Description

Wallets created when the users language was set to Spanish or Portuguese had mnemonics that were in Spanish or Portuguese. This causes interoperability issues for Valora users who wish to try other wallets: Slack Thread.

Test plan

  • Tested locally on iOS
  • Unit tests updated

Related issues

N/A

Backwards compatibility

Yes - Users with Spanish and Portuguese mnemonics are still able to restore into Valora.

Network scalability

N/A

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.56%. Comparing base (df00a4d) to head (afd3cbf).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5064   +/-   ##
=======================================
  Coverage   85.55%   85.56%           
=======================================
  Files         723      723           
  Lines       29448    29439    -9     
  Branches     5078     5074    -4     
=======================================
- Hits        25195    25189    -6     
+ Misses       4020     4017    -3     
  Partials      233      233           
Files Coverage Δ
src/backup/utils.ts 92.85% <100.00%> (-0.74%) ⬇️
src/web3/saga.ts 74.82% <100.00%> (+1.90%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df00a4d...afd3cbf. Read the comment docs.

src/web3/saga.ts Outdated
let mnemonic: string = yield* call(
generateMnemonic,
mnemonicBitLength,
MnemonicLanguages.english,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this just be set to mnemonicLanguage? since it is set as english in the lines above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in b9d9e56!

Copy link
Contributor

@finnian0826 finnian0826 left a comment

Choose a reason for hiding this comment

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

looks like knip check is failing because getMnemonicLanguage is no longer used anywhere so should be removed. Other than that looks good!

@MuckT MuckT enabled auto-merge March 8, 2024 21:13
Copy link

emerge-tools bot commented Mar 8, 2024

1 build increased size

Name Version Download Change Install Change Approval
⚠️ Celo (test)
org.celo.mobile.test
1.80.0 (145) 26.5 MB ⬆️ 2.4 MB (9.84%) 63.0 MB ⬆️ 2.7 MB (4.47%) N/A

Celo (test) 1.80.0 (145)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 2.7 MB (4.47%)
Total download size change: ⬆️ 2.4 MB (9.84%)

Largest size changes

Item Install Size Change
📝 splashBackground@3x.jpg ⬆️ 600.2 kB
📝 background@3x.jpg ⬆️ 368.6 kB
📝 boost-rewards@3x.png ⬆️ 188.4 kB
📝 background@2x.jpg ⬆️ 176.1 kB
📝 boost-rewards@2x.png ⬆️ 90.1 kB
View Treemap

Image of diff


🛸 Powered by Emerge Tools

@MuckT MuckT added this pull request to the merge queue Mar 8, 2024
Merged via the queue into main with commit c0e2b65 Mar 8, 2024
16 checks passed
@MuckT MuckT deleted the tomm/english-mnemonic-creation-only branch March 8, 2024 22:00
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
### Description

Wallets created when the users language was set to Spanish or Portuguese
had mnemonics that were in Spanish or Portuguese. This causes
interoperability issues for Valora users who wish to try other wallets:
[Slack
Thread](https://valora-app.slack.com/archives/C02KBT0DAHJ/p1709917662991769).

### Test plan

- Tested locally on iOS
- Unit tests updated

### Related issues

N/A

### Backwards compatibility

Yes - Users with Spanish and Portuguese mnemonics are still able to
restore into Valora.

### Network scalability

N/A
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.

None yet

2 participants