Add Multisignture support #76

jackcmay · 2020-07-02T07:13:48Z

Downstream users would like to be able to ultilize multisignature on Token Accounts. A Multisig program would be an option down the road, when cross-program invocations are enabled.
In the meantime, the shortest path seems to be to add M-of-N multisig support in Token.

Instead of just one account owner, enable configuration of each Token account to have N authorities (1 <= N <= 11) and M required signers for transfers and updates to authorities and num-required-signers.

WIP TODO:

More tests
Update js bindings

Fixes: #49

jackcmay · 2020-07-02T07:17:38Z

@mvines @CriesofCarrots First stab, thoughts?

mvines · 2020-07-02T15:40:31Z

token/src/instruction.rs

@@ -8,6 +8,11 @@ use solana_sdk::{
 };
 use std::mem::size_of;

+/// Maximum number of multisignature signers (max N)
+pub const MAX_SIGNERS: usize = 11;
+/// Minimum number of multisignature signers (max N)


maybe this?

Suggested change

/// Minimum number of multisignature signers (max N)

/// Minimum number of multisignature signers (min N)

mvines · 2020-07-02T15:42:43Z

token/src/instruction.rs

+    ///
+    /// # Accounts expected by this instruction:
+    ///
+    ///   0. `[signer]` Multisignature account to initialize


I don't think we need this account to sign (InitializeAccount/Mint eventually too). Same logic as #67 (comment). usecase: if the account is a derived address, that address literally can't be a signer

Yeah, was thinking about that and planning to address that together with the other instructions as a separate change

mvines · 2020-07-02T15:45:24Z

token/src/instruction.rs

@@ -51,6 +66,7 @@ pub enum TokenInstruction {
    ///   1. `[writable]` Source/Delegate account.
    ///   2. `[writable]` Destination account.
    ///   3. Optional: `[writable]` Source account if key 1 is a delegate account.
+    ///   4-14. Optional: `[Signer]` M multisignature Signer accounts


Doesn't the 0. [signer] Owner of the source account. account disappear?
The single owner concept is implicit 1x1 multisigner account, so it could just be included as the first multisignature Signer account.

That 3. Optional: [writable] Source account if key 1 is a delegate account. account is now clunky. I guess it goes away entirely when we drop from supporting N delegate accounts to just 1.

I was planning to keep the single owner support as is which has a shorter code path and less required keys and les user steps then having to implore the multi-sig route for everyone.

Yeah, I thought clunky too, and left it to get suggestions. It would go away if we do single delegate but it's indicative of a clunkiness we will have with other programs in the future especially one's that support a group of optional sigs as multisig support does.

Keeping single owner support makes sense to me. I'd think that from the interface perspective, using a single owner keypair would look identical to using a Multisig account configured at 1x1

Hmm, it looks like with this approach, the multisig account itself always has to be a signer (in order for it to appear as the first account in the metas list and be marked as the owner). That doesn't seem right to me.

Should the accounts maybe be something more like this?

* (Multiple) `[signer]` Owner of the source account or M multisignature Signer accounts 1. `[writable]` Source/Delegate account. 2. `[writable]` Destination account. 3. (Optional) `[writable]` Source account if key 1 is a delegate account. 4. (Optional) `[]` Multisignature data account

I was thinking:

/// 0. `[writable]` Source/Delegate account. /// 1. `[writable]` Destination account. /// 2. Optional: `[writable]` Source account if key 0 is a delegate account. /// 2-12, or 3-13. `[Signer]` M multisignature Signer accounts (1 if a single owner address)

or

/// 0. `[writable]` Source/Delegate account. /// 1. `[writable]` Destination account. /// 2-M. `[Signer]` M multisignature Signer accounts (M=1 if a single owner address) /// M+1. Optional: `[writable]` Source account if key 0 is a delegate account.

Oh wait that doesn't work

Yeah, so the complication of storing the multisig data in a separate account is that its address has to be passed in every time

Assuming we drop the N delegate support, and each token only supports one delegate:

Single owner/delegate:

/// 0. `[writable]` Source account. /// 1. `[writable]` Destination account. /// 2. `[Signer]` Owner/delegate address

Multisig owner/delegate:

/// 0. `[writable]` Source account. /// 1. `[writable]` Destination account. /// 2. `[]` Multisig account /// 3-M. `[Signer]` M multisignature Signer accounts

The single delegate simplifies things a lot. I like your suggestion @mvines, removing the "optional" from the docs makes things more clear. For Multisig owner/delegate we should to clarify that the multisig account can either be the owner or the delegate multisig account since the behavior is different between the two
2. '[]' Owner/delegate Multisig account

mvines · 2020-07-02T15:47:38Z

token/src/instruction.rs

+    ///
+    ///   0. `[signer]` Multisignature account to initialize
+    ///   1-11. `[]` Signer accounts, must equal to N where 1 <= N <= 11
+    InitializeMultisig(u8),


wdyt about including N in the instruction data:

Suggested change

InitializeMultisig(u8),

InitializeMultisig {n: u8, m: u8},

N is implicit by the number of signer accounts provided but for the cost of an extra 8 bits, adding an explicit N seems to be more readable

If we do it's adding a possible failure point and more work internally to check that the user didn't mess it up (though on the other hand, it does add redundancy to make sure the user included the intended number of singers to the instruction

mvines · 2020-07-02T15:52:33Z

token/src/instruction.rs

@@ -68,6 +85,7 @@ pub enum TokenInstruction {
    ///   0. `[signer]` Current owner of the token or account.
    ///   1. `[writable]` token or account to change the owner of.
    ///   2. `[]` New owner


Document that the new owner can be a Multisig account?

Yeah, I debated that, the InitializeMultisig instruction documentation mentions that the multisig can take the place of any owner, I was avoiding adding a bunch of duplicate verbiage. I'm not opposed to adding it if you think it's worth it.

Ah, I guess we just need one location that says, "owner could be either a multisig address, in which case M of the N signers must be present, or any other address where the matching signer must be present"

jackcmay · 2020-07-02T16:48:13Z

@mvines In general, what do you think of this approach? In particular, identifying the multisig account type by owner and size? Still think we should apply that to Mint?

mvines · 2020-07-02T16:53:09Z

what do you think of this approach? In particular, identifying the multisig account type by owner and size? Still think we should apply that to Mint?

sgtm. Yeah, I feel like the Mint should support multisig too. MintTo is a superuser command, great use case for multisig

jackcmay · 2020-07-02T18:44:19Z

@mvines

sgtm. Yeah, I feel like the Mint should support multisig too. MintTo is a superuser command, great use case for multisig

Mint already supports multisignature since its owner can be a multisignature account. Later SetOwner and MintTo will use multisignature to validate.

My question was what do you think about moving away from enum for Mint and to a standalone structure like is proposed here for MultiSig?

mvines · 2020-07-02T18:52:32Z

My question was what do you think about moving away from enum for Mint and to a standalone structure like is proposed here for MultiSig?

oh sry. Yep I think so. Ditch the enum entirely. It's weird to use it for some but not all the token accounts

jackcmay · 2020-07-04T00:08:14Z

@mvines @CriesofCarrots

This PR is rendered obsolete by these changes: jackcmay#1

Add Multisignture support

ed2c681

jackcmay added the WIP Work in progress label Jul 2, 2020

jackcmay added 2 commits July 2, 2020 00:21

only check is_signer is need to

df2e545

Remove js testing from CI for now

2f5cddd

mvines reviewed Jul 2, 2020

View reviewed changes

jackcmay mentioned this pull request Jul 4, 2020

Add multisignature support jackcmay/solana-program-library#1

Open

jackcmay closed this Jul 4, 2020

jackcmay deleted the add-multisig-support branch July 6, 2020 14:39

jackcmay mentioned this pull request Jul 6, 2020

Add multisig support #79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Multisignture support #76

Add Multisignture support #76

jackcmay commented Jul 2, 2020 •

edited

Loading

jackcmay commented Jul 2, 2020

mvines Jul 2, 2020

mvines Jul 2, 2020

jackcmay Jul 2, 2020

mvines Jul 2, 2020

jackcmay Jul 2, 2020 •

edited

Loading

mvines Jul 2, 2020

CriesofCarrots Jul 2, 2020

CriesofCarrots Jul 2, 2020

mvines Jul 2, 2020 •

edited

Loading

mvines Jul 2, 2020

CriesofCarrots Jul 2, 2020

mvines Jul 2, 2020

jackcmay Jul 2, 2020

mvines Jul 2, 2020

jackcmay Jul 2, 2020

mvines Jul 2, 2020

jackcmay Jul 2, 2020

mvines Jul 2, 2020

jackcmay commented Jul 2, 2020

mvines commented Jul 2, 2020

jackcmay commented Jul 2, 2020

mvines commented Jul 2, 2020

jackcmay commented Jul 4, 2020

	/// Minimum number of multisignature signers (max N)
	/// Minimum number of multisignature signers (min N)

Add Multisignture support #76

Add Multisignture support #76

Conversation

jackcmay commented Jul 2, 2020 • edited Loading

jackcmay commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackcmay Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvines Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackcmay commented Jul 2, 2020

mvines commented Jul 2, 2020

jackcmay commented Jul 2, 2020

mvines commented Jul 2, 2020

jackcmay commented Jul 4, 2020

jackcmay commented Jul 2, 2020 •

edited

Loading

jackcmay Jul 2, 2020 •

edited

Loading

mvines Jul 2, 2020 •

edited

Loading