Closed krisboit closed 6 years ago
dappcoder
commented
6 years ago Nice start! Related to blockchain field in Account interfface. I suppose a good modeling would be to move it to the Network class. Then you do not need to maintain the Map<Blockchain, Node> and Map<Blockchain, Map<String, Account>> fields in Wallet. I think it would simplify the interfaces.
krisboit
commented
6 years ago I think blockchain field can be removed from Account, i agree, if we put it in Network, Map<Blockchain, Map<String, Account>> will become Account[], it's more simple.
As for the nodes, i was thinking to have only one instance of node for each blockchain, and pass it to every account. When switching to a new network (let's say from main net to test net) from Wallet we do that and every account will take advantage of the changes.
At first i was thinking to use a singleton pattern for nodes, but if the library would be used in nodejs for an api than it could introduce problems as the same instance would be shared for all requests.
dappcoder
commented
6 years ago Now I see the reason for the dilemma with the blockchain field in Account. In that case, maybe it would make sense to not mix the Network and the Node with Account and Wallet. Just keep them separate. After all, you can have valid accounts and wallets (which are no more than a data structure that manages accounts / keys) without connecting them to the network. Use case. I might want to import mnemonics or a JSON and get all the private keys without caring about the network nodes.
dappcoder
commented
6 years ago Separating them would look a bit less convenient, but cleaner from my perspective. Basically the Wallet is responsible to only deal with accounts offline. What do you think?
namespace CoolWalletSuite {
enum Blockchain {
Ethereum = "ETHEREUM",
Bitcoin = "BITCOIN"
}
type Network = {
blockchain: Blockchain,
name: String,
url: String,
mainNet: Boolean
}
interface Node {
network: Network;
constructor(network: Network);
getNetwork();
setNetwork(network: Network);
getBalance(account: Account);
send(from: Account, toAddress, amount, data);
// sendErc20Token(token, toAddress, amount, data);
signMessage(account: Account, message);
signTransaction(account: Account, transaction);
}
interface Account {
name: String;
blockchain: Blockchain;
address: String;
publicKey: String;
privateKey: String;
constructor(/*node: Node,*/ name, address, publicKey, privateKey);
// All logic needing network connection moved to Node
// getBalance();
// send(toAddress, amount, data);
// sendErc20Token(token, toAddress, amount, data);
// signMessage(message);
// signTransaction(transaction);
}
interface Wallet {
/*nodes: Map<Blockchain, Node>;*/
accounts: Map<Blockchain, Map<String, Account>>;
currentAccount: Account;
getAccounts(): Map<Blockchain, Map<String, Account>>;
getBlockchainAccounts(blockchain: Blockchain): Map<String, Account>;
getCurrentAccount();
setCurrentAccount(blockchain: Blockchain, address: String): Account;
/*setNetwork(blockchain: Blockchain, network: Network);*/
toJson();
import(blockchain, name, address, publicKey, privateKey);
}
interface WalletFactory {
fromMnemonics(mnemonics: String): Wallet;
fromJSON(serializedWallet): Wallet;
}
/*
Usage examples
*/
// ----------------------
// Using Wallet Class
// ----------------------
const wallet = new Wallet();
wallet.import(Blockchain.Ethereum, "ETH 1", null, null, "privateKey");
wallet.import(Blockchain.Ethereum, "ETH 2", null, null, "privateKey");
wallet.import(Blockchain.Bitcoin, "BTC 1", null, null, "privateKey");
let account = wallet.setCurrentAccount(Blockchain.Ethereum, "0x...ETH1 ADDR...");
// do stuff
// Using the Node
let node = new Node(/* network config, from a const */);
node.getBalance(account);
node.signMessage(wallet.getCurrentAccount(), "My cool wallet");
// ----------------------
// Using Wallet Factory
// ----------------------
const wallet = WalletFactory.fromMnemonics("mnemonic phrase");
let accounts = wallet.getAccounts();
// select first ETH account
let ethAccountAddress = accounts.get(Blockchain.Ethereum).values().next().value.address;
let account = wallet.setCurrentAccount(Blockchain.Ethereum, ethAccountAddress);
// do stuff
node.getBalance(account);
node.signMessage(wallet.getCurrentAccount(), "My cool wallet");
// ----------------------
// Single Account
// ----------------------
const node = new Node(/* network config, from a const */);
const account = new Account("ETH", "0xaddress.....", "public key", "private key");
// do stuff
node.getBalance(account);
node.signMessage(account, "My cool wallet");
}Of course, you can add another class on top of these to combine the nodes and the wallet/accounts. And that would serve as a direct mapping from UI to functionality.
krisboit
commented
6 years ago Nice! I like the idea. Let's see other opinions :)
narcis2007
commented
6 years ago Maybe it would be better to get rid of the enum and change it to a class/interface that holds the particular details/custom implementations for it and have a config file that holds these configuration objects for each blockchain, would be easier this way when we decide to add new blockchains than adding if statements based on the blockchain implementation something like:
interface Blockchain {
name: String;
someCustomImplementation();
}
const supportedBlockchains=[...];
@narcis2007 i think Blockchain should remain enum, what you are referring to will be implemented at Node level. Node is an interface and for every blockchain that we integrate we will have a specific node class that implements Node interface. So no ifs, basically i think it's similar with your proposal.
krisboit
commented
6 years ago i will close this issue, please post you comments in the pull request opened by @dappcoder
Hey guys, this is how i see the wallet manager :)
What do you think ?