interface proposal

[mickys]

I've updated the PR with a few lint changes and added a GenericTransaction interface, some changes to transaction creation and signing.

[mickys]

Comments on existing implementation:

New Wallets:

• [OK] - new Wallet() -> just instantiates a new mnemonic wallet
  • [OK] - new Wallet("word1 word2 ... wordN");
  • [Missing] - Wallet.fromPrivateKey( new Buffer(privateKey) );
  • [Missing] - Wallet.fromPublicKey( new Buffer(privateKey) ); // needed for hardware wallets

Transaction objects:

• i've added a generic / ethereum and zilliqa transaction objects to be used as templates for creating new transactions.
• nonces need to be incremented thus read from the blockchain using an async method, should probably be done right before signing to minimise nonce collision Ethereum uses getTransactionCount(address) vs Zilliqa just returns it in the getBalance(address) methods
• we also need a "sendCancelTransaction" which forces last nonce for a zero value transaction ie. send

  { 
  value: 0, // use amount on Zilliqa
  to: 0x0,
  nonce: last // < populate this
  }

• different fields for VALUE transmission. Ethereum uses "value", Zilliqa uses "amount", we need a generic "sendValue(to, amount)" method that generates the correct transaction.

Questions

There are a couple of things in the interface proposal that do not really make sense to me: * Probably because i've been thinking about the extension.**

1 - Wallets storing the level 0 root hd key / mnemonic

* Ive done the same implementation in the first few commits of this repo and it creates issues when supporting multiple coins.**

• user might want to import an ethereum mnemonic, and a different one for zilliqa. not supported unless you create and manage multiple different wallets, in which case why tell every node related method the blockchain type ?!

  • instead create typed wallets that already access the correct utilities.
• why would the UI even want to manage this?

2. Transaction signing

should be a method on the wallet object itself, in order to be able to handle hardware wallets ( init signing request ), else you just end up calling "node.sign"..

• We could even allow hardware signing inside the extension.

3. Account network switching.

You should not be able to "switch an account" from a main network to the Test network or other type of network. Instead you can change the SERVER RPC URL you're sending transactions to.

• this should be a method on the "wallet" that allows you to add a custom url for this wallet ( that already knows what node type it has )

4. Test network accounts and HD paths.

• All Test networks use mnemonic derivation path ID 1 ( https://github.com/satoshilabs/slips/blob/master/slip-0044.md )
• Derived accounts are bound by network! This means other wallets that import this mnemonic, will generate different keys if you just switched from live to test

---

Initial implementation in master branch:

My idea was to have one "core" object that the UI instantiates, sends "stored option data" to, then talks to when it needs to.

• that core handles multiple wallets of any type
• each wallet is bound to a network and cannot be changed
• each wallet exposes accounts it stores ( addresses and types ) and data about them ( nonce / balance )
• each wallet then allows transaction creation / signing / sending, based on it's network settings.
• networks can have their URLS updated ( if you want to connect to a different RPC server )
  • you can connect a main "network account" to a "test RPC" but transactions will fail because of the replay protection in transaction creation

In the end, the UI package just needs to be able to parse an object containing the following:

[
    {
        address:0x01,
        type: "ETH",
        network: 1, // Main
        symbol: "ETH",
        symbolMultiple: "ETH",
        amount: 10, // in ETH
        decimals: 18,
        walletType: unlinked ( that means it was created using a privateKey )
        // nonce should probably stay internal as you don't really need to display it.
    },
    {
        address:0x02,
        type: "ETH",
        network: 1, // Main
        symbol: "ETH",
        symbolMultiple: "ETH",
        amount: 5, // in ETH
        decimals: 18,
        walletType: mnemonic
    },
    {
        address:0x01,
        type: "ETH",
        network: 4, // Rinkeby
        symbol: "Test ETH",
        symbolMultiple: "Test ETH",
        amount: 1, // in ETH
        decimals: 18,
        walletType: hardware ( that means it was created using a publicKey )
    },
    {
        address:0x01,
        type: "ZIL",
        symbol: "ZIL",
        network: 1, // Main
        symbolMultiple: "zillings",
        amount: 100 // in ZIL
        decimals: 0 // yep they're ZERO, you can't send 0,1
        walletType: mnemonic
    },
    {
        address:0x02,
        type: "ZIL",
        symbol: "ZIL",
        network: 1, // Main
        symbolMultiple: "zillings",
        amount: 100 // in ZIL
        decimals: 0 // yep they're ZERO, you can't send 0,1
        walletType: mnemonic
    },  
]

Based on that data you should be able to show the user

• a list of wallets, grouped by type and network,

• display a list of holdings ( even do price checks )

• have them select which one they want to use and never have to worry they selected a test account on a live network

• when exporting accounts:

  • add an icon to the list based on walletType ( pk / hw / derrived from hd ).
  • just call the export method on the wallet, then present to the user whichever they want.

    Result: {
    pk: string,
    hd: { mnemonic: string, level: number }
    }

Note: Async methods would support both returning promises AND callbacks.

Let me know what you think.

[krisboit]

@mickys thanks for looking into this.

Let me clarify a little the terms, at least how i see them:

• Wallet
  • it's basically an account manager
  • it generates a mnemonic phrase if not given one
  • based on that mnemonic will generate accounts
  • it can have external accounts (accounts that are not created with the wallet mnemonics)
• Account
  • it's an account on blockchain

So with this in mind for Wallet class we don't need fromPrivateKey and fromPubKey methods as they represent a single account and the wallet is a collection of accounts. If you take a quick look in test.ts file i made an example for this usecase:

// add a new account in wallet (an account that is not generated from the mnemonics of the wallet)
const newAccount = new Ethereum.Account("private key...");
wallet.import(newAccount);

Note: For now Account class supports only privateKey and address as params in constructor but we can easily add pubKey

Regarding your questions:

1. Wallets

I don't see would we want to manage multiple mnemonics in a single wallet, it can be done but i think it would be confusing for the user. When i'm saying this i think of the UI, it should be easy for the user, with one mnemonic phrase he can restore all his accounts. If user wants to add other wallets he can add them with the private key, like in metamask.

If someone wants to use our core multiple mnemonics management can be achieved with multiple wallet instances. It's true it need to pass the blockchian param to multiple methods.

2. Transaction signing

Again Wallet is an Account collection it doesn't make sense to have a signTransaction method. Account class will have the capability to sign a transaction.

To have support for hardware wallets:

• create a wallet from address/pubKey
• use hardware wallet to sign the transactio
• post the transaction in blockchain using the node. Note: Hardware wallet integration will not be implemented here, it will be implemented in the UI app. Code example:

  
  let wallet = new Wallet();
  let newAccount = new Ethereum.Account(null, "0x01");
  wallet.import(newAccount);

// logic to sign transaction using hardware wallet let rawTransaction = .....;

wallet.getNode(Blockchain.ETHEREUM).send(rawTransaction);

## 3. Account network switching
`Account` is independent from `Node` class, `Node` class is used to post an `Account` operation on blockchain. So network switching is done at `Node` level. 

I agree to make network property readonly in `Node` but this mean that rpcUrl should be readonly also. With this approach we could have multiple `Node` instances for each blockchain, one for each network. (I'm not a fan of this approach)

## 4. Test network accounts and HD paths
I did a check in Metamask, it seems it generates the same wallets for all networks (main and test). Since this thing is specific to testnets i think we need to debate if we really want to do this (generate network specific accounts), all the wallets i've tested till now generate accounts for mainnet and reuses them for testnet.