search

KomodoPlatform / komodo-defi-framework

This is the official Komodo DeFi Framework repository
https://komodoplatform.com/en/docs/komodo-defi-framework/
103 stars 94 forks source link

[feature request] HD Wallet (multi-account) #740

Open ca333 opened 3 years ago

ca333 commented 3 years ago

Multi Account wallet BIP44 integration alongside BIP39 will be nice enhancement - GUI developers and CLI users would not worry about key-management anymore. Furthermore we can extend this beyond wallet into the DEX - will increase user/dev experience. I envision it one day user can chose which "account" (derivation path/key) to use for specific atomic trades or wallet operations. And as positive effect users can reuse mnemonic from other BIP32/39/44 wallets.

resources: https://www.reddit.com/r/Bitcoin/comments/2srxr2/what_is_the_difference_between_bip_32_and_bip_44/cnspxwq/ rust lib: https://github.com/rust-bitcoin/rust-wallet (not secure code reviewed)

artemii235 commented 3 years ago

@ca333 Thanks for creating the issue, it will be nice enhancement indeed!

Milerius commented 3 years ago

By implementing PR for offline staking of QTUM I realized the need for this feature.

In general, offline staking does not mix well with active trading.

My idea to go to multi-address without having to refactor everything and break everything would rather be to make this feature work only on the new compatible bip39 / bip44 seed and by definition to have the bip39 address + the derived bip44 address.

I imagine an option when starting AtomicDEX to no longer use the Iguana derivation, but bip39 and from there add an optional address collection next to our main address. After that, we could refactor the functions one by one to automatically fall back to the primary address in case.

So WithdrawRequest could became:

#[allow(dead_code)]
#[derive(Deserialize)]
pub struct WithdrawRequest {
    coin: String,
    to: String,
    #[serde(default)]
    amount: BigDecimal,
    #[serde(default)]
    max: bool,
    fee: Option<WithdrawFee>,
    // If not None used as from address instead of the default one
    from: Option<String>
}

and an RPC get_accounts that return the list of public addresses for the given coin.

In the same idea, later my_balance will be able to return the balance of all the addresses, but by default keep the same behaviour. And we can abuse the V2 of the RPC for this kind of operation.

Otherwise, we can also introduce RPC like accounts_infos which will basically regroup balance and other useful information about each account of a given coin

artemii235 commented 3 years ago

I like the idea of having the from: Option<String> in the WithdrawRequest. I think it should even be Option<Vec<String>> to allow withdrawal from multiple addresses at once. Overall, your proposal seems a good point to kickstart HD wallets integration. Please complete your ongoing tasks (QTUM delegation and Solana wallet only) and we can possibly prioritize this then.

artemii235 commented 1 year ago

@sergeyboyko0791 Could you please post a comprehensive test plan on testing the global HD feature? It should contain reference wallets to check against, etc.

sergeyboyko0791 commented 1 year ago

Global HD account feature allows the user to use the BIP32/BIP44/BIP49/BIP84 derivation scheme at the following path: m/purpose'/coin'/0'/0/hd_account_id, where hd_account_id is set globally at the mm2 config, and purpose, coin values vary and depend on a coin configuration.

For example, if the user wants to activate a 3rd HD account (m/purpose'/coin'/0'/0/3), the config should look like:

{
  "gui": "DEVDOCS_CLI",
  "netid": 7777,
  "rpc_password": "YOUR_PASS_HERE",
  "passphrase": "BIP39 MNEMONIC PHRASE",
  // ...
  "hd_account_id": 3
}

Please note that if hd_account_id is set, the passphrase field must follow the BIP39 mnemonic sentence. Currently, English words are supported only.

Coin activation

In order to activate coins, they have to contain derivation_path field to determine purpose and coin values at the derivation path. Coin types can be found at https://github.com/satoshilabs/slips/blob/master/slip-0044.md For example, BTC should have:

{
  "coin": "BTC",
  "name": "bitcoin",
  // ...
  "derivation_path": "m/44'/0'"
}

Testing plan

  1. Standard UTXO. For example, RICK, KMD, BTC, QTUM using electrum, enable (with a local Node) RPCs. purpose = 44.
  2. ETH, ERC20, BNB, ..., using enable, enable_eth_with_tokens RPCs. purpose = 44.
  3. QRC20 using electrum, enable RPCs purpose = 44.
  4. ZCoin using task::enable_z_coin::init. In addition to the derivation_path field, there is also z_derivation_path that should be present at coin_conf["protocol"]["protocol_data"]. For example, 
    {
"coin": "ZOMBIE",
"asset": "ZOMBIE",
"txversion": 4,
"overwintered": 1,
"mm2": 1,
"protocol": {
    "type": "ZHTLC",
    "protocol_data": {
        "consensus_params": {
            "overwinter_activation_height": 0,
            "sapling_activation_height": 1,
            "blossom_activation_height": null,
            "heartwood_activation_height": null,
            "canopy_activation_height": null,
            "coin_type": 133,
            "hrp_sapling_extended_spending_key": "secret-extended-key-main",
            "hrp_sapling_extended_full_viewing_key": "zxviews",
            "hrp_sapling_payment_address": "zs",
            "b58_pubkey_address_prefix": [28, 184],
            "b58_script_address_prefix": [28, 189]
        },
        "check_point_block": {
            "height": 290000,
            "time": 1664200629,
            "hash": "106BAA72C53E7FA52E30E6D3D15B37001207E3CF3B9FCE9BAB6C6D4AF9ED9200",
            "sapling_tree": "017797D05B070D29A47EFEBE3FAD3F29345D31BE608C46A5131CD55D201A631C13000D000119CE6220D0CB0F82AD6466B677828A0B4C2983662DAB181A86F913F7E9FB9C28000139C4399E4CA741CBABBDDAEB6DCC3541BA902343E394160EEECCDF20C289BA65011823D28B592E9612A6C3CF4778F174E10B1B714B4FF85E6E58EE19DD4A0D5734016FA4682B0007E61B63A0442B85E0B8C0CE2409E665F219013B5E24E385F6066B00000001A325043E11CD6A431A0BD99141C4C6E9632A156185EB9B0DBEF665EEC803DD6F00000103C11FCCC90C2EC1A126635F708311EDEF9B93D3E752E053D3AA9EFA0AF9D526"
        },
        "z_derivation_path": "m/32'/133'",
    }
},
"required_confirmations": 0,
"derivation_path": "m/44'/133'",
}

    Please note: derivation_path should have purpose = 44. z_derivation_path should have purpose = 32 (this is required to make mm2 compatible with other zcash wallets). Wallets: https://z.cash/wallets/

  5. BCH, SLP using enable_bch_with_tokens RPC. purpose = 44
  6. Tendermint, TendermintToken using enable_tendermint_with_assets, enable_tendermint_token RPCs. purpose = 44, coin = 566
  7. Solana, SPL token using enable_solana_with_tokens (not required as no GUI supported them) purpose = 44
  8. Lightning using task::enable_lightning::init. Lightning is based on UTXO-segwit coin, so the config and the address should be the same as the platform ones.

cc: @smk762 @tonymorony

shamardy commented 1 year ago
  1. Lightning using task::enable_lightning::init. Lightning is based on UTXO-segwit coin, so the config and the address should be the same as the platform ones.

Please note that lightning derivation path is still an open for research topic after I discussed it with @sergeyboyko0791 on DM. Different lightning node implementations use different derivation paths, e.g.:

In addition to how the master lightning node keys are generated, channel backups are not compatible between different node implementations as well.

so the config and the address should be the same as the platform ones

Currently in AtomicDEX lightning implementation the node's master key is generated from the platform's private key, a bit similar to how it's done in Core Lightning but only one child level is used to get the node's secret that generates the node's publickey/address. Different hardened childs are used to generate more master keys that are important for the node's operation.

smk762 commented 1 year ago

Testing completed prior to merge, though there have since been changes to z_coin code so will recheck that this week