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

Add support for multisig wallets. #493

Closed

Conversation

chris-namebase
Copy link

There is definitely work to be done here on the UX side but this is functional.

  • Ability to create/import multisig wallet
  • Support for creating a new transaction to be signed by multiple signers, signing, and broadcast

Also, probably doesn't belong in this PR but using an M1 here and had to upgrade electron-builder to get builds going.

@pinheadmz
Copy link
Contributor

pinheadmz commented Apr 4, 2022

Ok a few big ideas:

1. refactor wallet signing proxies

This is a common pattern in wallet service:

sendOpen = (name) => this._ledgerProxy(
() => this._executeRPC('createopen', [name]),
() => this._executeRPC('sendopen', [name], this.lock),
);

Basically every wallet tx action from the UI gets split into "create" and "create and sign" functions. The actual _ledgerProxy() function is going to get too bloated to start handling all these new configurations so I think a bigger refactor is necessary.

Maybe something like this:

  • change _LedgerProxy() to something else like _walletProxy()
    • change in all (~26) call sites for every create tx, open, bid, reveal, atomic swap, shakedex, everything...
  • _walletProxy() is new function that gets the wallet info (watch-only) and accountinfo (multisig) and then picks either the "creater" or "create-and-signer" function based on that (this way we can also add ledger to a multisig wallet)
    • the existing _ledgerProxy() function gets called from _walletProxy() if it applies
    • a new function _multisigProxy() handles the partial-signing

You can look at how _ledgerProxy() pops open the ledger modal for signing, I think a similar mechanism could work for multisig. The difference is when multisig is done, the TX won't be final or ready for broadcast yet.

2. Ledger

The Ledger app supports multisig! It's protocol is slightly different than regular key spends (you provide a redeem script not just a public key, to the device) but you can see examples: https://github.com/handshake-org/hsd-ledger/blob/s-x-compat/examples/signTransaction-p2sh.js THANK YOU @boymanjor <3

3. multisig dashboard

As discussed last week, the me-signed but un-final transactions can be stored in bobs local db and displayed in a new tab from the wallet sidebar. That way even after a restart a user can retrieve the current in-progress proposal TX and still copy it. partially signed txs could be copyable as hex or downloaded as a file that can be emailed. Any participant should be able to aggregate signatures from partially-signed txs (ie alice can collect signatures from each person and create the final tx. it doesn't necessarily have to go from alice->bob->charlie). Signautres must be placed in the witness in the correct order though, so look out for that.

In addition to editing and storing nicknames for each signer by public key, we can use some tricky wallet tricks to analyze signatures in the TX and determine who has already signed it, and display those nicknames to the user. We can add this later but I think it'd be cool to see that even in the TX history list.

@pinheadmz
Copy link
Contributor

other UI nit: youll need to expand the area for receive addresses ;-)

diff --git a/app/components/ReceiveModal/receive.scss b/app/components/ReceiveModal/receive.scss
index a3316a8..ff4d760 100644
--- a/app/components/ReceiveModal/receive.scss
+++ b/app/components/ReceiveModal/receive.scss
@@ -73,7 +73,6 @@
     border: solid 1.25px rgba($black, 0.1);
     margin: 0rem 0 1.5rem 0;
     border-radius: 8px;
-    width: 350px;
     align-items: center;
     padding: 0.5rem 0.5rem 0.5rem 1rem;
   }
@@ -83,6 +82,7 @@
     font-size: 0.7rem;
     font-family: 'Roboto Mono', monospace;
     font-weight: 500;
+    margin: 5px;
   }
 
   &__disclaimer {

@@ -738,6 +759,17 @@ class WalletService {
},
);

signMessage = async (txJSON, passphrase) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think signTX is a better name for this since its not an arbitrary message the way signMessageWithName is. Although this function may be moved anyway if you refactor the wallet proxy thing.

@@ -101,6 +105,7 @@ export const fetchWallet = () => async (dispatch, getState) => {
balance: accountInfo.balance,
changeDepth: accountInfo.changeDepth,
receiveDepth: accountInfo.receiveDepth,
type: accountInfo.type,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually had a TODO here in my branch:

// TODO: remove all the above crap and just pass account object

It seems tedious to add and verify each inidivual little property of accountInfo -- lets just add the whole damn object to the wallet state. Eventually we can also refactor out address, balance, changeDepth etc... But by chucking the whole object in there we get type (for "multisig") and also accountKey which will be how we can display our own xpub on the multisig dashboard

Comment on lines +175 to +181
{this.props.walletType == 'multisig' ? <NavLink
className="sidebar__action"
to="/multisig"
activeClassName="sidebar__action--selected"
>
{t('headingMultisig')}
</NavLink> : ''}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking this would go first in the sidebar so it is obvious and easy to find. I also think if the wallet is not initialized, nothing else should appear in the sidebar at all.

This is what I had:

   &__disclaimer {
diff --git a/app/components/Sidebar/index.js b/app/components/Sidebar/index.js
index 66de16e..c025008 100644
--- a/app/components/Sidebar/index.js
+++ b/app/components/Sidebar/index.js
@@ -22,6 +22,7 @@ const nodeClient = clientStub(() => require('electron').ipcRenderer);
     walletSync: state.wallet.walletSync,
     walletHeight: state.wallet.walletHeight,
     address: state.wallet.address,
+    accountInfo: state.wallet.accountInfo,
   }),
   dispatch => ({
 
@@ -43,7 +44,8 @@ class Sidebar extends Component {
     walletSync: PropTypes.bool.isRequired,
     walletHeight: PropTypes.number.isRequired,
     network: PropTypes.string.isRequired,
-    address: PropTypes.string.isRequired,
+    address: PropTypes.string,
+    accountInfo: PropTypes.object.isRequired,
   };
 
   static contextType = I18nContext;
@@ -64,14 +66,46 @@ class Sidebar extends Component {
 
   renderNav() {
     const {t} = this.context;
-    const title = this.props.walletWatchOnly
-      ? `Ledger Wallet (${this.props.walletId})`
-      : `Wallet (${this.props.walletId})`;
+    const {watchOnly, type, initialized} = this.props.accountInfo;
+
+    let title = 'Wallet';
+    if (watchOnly)
+      title = 'Ledger Wallet';
+    else if (type === 'multisig')
+      title = 'Multisig Wallet';
+    title += ` (${this.props.walletId})`;
+
+    if (!initialized) {
+      return(
+        <React.Fragment>
+          <div className="sidebar__section">{title}</div>
+          <div className="sidebar__actions">
+            <NavLink
+              className="sidebar__action"
+              to="/multisig"
+              activeClassName="sidebar__action--selected"
+            >
+              ⚠️ Multisig
+            </NavLink>
+          </div>
+      </React.Fragment>
+      );
+    }
 
     return (
       <React.Fragment>
         <div className="sidebar__section">{title}</div>
         <div className="sidebar__actions">
+          {
+            type === 'multisig' &&
+            <NavLink
+              className="sidebar__action"
+              to="/multisig"
+              activeClassName="sidebar__action--selected"
+            >
+              Multisig
+            </NavLink> 
+          }
           <NavLink
             className="sidebar__action"
             to="/account"

Comment on lines +79 to +82
reloadAccount = async () => {
const accountInfo = await walletClient.getAccountInfo();
this.setState({accountInfo: accountInfo, xpubKey: accountInfo.accountKey});
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we'll need this if we put entire accountInfo object in the state (#493 (comment))

@rithvikvibhu rithvikvibhu mentioned this pull request Oct 7, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants