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

Redundant Ordinal Numbers in Wallet Recovery Phrase #19813

Closed
jonathansampson opened this issue Nov 29, 2021 · 1 comment · Fixed by brave/brave-core#11359
Closed

Redundant Ordinal Numbers in Wallet Recovery Phrase #19813

jonathansampson opened this issue Nov 29, 2021 · 1 comment · Fixed by brave/brave-core#11359
Assignees
Labels
bug feature/web3/wallet Integrating Ethereum+ wallet support OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA/No release-notes/include

Comments

@jonathansampson
Copy link
Contributor

Description

As reported by a user, the Brave Wallet recovery phrase is displayed with redundant ordinal numbers at times (when a word appears more than once in the phrase).

Steps to Reproduce

  1. Display your Brave Wallet recovery phrase

Actual result:

Note redundant ordinal numbers.

Expected result:

Ordinal numbers should not be duplicated.

Reproduces how often:

Easily.

Brave version (info found on brave://version)

1.34.41

Version/Channel Information:

  • Can you reproduce this issue with the current release? Yes
  • Can you reproduce this issue with the beta channel? Yes
  • Can you reproduce this issue with the dev channel? Yes
  • Can you reproduce this issue with the nightly channel? Yes

Miscellaneous Information:

The problem is with the manner in which the ordinal number is determined. We currently look-up the word in the recovery phrase, and use the returned index to determine the ordinal number. This approach will always return the index of the first instance of the word.

<RecoveryPhraseContainer>
  {recoverPhrase.map((word) =>
    <RecoveryBubble key={word}>
      <RecoveryBubbleText>{recoverPhrase.indexOf(word) + 1}. {word}</RecoveryBubbleText>
    </RecoveryBubble>
  )}
</RecoveryPhraseContainer>

Since we're using recoveryPhrase.map here, we could use the map's index value instead:

<RecoveryPhraseContainer>
  {recoverPhrase.map((word, index) =>
    <RecoveryBubble key={word}>
      <RecoveryBubbleText>{index + 1}. {word}</RecoveryBubbleText>
    </RecoveryBubble>
  )}
</RecoveryPhraseContainer>

Alternatively, another approach would be to use an HTML ordered list (<ol>) and avoid manual-construction of indexes altogether.

@jonathansampson jonathansampson added bug feature/web3/wallet Integrating Ethereum+ wallet support labels Nov 29, 2021
@jonathansampson
Copy link
Contributor Author

I just realized that we're using the word as the key as well. While I'm not aware of any issues this could cause here, this too is probably not wise given the potential for duplicity among keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature/web3/wallet Integrating Ethereum+ wallet support OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA/No release-notes/include
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants