Adding multisig wallet #354
Conversation
Extracting root account interface so that it can be substituted by multisig wallet classes
Extracting account interface so that it can be substituted by multisig wallet classes
WalletManager should prefer dealing with interface based objects , instead of concrete classes so that multiple variations of account and etc can be used
Refactoring to use interfaces instead of concrete classes.
Added multisig wallet creation test
…ll to be backward compatible.
…e key deriviation.
… was reduced to just readonly multisig wallet.
…mbine signatures.
|
Excellent job, any chance you can add a step by step how to make a multisig between 3 participants? |
|
Created a small issue documenting the process as requested. #357 please let me know if you have questions/comments on things that were not sufficiently clear. |
rodendron-xrhodium
changed the title
|
I will try to review this today |
There was a problem hiding this comment.
My first review.
I think my main concern is not to make too much changes to the current wallet mainly because all though we have many tests there is still so much complexity involved, the wallet has grown really big and we should try to break components out not add to it, the best would be if this would be its own feature though I am aware it will probably be harder to implement and more work.
We should still consider this because breaking anything in the wallet can effect many chains, staking, cold staking and mining.
| /// <returns>(string) Hex string of the transaction</returns> | ||
| [ActionName("createrawtransaction")] | ||
| [ActionDescription("Create a transaction spending the given inputs and creating new outputs. Outputs can be addresses or data. Returns hex - encoded raw transaction. Note that the transaction's inputs are not signed, and it is not stored in the wallet or transmitted to the network.")] | ||
| public IActionResult CreateRawTransaction(string inputs, string outputs) |
There was a problem hiding this comment.
why use strings instead of an object?
| @@ -151,6 +154,239 @@ public async Task<uint256> SendToAddressAsync(BitcoinAddress address, decimal am | |||
| } | |||
| } | |||
|
|
|||
| /// <summary> | |||
There was a problem hiding this comment.
I would suggest to create a new controller for the multisig methods
| if (wallet.IsMultisig) | ||
| { | ||
| // Add keys for signing inputs. This takes time so only add keys for distinct addresses. | ||
| foreach (UnspentOutputReference unspentOutput in this.walletManager.GetSpendableTransactionsInWallet(accountReference.WalletName, 1)) |
There was a problem hiding this comment.
this will take all spendable outputs is that the intention? don't you want to get outptus by amount?
I assume this method started the multisig processes and populates inputs, it looks like you just take all outputs from a wallet
|
|
||
| [ActionName("combinemultisigsignatures")] | ||
| [ActionDescription("Combines multiple signed (same) transctions into 1 properly signed transaction")] | ||
| public IActionResult CombineMultisigSignatures(string transactions) |
There was a problem hiding this comment.
I think its a very good idea to look at PSBT even if we need to add a reference to the current NBitcoin repo (they have PSBT) that will really simplify moving trx between clients
| return this.Json(wallet); | ||
| } | ||
|
|
||
| [ActionName("combinemultisigsignatures")] |
There was a problem hiding this comment.
you are missing comments here so its hard to know what is the intention of this method
| @@ -55,6 +55,7 @@ public class WalletManager : IWalletManager | |||
| /// <summary>File extension for wallet files.</summary> | |||
| private const string WalletFileExtension = "wallet.json"; | |||
|
|
|||
| private const string WalletMultisigFileExtension = "multisigwallet.json"; | |||
There was a problem hiding this comment.
interesting approach, so we will have a new wallet file for multisig wallets hmmm...
Perhaps this is a good solution that has the least changes made to the current wallet code (without multisig)
We have to also remember that cold staking is doing some method overrides in the cold stake feature
|
|
||
| return wallet; | ||
| } | ||
| private static string GetWalletFileName(WalletMultisig wallet) |
|
|
||
| /// <summary>Filter for all wallet accounts.</summary> | ||
| public static Func<HdAccount, bool> AllAccounts = a => true; | ||
| public static Func<IHdAccount, bool> AllAccounts = a => true; |
There was a problem hiding this comment.
Honestly this PR would be much easier to review if the interfaces over account and address was not created, is that really necessary?
| @@ -29,6 +29,7 @@ | |||
| <ItemGroup> | |||
| <ProjectReference Include="..\..\Features\Blockcore.Features.Wallet\Blockcore.Features.Wallet.csproj" /> | |||
| <ProjectReference Include="..\..\Features\Blockcore.Features.RPC\Blockcore.Features.RPC.csproj" /> | |||
| <ProjectReference Include="..\..\Networks\Blockcore.Networks.XRC\Blockcore.Networks.XRC.csproj" /> | |||
There was a problem hiding this comment.
Adding a specific network is the wrong approach, it should be generic, network agnostic
| [Fact] | ||
| public void Generate4SequnetialMultisigRecievingAndChangeAddress() | ||
| { | ||
| string[] receiving = { "rbAxG3vTMCuVMWppaobvTajBtUHSiFtkr5", "roLKXofxDFrkZqW7kU9aD7j7E6pTajEe16", "rjmcR79w6MatNK3KeHR3FvaEEHngdAKa9h", "reD6MyXPFUaJpyBtJVDgdYf7iN4qHbhzBz" }; |
There was a problem hiding this comment.
Ah you added the network in order to use the address format for XRC ok that is acceptable
|
Anyone still working on this PR? Would be great to have multisig available. |
|
@dimsavva this is a complicated PR hopefully we will get there |
|
Is there any way that I can help? |
Yes, test and verify and write some easy instructions on how to replicate / setup so I can also test it. We also need some instructions for users that could be added to the documentation repo. |
This should provide multisig wallet features: