feat: require feePayer account before tx serialization (#12109)

* feat: require feePayer account before tx serialization

* feat: add setSigners method

* feat: rename signPartial to partialSign

web3.js/module.d.ts

@@ -623,6 +623,7 @@ declare module '@solana/web3.js' {
     instructions: Array<TransactionInstruction>;
     recentBlockhash?: Blockhash;
     nonceInfo?: NonceInformation;
+    feePayer: PublicKey | null;
 
     constructor(opts?: TransactionCtorFields);
     static from(buffer: Buffer | Uint8Array | Array<number>): Transaction;
@@ -635,9 +636,9 @@ declare module '@solana/web3.js' {
     compileMessage(): Message;
     serializeMessage(): Buffer;
     sign(...signers: Array<Account>): void;
-    signPartial(...partialSigners: Array<PublicKey | Account>): void;
+    partialSign(...partialSigners: Array<Account>): void;
     addSignature(pubkey: PublicKey, signature: Buffer): void;
-    addSigner(signer: Account): void;
+    setSigners(...signer: Array<PublicKey>): void;
     verifySignatures(): boolean;
     serialize(): Buffer;
   }

web3.js/module.flow.js

@@ -12,6 +12,7 @@
 
 import {Buffer} from 'buffer';
 import * as BufferLayout from 'buffer-layout';
+import {PublicKey} from './src/publickey';
 
 declare module '@solana/web3.js' {
   // === src/publickey.js ===
@@ -626,6 +627,7 @@ declare module '@solana/web3.js' {
     instructions: Array<TransactionInstruction>;
     recentBlockhash: ?Blockhash;
     nonceInfo: ?NonceInformation;
+    feePayer: PublicKey | null;
 
     constructor(opts?: TransactionCtorFields): Transaction;
     static from(buffer: Buffer | Uint8Array | Array<number>): Transaction;
@@ -638,9 +640,9 @@ declare module '@solana/web3.js' {
     compileMessage(): Message;
     serializeMessage(): Buffer;
     sign(...signers: Array<Account>): void;
-    signPartial(...partialSigners: Array<PublicKey | Account>): void;
+    partialSign(...partialSigners: Array<Account>): void;
-    addSigner(signer: Account): void;
     addSignature(pubkey: PublicKey, signature: Buffer): void;
+    setSigners(...signers: Array<PublicKey>): void;
     verifySignatures(): boolean;
     serialize(): Buffer;
   }

web3.js/src/message.js

@@ -13,7 +13,8 @@ import * as shortvec from './util/shortvec-encoding';
  * The message header, identifying signed and read-only account
  *
  * @typedef {Object} MessageHeader
- * @property {number} numRequiredSignatures The number of signatures required for this message to be considered valid
+ * @property {number} numRequiredSignatures The number of signatures required for this message to be considered valid. The
+ * signatures must match the first `numRequiredSignatures` of `accountKeys`.
  * @property {number} numReadonlySignedAccounts: The last `numReadonlySignedAccounts` of the signed keys are read-only accounts
  * @property {number} numReadonlyUnsignedAccounts The last `numReadonlySignedAccounts` of the unsigned keys are read-only accounts
  */

web3.js/src/transaction.js

@@ -141,6 +141,16 @@ export class Transaction {
     return null;
   }
 
+  /**
+   * The transaction fee payer (first signer)
+   */
+  get feePayer(): PublicKey | null {
+    if (this.signatures.length > 0) {
+      return this.signatures[0].publicKey;
+    }
+    return null;
+  }
+
   /**
    * The instructions to atomically execute
    */
@@ -206,6 +216,10 @@ export class Transaction {
       throw new Error('No instructions provided');
     }
 
+    if (this.feePayer === null) {
+      throw new Error('Transaction feePayer required');
+    }
+
     let numReadonlySignedAccounts = 0;
     let numReadonlyUnsignedAccounts = 0;
 
@@ -231,8 +245,6 @@ export class Transaction {
       });
     });
 
-    // Prefix accountMetas with feePayer here whenever that gets implemented
-
     // Sort. Prioritizing first by signer, then by writable
     accountMetas.sort(function (x, y) {
       const checkSigner = x.isSigner === y.isSigner ? 0 : x.isSigner ? -1 : 1;
@@ -256,23 +268,36 @@ export class Transaction {
       }
     });
 
-    this.signatures.forEach(signature => {
+    // Move payer to the front and append other unknown signers as read-only
-      const sigPubkeyString = signature.publicKey.toString();
+    this.signatures.forEach((signature, signatureIndex) => {
+      const isPayer = signatureIndex === 0;
       const uniqueIndex = uniqueMetas.findIndex(x => {
-        return x.pubkey.toString() === sigPubkeyString;
+        return x.pubkey.equals(signature.publicKey);
       });
       if (uniqueIndex > -1) {
-        uniqueMetas[uniqueIndex].isSigner = true;
+        if (isPayer && uniqueIndex !== 0) {
-      } else {
+          const [payerMeta] = uniqueMetas.splice(uniqueIndex, 1);
+          payerMeta.isSigner = true;
+          uniqueMetas.unshift(payerMeta);
+        } else {
+          uniqueMetas[uniqueIndex].isSigner = true;
+        }
+      } else if (isPayer) {
         uniqueMetas.unshift({
-          pubkey: new PublicKey(sigPubkeyString),
+          pubkey: signature.publicKey,
           isSigner: true,
           isWritable: true,
         });
+      } else {
+        uniqueMetas.push({
+          pubkey: signature.publicKey,
+          isSigner: true,
+          isWritable: false,
+        });
       }
     });
 
-    // Split out signing from nonsigning keys and count readonlys
+    // Split out signing from non-signing keys and count read-only keys
     const signedKeys: string[] = [];
     const unsignedKeys: string[] = [];
     uniqueMetas.forEach(({pubkey, isSigner, isWritable}) => {
@@ -291,15 +316,6 @@ export class Transaction {
       }
     });
 
-    // Initialize signature array, if needed
-    if (this.signatures.length === 0) {
-      const signatures: Array<SignaturePubkeyPair> = [];
-      signedKeys.forEach(pubkey => {
-        signatures.push({signature: null, publicKey: new PublicKey(pubkey)});
-      });
-      this.signatures = signatures;
-    }
-
     const accountKeys = signedKeys.concat(unsignedKeys);
     const instructions: CompiledInstruction[] = this.instructions.map(
       instruction => {
@@ -321,7 +337,7 @@ export class Transaction {
 
     return new Message({
       header: {
-        numRequiredSignatures: this.signatures.length,
+        numRequiredSignatures: signedKeys.length,
         numReadonlySignedAccounts,
         numReadonlyUnsignedAccounts,
       },
@@ -339,9 +355,24 @@ export class Transaction {
   }
 
   /**
-   * Sign the Transaction with the specified accounts.  Multiple signatures may
+   * Specify the public keys which will be used to sign the Transaction.
+   * The first signer will be used as the transaction fee payer account.
+   *
+   * Signatures can be added with either `partialSign` or `addSignature`
+   */
+  setSigners(...signers: Array<PublicKey>) {
+    if (signers.length === 0) {
+      throw new Error('No signers');
+    }
+
+    this.signatures = signers.map(publicKey => ({signature: null, publicKey}));
+  }
+
+  /**
+   * Sign the Transaction with the specified accounts. Multiple signatures may
    * be applied to a Transaction. The first signature is considered "primary"
-   * and is used when testing for Transaction confirmation.
+   * and is used when testing for Transaction confirmation. The first signer
+   * will be used as the transaction fee payer account.
    *
    * Transaction fields should not be modified after the first call to `sign`,
    * as doing so may invalidate the signature and cause the Transaction to be
@@ -350,70 +381,39 @@ export class Transaction {
    * The Transaction must be assigned a valid `recentBlockhash` before invoking this method
    */
   sign(...signers: Array<Account>) {
-    this.signPartial(...signers);
+    if (signers.length === 0) {
-  }
-
-  /**
-   * Partially sign a Transaction with the specified accounts.  The `Account`
-   * inputs will be used to sign the Transaction immediately, while any
-   * `PublicKey` inputs will be referenced in the signed Transaction but need to
-   * be filled in later by calling `addSigner()` with the matching `Account`.
-   *
-   * All the caveats from the `sign` method apply to `signPartial`
-   */
-  signPartial(...partialSigners: Array<PublicKey | Account>) {
-    if (partialSigners.length === 0) {
       throw new Error('No signers');
     }
 
-    function partialSignerPublicKey(accountOrPublicKey: any): PublicKey {
+    this.signatures = signers.map(signer => ({
-      if ('publicKey' in accountOrPublicKey) {
+      signature: null,
-        return accountOrPublicKey.publicKey;
+      publicKey: signer.publicKey,
-      }
+    }));
-      return accountOrPublicKey;
+
+    this.partialSign(...signers);
+  }
+
+  /**
+   * Partially sign a transaction with the specified accounts. All accounts must
+   * correspond to a public key that was previously provided to `setSigners`.
+   *
+   * All the caveats from the `sign` method apply to `partialSign`
+   */
+  partialSign(...signers: Array<Account>) {
+    if (signers.length === 0) {
+      throw new Error('No signers');
     }
 
-    function signerAccount(accountOrPublicKey: any): ?Account {
-      if (
-        'publicKey' in accountOrPublicKey &&
-        'secretKey' in accountOrPublicKey
-      ) {
-        return accountOrPublicKey;
-      }
-    }
-
-    const signatures: Array<SignaturePubkeyPair> = partialSigners.map(
-      accountOrPublicKey => ({
-        signature: null,
-        publicKey: partialSignerPublicKey(accountOrPublicKey),
-      }),
-    );
-    this.signatures = signatures;
     const signData = this.serializeMessage();
-
+    signers.forEach(signer => {
-    partialSigners.forEach((accountOrPublicKey, index) => {
+      const signature = nacl.sign.detached(signData, signer.secretKey);
-      const account = signerAccount(accountOrPublicKey);
+      this.addSignature(signer.publicKey, signature);
-      if (account) {
-        const signature = nacl.sign.detached(signData, account.secretKey);
-        invariant(signature.length === 64);
-        signatures[index].signature = Buffer.from(signature);
-      }
     });
   }
 
   /**
-   * Fill in a signature for a partially signed Transaction.  The `signer` must
+   * Add an externally created signature to a transaction. The public key
-   * be the corresponding `Account` for a `PublicKey` that was previously provided to
+   * must correspond to a public key that was previously provided to `setSigners`.
-   * `signPartial`
-   */
-  addSigner(signer: Account) {
-    const signData = this.serializeMessage();
-    const signature = nacl.sign.detached(signData, signer.secretKey);
-    this.addSignature(signer.publicKey, signature);
-  }
-
-  /**
-   * Add an externally created signature to a transaction
    */
   addSignature(pubkey: PublicKey, signature: Buffer) {
     invariant(signature.length === 64);

web3.js/test/bpf-loader.test.js

@@ -157,6 +157,7 @@ describe('load BPF Rust program', () => {
       programId: program.publicKey,
     });
 
+    simulatedTransaction.setSigners(payerAccount.publicKey);
     const {err, logs} = (
       await connection.simulateTransaction(simulatedTransaction)
     ).value;
@@ -182,6 +183,7 @@ describe('load BPF Rust program', () => {
       programId: new Account().publicKey,
     });
 
+    simulatedTransaction.setSigners(payerAccount.publicKey);
     const {err, logs} = (
       await connection.simulateTransaction(simulatedTransaction)
     ).value;
@@ -206,7 +208,13 @@ describe('load BPF Rust program', () => {
     const {err, logs} = (
       await connection.simulateTransaction(simulatedTransaction, [program])
     ).value;
-    expect(err).toEqual('SignatureFailure');
+    expect(err).toEqual('SanitizeFailure');
-    expect(logs).toBeNull();
+
+    if (logs === null) {
+      expect(logs).not.toBeNull();
+      return;
+    }
+
+    expect(logs.length).toEqual(0);
   });
 });

web3.js/test/transaction.test.js

@@ -9,7 +9,73 @@ import {StakeProgram} from '../src/stake-program';
 import {SystemProgram} from '../src/system-program';
 import {Message} from '../src/message';
 
-test('signPartial', () => {
+describe('compileMessage', () => {
+  test('payer is first account meta', () => {
+    const payer = new Account();
+    const other = new Account();
+    const recentBlockhash = new Account().publicKey.toBase58();
+    const programId = new Account().publicKey;
+    const transaction = new Transaction({recentBlockhash}).add({
+      keys: [
+        {pubkey: other.publicKey, isSigner: true, isWritable: true},
+        {pubkey: payer.publicKey, isSigner: true, isWritable: true},
+      ],
+      programId,
+    });
+
+    transaction.sign(payer, other);
+    const message = transaction.compileMessage();
+    expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true);
+    expect(message.accountKeys[1].equals(other.publicKey)).toBe(true);
+    expect(message.header.numRequiredSignatures).toEqual(2);
+    expect(message.header.numReadonlySignedAccounts).toEqual(0);
+    expect(message.header.numReadonlyUnsignedAccounts).toEqual(1);
+  });
+
+  test('payer is writable', () => {
+    const payer = new Account();
+    const recentBlockhash = new Account().publicKey.toBase58();
+    const programId = new Account().publicKey;
+    const transaction = new Transaction({recentBlockhash}).add({
+      keys: [{pubkey: payer.publicKey, isSigner: true, isWritable: false}],
+      programId,
+    });
+
+    transaction.sign(payer);
+    const message = transaction.compileMessage();
+    expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true);
+    expect(message.header.numRequiredSignatures).toEqual(1);
+    expect(message.header.numReadonlySignedAccounts).toEqual(0);
+    expect(message.header.numReadonlyUnsignedAccounts).toEqual(1);
+  });
+
+  test('signers are ordered and only writable if necessary', () => {
+    const payer = new Account();
+    const account1 = new Account();
+    const account2 = new Account();
+    const recentBlockhash = new Account().publicKey.toBase58();
+    const programId = new Account().publicKey;
+    const transaction = new Transaction({recentBlockhash}).add({
+      keys: [
+        {pubkey: payer.publicKey, isSigner: true, isWritable: false},
+        {pubkey: account1.publicKey, isSigner: false, isWritable: true},
+      ],
+      programId,
+    });
+
+    // account2 is an extra signer, not involved in any instructions
+    transaction.sign(payer, account1, account2);
+    const message = transaction.compileMessage();
+    expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true);
+    expect(message.accountKeys[1].equals(account1.publicKey)).toBe(true);
+    expect(message.accountKeys[2].equals(account2.publicKey)).toBe(true);
+    expect(message.header.numRequiredSignatures).toEqual(3);
+    expect(message.header.numReadonlySignedAccounts).toEqual(1);
+    expect(message.header.numReadonlyUnsignedAccounts).toEqual(1);
+  });
+});
+
+test('partialSign', () => {
   const account1 = new Account();
   const account2 = new Account();
   const recentBlockhash = account1.publicKey.toBase58(); // Fake recentBlockhash
@@ -23,9 +89,10 @@ test('signPartial', () => {
   transaction.sign(account1, account2);
 
   const partialTransaction = new Transaction({recentBlockhash}).add(transfer);
-  partialTransaction.signPartial(account1, account2.publicKey);
+  partialTransaction.setSigners(account1.publicKey, account2.publicKey);
+  expect(partialTransaction.signatures[0].signature).toBeNull();
   expect(partialTransaction.signatures[1].signature).toBeNull();
-  partialTransaction.addSigner(account2);
+  partialTransaction.partialSign(account1, account2);
 
   expect(partialTransaction).toEqual(transaction);
 });
@@ -217,16 +284,23 @@ test('serialize unsigned transaction', () => {
   expect(() => {
     expectedTransaction.serialize();
   }).toThrow(Error);
-  expect(expectedTransaction.signatures.length).toBe(0);
+  expect(() => {
-  // Signature array populated with null signatures fails.
+    expectedTransaction.serializeMessage();
-  expectedTransaction.serializeMessage();
+  }).toThrow('Transaction feePayer required');
+
+  expectedTransaction.setSigners(sender.publicKey);
   expect(expectedTransaction.signatures.length).toBe(1);
+
+  // Signature array populated with null signatures fails.
   expect(() => {
     expectedTransaction.serialize();
   }).toThrow(Error);
-  expect(expectedTransaction.signatures.length).toBe(1);
+
+  // Serializing the message is allowed when signature array has null signatures
+  expectedTransaction.serializeMessage();
+
   // Properly signed transaction succeeds
-  expectedTransaction.sign(sender);
+  expectedTransaction.partialSign(sender);
   expect(expectedTransaction.signatures.length).toBe(1);
   const expectedSerialization = Buffer.from(
     'AVuErQHaXv0SG0/PchunfxHKt8wMRfMZzqV0tkC5qO6owYxWU2v871AoWywGoFQr4z+q/7mE8lIufNl/' +
@@ -254,6 +328,7 @@ test('externally signed stake delegate', () => {
   });
   const from = authority;
   tx.recentBlockhash = bs58.encode(recentBlockhash);
+  tx.setSigners(from.publicKey);
   const tx_bytes = tx.serializeMessage();
   const signature = nacl.sign.detached(tx_bytes, from.secretKey);
   tx.addSignature(from.publicKey, signature);