Added Ledger to Account Create, Import, Migration of Existing #5701

j-s-n · 2024-12-21T01:18:21Z

Summary

Added ledger flag to newly created accounts (create, import, multisig, DKG) and migrated db.

Testing Plan

Ledger imports were not workingon my laptop so will need to confirm those next.

Create wallet
1. false hard coded
2. tested
Import wallet
1. ledger
  1. false hard coded
  2. tested
2. ledger flag
  1. true hard coded
  2. tested
3. import file
  1. wallet
    1. ledger value decoded
    2. tested
  2. ledger
    1. ledger value decoded
    2. tested
  3. long prompt
    1. ledger value decoded
Multisig
1. particpants wallets
  1. false hard coded
  2. tested
DKG
1. wallet
  1. false hard coded
  2. tested
2. ledger
  1. true hard coded
  2. tested
Edge Cases
1. Migration
  1. old wallets add false ledger flag
2. Old wallet imports
  1. decoding added conditional false ledger property if it doesnt exist

Documentation

N/A


## Breaking Change
Don't think so. Ledger flag is only being added to new and existing entries in db.

hughy · 2025-01-21T19:26:03Z

ironfish-cli/src/commands/wallet/multisig/dkg/create.ts

@@ -529,6 +529,7 @@ export class DkgCreateCommand extends IronfishCommand {
      name: accountName,
      createdAt: null,
      spendingKey: null,
+      ledger: false,


I think this should be ledger: true -- this is in the performRound3WithLedger method

thanks. i corrected this.

hughy · 2025-01-21T19:27:39Z

ironfish/src/migrations/data/033-add-ledger-to-accounts.ts

+} from './033-add-ledger-to-accounts/old/accountValue'
+import { GetStores } from './033-add-ledger-to-accounts/stores'
+
+export class Migration033 extends Migration {


This might need to be Migration034 if it goes in after #5630

moved all of the related files to migration 34, but will discuss if the migration logic will be implemented.

hughy · 2025-01-21T19:31:28Z

ironfish/src/migrations/data/033-add-ledger-to-accounts.ts

+  accountForward(oldValue: OldDecryptedAccountValue): NewDecryptedAccountValue {
+    const newValue = {
+      ...oldValue,
+      ledger: false,


Since the ledger flag will already be false in every account I don't think we need a migration for accounts.

If we want to update the database version we could have a placeholder migration

will discuss if the migration logic will be implemented or if we can do a placeholder migration

hughy · 2025-01-21T19:38:57Z

ironfish/src/rpc/routes/wallet/multisig/importParticipant.ts

@@ -68,6 +68,7 @@ routes.register<typeof ImportParticipantRequestSchema, ImportParticipantResponse
      {
        name: request.data.name,
        secret: request.data.secret ? Buffer.from(request.data.secret, 'hex') : undefined,
+        ledger: true,


I don't think we want this to be true unless we can pass in ledger with the request.

On the other hand, I'm not sure we will want to import participant identities moving forward.

hughy · 2025-01-21T21:06:36Z

ironfish/src/wallet/walletdb/multisigIdentityValue.ts

@@ -19,6 +19,7 @@ export interface MultisigIdentityValue {
   * while allowing distinction between undefined and actual secrets during deserialization.
   */
  secret?: Buffer
+  ledger: boolean


Since this field isn't optional we need a migration to update existing stored identities.

I think we may actually want to avoid adding ledger to this datastore. I think we should avoid importing just the identity from a ledger device without also importing a full account, and the goal of #5692 is to not have multisig identities stored for imported accounts

NullSoldier · 2025-01-21T23:27:59Z

ironfish-rust-nodejs/index.d.ts

@@ -320,6 +320,7 @@ export namespace multisig {
    incomingViewKey: string
    outgoingViewKey: string
    proofAuthorizingKey: string
+    ledger: boolean


This is an unused change and will be deleted the next time this file is re-generated. This is auto-generated by NAPI-RS so you cannot modify it manually.

think this was added in a previous commit and was causing the lining error comparing rust changes. i removed and it solved the problem.

NullSoldier · 2025-01-21T23:46:14Z

ironfish/src/wallet/walletdb/walletdb.ts

@@ -45,7 +45,7 @@ import { MultisigIdentityValue, MultisigIdentityValueEncoder } from './multisigI
 import { ParticipantIdentity, ParticipantIdentityEncoding } from './participantIdentity'
 import { TransactionValue, TransactionValueEncoding } from './transactionValue'

-const VERSION_DATABASE_ACCOUNTS = 32
+const VERSION_DATABASE_ACCOUNTS = 33


This will need to be bumped and the migration needs to be renamed because this PR just merged in migration 33.

#5630

corrected all references to 34 after 33 was merged

NullSoldier · 2025-01-21T23:49:51Z

ironfish/src/wallet/walletdb/accountValue.test.ts

@@ -30,6 +30,7 @@ describe('AccountValueEncoding', () => {
      },
      scanningEnabled: true,
      proofAuthorizingKey: key.proofAuthorizingKey,
+      ledger: false,


maybe we should flip this to true in one of these tests to check the serialization is working.

test passed with ledger as true

…ger to wallet/createAccount. added a ledger flag to buffer for accountvalue

… addition wouldn't cause bech32 to exceed max limit

…ests

…and dkg

… in testing environment. tested import manually.

… in the import

j-s-n force-pushed the wallet-ledger-flag branch from e497266 to e21967d Compare January 17, 2025 03:00

hughy reviewed Jan 21, 2025

View reviewed changes

NullSoldier reviewed Jan 21, 2025

View reviewed changes

j-s-n force-pushed the wallet-ledger-flag branch from 6740a3c to 38bf2f8 Compare January 22, 2025 00:22

j-s-n and others added 24 commits January 21, 2025 16:28

Added ledger flag to Account class, serialize method. Added false led…

75ec1b7

…ger to wallet/createAccount. added a ledger flag to buffer for accountvalue

added ledger flag to create, import, multisig, dkg

67981ab

added ledger to multisig, dkg, encoders

402c249

debugged multisig and dkg ledger flag. tested on nonledger accounts

93c38d1

created wallet migration for ledger flag

f268549

created 033 migration

73100f5

updated ledger migration to 34

a7d32bd

debugged flow for importing ledger wallet from file

4dbe120

removed ledger flag as param for create/import Participant

1eace0d

fixed merge conflict

d4a3756

updated tests to include new ledger flag and fixed linting

16d571a

added ledger flag to wallet slow tests

ec64d11

restored rust index.d

d7bed31

fixed merge conflict

dc87d43

debugged JsonEncoder tests to add ledger

3e7f143

added ledger flag to account.test JSON

31cef11

updated bech32 decoders and tests to add ledger

59eb3e4

wallet fixture added

d670c07

upated version to 4 in bech32 decoder

ee9198a

hard coded bech32json import string as inpout for test so ledger flag…

32f12e1

… addition wouldn't cause bech32 to exceed max limit

regenerated bech32 JSON import to avoid duplicate account import in t…

f98c7b7

…ests

added json and base64 encoded importTestCases for wallets, multisig, …

316c1c1

…and dkg

removed base64 multisig importTestCase since it code not be decrypted…

5ed9b13

… in testing environment. tested import manually.

removed Bech32JsonEncoder from importAccount.test since we hard coded…

6eb0260

… in the import

Jason Kuroda added 4 commits January 21, 2025 16:40

fixed linting

7e8485c

fixed merge conflict

9801ffc

removed sort from sequence range test

470b3ff

added buffer comparison into sort for getTransactionsByTime

9234876

j-s-n force-pushed the wallet-ledger-flag branch from 38bf2f8 to 9234876 Compare January 22, 2025 00:41

Jason Kuroda added 6 commits January 22, 2025 12:02

updated db version to 34

27aa628

merged wallet fixture merge conflict

3954288

deleted and readded wallet fixtures

d53ab67

removed ledger flag from rust node index dkgRound3Package Interface

034e5ad

deleted and regenerated fixtures

993fa27

corrected dkg round 3 ledger to true

8b42aab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Ledger to Account Create, Import, Migration of Existing #5701

Added Ledger to Account Create, Import, Migration of Existing #5701

j-s-n commented Dec 21, 2024 •

edited

Loading

hughy Jan 21, 2025

j-s-n Jan 22, 2025

hughy Jan 21, 2025

j-s-n Jan 22, 2025

hughy Jan 21, 2025

j-s-n Jan 22, 2025

hughy Jan 21, 2025

hughy Jan 21, 2025

hughy Jan 21, 2025

NullSoldier Jan 21, 2025

j-s-n Jan 22, 2025

NullSoldier Jan 21, 2025

j-s-n Jan 22, 2025

NullSoldier Jan 21, 2025

j-s-n Jan 22, 2025

Added Ledger to Account Create, Import, Migration of Existing #5701

Are you sure you want to change the base?

Added Ledger to Account Create, Import, Migration of Existing #5701

Conversation

j-s-n commented Dec 21, 2024 • edited Loading

Summary

Testing Plan

Documentation

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

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

j-s-n commented Dec 21, 2024 •

edited

Loading