refactor: change how addresses are derived #133

meeh0w · 2025-02-04T13:48:35Z

Description

CP-9730
Related VM Modules PR: https://github.com/ava-labs/vm-modules/pull/223

Changes

Includes a schema migration for the secrets:
- Instead of storing xpub and xpubXP, all primary wallets now store a collection of publicKeys: AddressPublicKey[].
  - Each AddressPublicKey includes the key itself (hex-encoded) and information on the derivation path that was used when it was derived (derivationPath and curve (either secp256k1 or ed25519)).
  - Some AddressPublicKeys can also include a btcWalletPolicyDetails (which did not go through any changes).
- Some wallet types (i.e. Ledger and Keystone) will also store extendedPublicKeys collection.
Addresses are now computed using VM modules. To compute the addresses, VM Modules will build the required derivation paths and request public keys to be derived (so they can use them to compute the addresses):
- Please see the attached diagram for information on what the expected flow of information now is:
Includes the refactoring needed to support the new schema + updated tests.
In the migrations code, I added a new type of migration - one that requires information from a different storage key.
- This was needed because for some wallet types (i.e. Seed Phrase / Ledger / Keystone), I needed to know how many public keys need to be derived in order to keep the same amount of accounts after migration completes. Therefore I had to query the accounts storage space as a dependency of the migration.

Rationale

The previous approach was not very generic -- it relied on passing multiple parameters to VM Modules, many of which were ignored by some modules and required by others. For example:

EvmModule only relied on the extended public key (passed in as xpub)
BitcoinModule relied on the extended public key (xpub) and network.isTestnet.
AvalancheModule relied on the X/P extended public key (xpubXP) and multiple properties of the Network (isTestnet and isDevnet, which boiled down to the network's HRP)

Some implementations even relied on WalletType -- an implementation detail of the wallet that the modules should not care about at all.
Others did not care about most of the parameters they received. This made the interface for getAddress() method quite complex/bloated.

Notes

The existing PubKeyType { evm: string; xp?: string; ed25519?: string } is still alive in the code.
- It is still used in the onboarding frontend, which I did not want to change at the moment as to not bloat the PR even further
- It is also used by avalanche_getAccountPubKey, which I wanted to keep backwards-compatible (but we should update the response for that somewhat soon, as with the addition of ed25519 it became a bit confusing).

Testing

Full regression of the onboarding flow
Full regression of the Import with <XYZ> flows
Full regression of the Add new address flows
Full regression of the Export private key flow
Tests of the migration itself (I believe storage snapshots used by the QA team will be super helpful with that -- we just need to make sure we have snapshots for each wallet type, possibly even for combinations of those).

For all of the above, we need to make sure the wallet's state remains the same:

no accounts are missing
account addresses stay the same (with the exception of HVM addresses - those will change, as it's now using 9000 coin type instead of 60 (same as X/P chains, as opposed to EVM).
for Ledger, remember to check both BIP44 and Ledger Live derivation paths

Screenshots:

No visual changes

Checklist for the author

I've covered new/modified business logic with Jest test cases.
⏳ I've tested the changes myself before sending it to code review and QA.
- I'm still testing scenarios that come to my mind.

src/background/services/storage/schemaMigrations/migrations/wallet_v4/utils/getSecretsType.ts

src/tests/test-utils.tsx

src/background/vmModules/ApprovalController.ts

src/background/services/storage/schemaMigrations/migrations/wallet_v5/wallet_v5.ts

meeh0w · 2025-02-10T18:22:14Z

.nvmrc

IIRC it's the slip10 package that we use for deriving keys with ed25519 curve that needed this version of node (or newer).

meeh0w · 2025-02-10T18:27:25Z

src/background/services/secrets/models.ts

+export const EVM_BASE_DERIVATION_PATH = "m/44'/60'/0'";
+export const AVALANCHE_BASE_DERIVATION_PATH = "m/44'/9000'/0'";


These could probably use better names (those are paths for the legacy xpub and xpubXP).

meeh0w · 2025-02-13T13:32:47Z

src/background/services/secrets/AddressPublicKey.ts

+
+      case 'ed25519': {
+        const hdKey = slip10.fromMasterSeed(seed);
+        key = hex.encode(hdKey.derive(derivationPath).publicKey);


Use .publicKeyRaw here -- publicKey is 0-prefixed (33 bytes total), we need the raw 32 bytes.

Done in d218327

csabbee

Kudos for this, really well done!
I'm not saying I did an all around testing but, I did played around with the dev build and didn't run into anything 🔥

gergelylovas

Very nice work. Especially like the documentation and the new clean secret storage model.

gergelylovas · 2025-02-24T18:28:00Z

src/background/services/secrets/AddressResolver.test.ts

+    // it('should throw error if derivation path is missing', async () => {
+    //   moduleManager.modules = [
+    //     {
+    //       buildDerivationPath: jest.fn().mockResolvedValue({
+    //         EVM: 'path1',
+    //       }),
+    //     } as unknown as Module,
+    //   ];
+
+    //   await expect(
+    //     addressResolver.getDerivationPathsByVM(0, DerivationPath.BIP44, [
+    //       'AVM',
+    //     ]),
+    //   ).rejects.toThrow(SecretsError.DerivationPathMissing);
+    // });
+  });


Please remove

gergelylovas · 2025-02-24T18:30:08Z

src/background/services/secrets/AddressResolver.ts

+  init(moduleManager: ModuleManager) {
+    this.#moduleManager = moduleManager;
+  }


Is there a particular reason why the module manager not injected like the Network and Secrets service?

gergelylovas · 2025-02-24T18:32:16Z

src/background/vmModules/ModuleManager.ts

+  get modules(): Module[] {
+    return this.#modules;
+  }
+


Should we use some more specific getter which doesn't assume all of the modules are always loaded?

gergelylovas · 2025-02-25T18:02:35Z

src/background/services/wallet/WalletService.test.ts

@@ -1586,7 +1678,10 @@ describe('background/services/wallet/WalletService.ts', () => {

    it('returns the correct list of addresses', async () => {
      secretsService.getPrimaryAccountSecrets.mockResolvedValueOnce({
-        xpubXP,
+        extendedPublicKeys: [
+          // buildExtendedPublicKey('xpub', EVM_BASE_DERIVATION_PATH),


please remove if not needed

gergelylovas · 2025-02-25T18:13:40Z

src/background/runtime/BackgroundRuntime.ts

@@ -30,6 +32,8 @@ export class BackgroundRuntime {
    this.lockService.activate();
    this.onboardingService.activate();
    this.moduleManager.activate();
+
+    this.addressResolver.init(this.moduleManager);


Do we need to define the addressResolver here?
Can it just inject the moduleManager via DI like it does with it's other dependencies

gergelylovas · 2025-02-26T20:26:30Z

src/background/services/storage/schemaMigrations/migrations/wallet_v5/wallet_v5.test.ts

+    xpub: 'xpub',
+    xpubXP: 'xpubXP',
+    secretType: 'ledger',
+    btcWalletPolicyDetails: {


Should we have test scenarios where the optional properties are missing?

gergelylovas · 2025-02-26T20:36:20Z

src/background/services/storage/schemaMigrations/migrations/wallet_v5/wallet_v5.ts

+
+export default {
+  previousSchema: legacySecretsSchema,
+  dependencyKeys: [ACCOUNTS_STORAGE_KEY],


How do we ensure version consistency here?
Since the ACCOUNTS_STORAGE_KEY's migrations are running earlier than the wallet migrations (for now) it is possible that a future migration on the account storage will break this V5 migration. We should make sure the key's migration order stays the same.
Maybe we should use the types from the account service? Then at least a change in the types would trigger a TS error in here.

meeh0w had a problem deploying to alpha February 4, 2025 13:48 — with GitHub Actions Failure

meeh0w commented Feb 4, 2025

View reviewed changes

src/background/services/storage/schemaMigrations/migrations/wallet_v4/utils/getSecretsType.ts Outdated Show resolved Hide resolved

meeh0w commented Feb 4, 2025

View reviewed changes

src/tests/test-utils.tsx Outdated Show resolved Hide resolved

meeh0w commented Feb 4, 2025

View reviewed changes

src/background/vmModules/ApprovalController.ts Outdated Show resolved Hide resolved

meeh0w force-pushed the feat/solana-secrets branch from 342d67b to 8d7d399 Compare February 5, 2025 16:18

meeh0w had a problem deploying to alpha February 5, 2025 16:19 — with GitHub Actions Failure

refactor: change how addresses are derived

850bc76

meeh0w force-pushed the feat/solana-secrets branch from 8d7d399 to 850bc76 Compare February 5, 2025 16:42

meeh0w temporarily deployed to alpha February 5, 2025 16:42 — with GitHub Actions Inactive

meeh0w marked this pull request as ready for review February 5, 2025 17:33

meeh0w temporarily deployed to alpha February 5, 2025 17:33 — with GitHub Actions Inactive

meeh0w had a problem deploying to e2e February 5, 2025 17:37 — with GitHub Actions Failure

docs: diagram of address derivation flow

e71788b

meeh0w temporarily deployed to alpha February 5, 2025 18:51 — with GitHub Actions Inactive

meeh0w had a problem deploying to e2e February 5, 2025 18:55 — with GitHub Actions Failure

meeh0w commented Feb 5, 2025

View reviewed changes

src/background/services/storage/schemaMigrations/migrations/wallet_v5/wallet_v5.ts Outdated Show resolved Hide resolved

meeh0w added 7 commits February 6, 2025 09:00

fix: seedphrase onboarding

f4f44aa

fix: seedless & ledger live onboarding

bf383ae

fix: keystone onboarding

f92ba8a

fix: account list when address not present

2d7c552

Merge remote-tracking branch 'origin/main' into feat/solana-secrets

e332bef

fix: self-review issues

80464ab

fix: some fixes & tests for AddressPublicKey class

3c7675b

meeh0w commented Feb 10, 2025

View reviewed changes

meeh0w added 4 commits February 10, 2025 19:56

test: missing specs for AddressResolver

646b3a2

fix: migration types & tests

84a9dfb

fix: further fixes to schema migration

28d67b5

Merge remote-tracking branch 'origin/main' into feat/solana-secrets

55861af

meeh0w temporarily deployed to alpha February 10, 2025 20:52 — with GitHub Actions Inactive

meeh0w had a problem deploying to e2e February 10, 2025 20:57 — with GitHub Actions Failure

fix: missing error messages

15c64e5

meeh0w temporarily deployed to alpha February 11, 2025 07:38 — with GitHub Actions Inactive

meeh0w had a problem deploying to alpha February 11, 2025 07:38 — with GitHub Actions Failure

meeh0w had a problem deploying to e2e February 11, 2025 07:43 — with GitHub Actions Failure

chore: translations

ebacc91

meeh0w temporarily deployed to alpha February 11, 2025 07:49 — with GitHub Actions Inactive

meeh0w had a problem deploying to e2e February 11, 2025 07:54 — with GitHub Actions Failure

meeh0w commented Feb 13, 2025

View reviewed changes

csabbee previously approved these changes Feb 13, 2025

View reviewed changes

meeh0w mentioned this pull request Feb 14, 2025

refactor: NetworkService as a dependency #92

Closed

meeh0w added 2 commits February 17, 2025 18:51

Merge remote-tracking branch 'origin/main' into feat/solana-secrets

fd4e395

fix: use publicKeyRaw instead of publicKey

d218327

meeh0w dismissed csabbee’s stale review via d218327 February 17, 2025 17:51

meeh0w had a problem deploying to alpha February 17, 2025 17:51 — with GitHub Actions Failure

meeh0w requested a review from csabbee February 17, 2025 17:52

fix: conflict resolution issue

a9ddc32

meeh0w temporarily deployed to alpha February 18, 2025 07:47 — with GitHub Actions Inactive

meeh0w had a problem deploying to e2e February 18, 2025 07:52 — with GitHub Actions Failure

Merge remote-tracking branch 'origin/main' into feat/solana-secrets

78bdbb3

meeh0w had a problem deploying to alpha February 19, 2025 12:39 — with GitHub Actions Failure

meeh0w temporarily deployed to alpha February 19, 2025 12:39 — with GitHub Actions Inactive

meeh0w had a problem deploying to e2e February 19, 2025 12:44 — with GitHub Actions Failure

This was referenced Feb 19, 2025

feat: solana chain, balances & fee #150

Closed

[TEST] Solana work-in-progress #160

Draft

gergelylovas requested changes Feb 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: change how addresses are derived #133

refactor: change how addresses are derived #133

meeh0w commented Feb 4, 2025 •

edited

Loading

meeh0w Feb 10, 2025

meeh0w Feb 10, 2025 •

edited

Loading

meeh0w Feb 13, 2025

meeh0w Feb 17, 2025

csabbee left a comment

gergelylovas left a comment

gergelylovas Feb 24, 2025

gergelylovas Feb 24, 2025

gergelylovas Feb 24, 2025

gergelylovas Feb 25, 2025

gergelylovas Feb 25, 2025

gergelylovas Feb 26, 2025

gergelylovas Feb 26, 2025

		export const EVM_BASE_DERIVATION_PATH = "m/44'/60'/0'";
		export const AVALANCHE_BASE_DERIVATION_PATH = "m/44'/9000'/0'";

refactor: change how addresses are derived #133

Are you sure you want to change the base?

refactor: change how addresses are derived #133

Conversation

meeh0w commented Feb 4, 2025 • edited Loading

Description

Changes

Rationale

Notes

Testing

Screenshots:

Checklist for the author

Choose a reason for hiding this comment

meeh0w Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabbee left a comment

Choose a reason for hiding this comment

gergelylovas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meeh0w commented Feb 4, 2025 •

edited

Loading

meeh0w Feb 10, 2025 •

edited

Loading