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

Hardware Wallet storage should be shared between Iguana and all HD accounts #1621

Open
sergeyboyko0791 opened this issue Jan 19, 2023 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@sergeyboyko0791
Copy link

Currently, Hardware Wallet storage is in the same directory as Iguana/HD account:
https://github.com/KomodoPlatform/atomicDEX-API/blob/eb8792ada38dd7171ede1dd243b6a05b69bcbd32/mm2src/coins/hd_wallet_storage/sqlite_storage.rs#L112-L119

This is incorrect since Hardware Wallet should be shared between the accounts.
This can be fixed by adding a Wallet storage that is not dependent on either Iguana or HD account ID, but depends on the mnemonic passphrase only.

@sergeyboyko0791 sergeyboyko0791 self-assigned this Jan 19, 2023
@sergeyboyko0791 sergeyboyko0791 added the bug Something isn't working label Jan 19, 2023
@sergeyboyko0791 sergeyboyko0791 changed the title HW wallet storage should be shared between Iguana and all HD accounts Hardware Wallet storage should be shared between Iguana and all HD accounts Jan 19, 2023
@sergeyboyko0791
Copy link
Author

sergeyboyko0791 commented Jan 24, 2023

This problem is also related to GUI storage as it should be shared between Iguana and all HD accounts as well, so I believe it's important to design such a shared DB correctly.
I have multiple suggestions:

  1. Use an Iguana DB for data shared between Iguana and HD accounts. This will avoid confusion with the storage names, and . For example, we'll have two properties in the MmCtx:
pub struct MmCtx {
  pub conf: Json,
  // ...
  /// RIPEMD160(SHA256(x)) where x is a secp256k1 pubkey derived from passphrase using **either** Iguana or HD derivation method.
  /// This hash is **unique** among Iguana and each HD accounts derived from the same passphrase.
  pub rmd160: Constructible<H160>,
   /// RIPEMD160(SHA256(x)) where x is a secp256k1 pubkey derived from passphrase using **exactly** Iguana derivation method.
   /// This hash is **common** for Iguana and each HD accounts derived from the same passphrase.
  pub iguana_rmd160: Constructible<H160>,
}
  1. Use a different DB for such shared data. This can be also done in the following ways:
  • Add a hardcoded magic string to the input mnemonic phrase, derive a secp256k1 pubkey from it using Iguana derivation method, and then calculate RIPEMD160(SHA256(x)).
    If we choose this one, we should give such the shared storage a name that won't be confusing. For example, Shared and shared_rmd160: Constructible<H160> correspondingly.
  • Use the same iguana_rmd160: Constructible<H160> as in the 1. point, but use an additional suffix/prefix in the DB file path (in native <RMD160>_WALLET, in WASM WALLET::<RMD160>::<DB_NAME>).

@artemii235 what do you think?

@artemii235
Copy link
Member

@sergeyboyko0791
I think approach 2 is more preferred: iguana_rmd160 name will require additional documentation and explanation that this is used as shared DB ID. Also, after multiple accounts are used, it will be not clear where to look for shared DB files in directories with names like 05aab5342166f8594baf17a7d9bef5d567443327 (not a big problem, but will still require a bit of effort).

Add a hardcoded magic string to the input mnemonic phrase, derive a secp256k1 pubkey from it using Iguana derivation method, and then calculate RIPEMD160(SHA256(x)).

It's good. This way, you can even print the shared DB ID to log, as it can't be linked to any of the user's addresses (unlike iguana_rmd160).
Please also rename shared_rmd160 to shared_db_id to define the purpose clearly.

@sergeyboyko0791
Copy link
Author

Awesome, thank you @artemii235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants