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

Group private keys when categorizing #3618

Merged
merged 1 commit into from
Sep 4, 2023
Merged

Group private keys when categorizing #3618

merged 1 commit into from
Sep 4, 2023

Conversation

Shadowfiend
Copy link
Contributor

@Shadowfiend Shadowfiend commented Sep 3, 2023

When the signer id abstraction was introduced, I misunderstood each private key as needing to be treated as its own signer. In fact, we treat, on the UI side, all private keys as a single signer group. To fix this, rather than return the wallet id for the signer in these cases, we just return the constant string "private-key". This ensures all private keys are categorized together.

Testing

  • Import 2 or more private keys. Observe that they are under one category in the account list (see below). On the previous release version, they should be listed as distinct categories. For simplicity, you can use 0c1cd323ff8fd5b34f260d580c3a843f3e06c3e1e31c357d38a4a22e8b26809b (used for testing 0-leading private key imports) and 1c1cd323ff8fd5b34f260d580c3a843f3e06c3e1e31c357d38a4a22e8b26809b (swaps the 0 to a 1 in the previous key) as private keys.
Screenshot 2023-09-02 at 23 50 04

Fixes #3606.

Latest build: extension-builds-3618 (as of Sun, 03 Sep 2023 04:04:18 GMT).

When the signer id abstraction was introduced, I misunderstood each
private key as needing to be treated as its own signer. In fact, we
treat, on the UI side, all private keys as a single signer group. To
fix this, rather than return the wallet id for the signer in these
cases, we just return the constant string `"private-key"`. This ensures
all private keys are categorized together.
Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

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

Looks good ✅

@jagodarybacka jagodarybacka merged commit 1f9329f into main Sep 4, 2023
7 checks passed
@jagodarybacka jagodarybacka deleted the joint-privacy branch September 4, 2023 10:21
@Shadowfiend Shadowfiend mentioned this pull request Sep 7, 2023
Shadowfiend added a commit that referenced this pull request Sep 8, 2023
## Highlights
- Fixed an issue where valid private keys would not be accepted. These
always had one or more `0`s at the beginning.
- Fixed an issue where certain types of errors were causing sites not to
load correctly.

## What's Changed
* v0.48.0 by @Shadowfiend in
#3610
* Attempt to import private key to validate it by @jagodarybacka in
#3614
* Group private keys when categorizing by @Shadowfiend in
#3618
* Assert URL of the scan website opened for unverified assets by
@michalinacienciala in #3602
* Handle invalid responses on batch rpc providers by @hyphenized in
#3615
* Joint Privacy: Rework analytics service event management and add
signing tracking by @Shadowfiend in
#3623


**Full Changelog**:
v0.48.0...v0.49.0

Latest build:
[extension-builds-3625](https://github.com/tahowallet/extension/suites/15924368205/artifacts/909765830)
(as of Thu, 07 Sep 2023 20:56:50 GMT).
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.

Account list - regression with multiple Priv keys sections
2 participants