From e1abb64f41baf7ba32379674aa9a7f9518864c96 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Thu, 10 Sep 2020 14:04:09 +0800 Subject: [PATCH] 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 | 5 +- web3.js/module.flow.js | 6 +- web3.js/src/message.js | 3 +- web3.js/src/transaction.js | 150 +++++++++++++++---------------- web3.js/test/bpf-loader.test.js | 12 ++- web3.js/test/transaction.test.js | 91 +++++++++++++++++-- 6 files changed, 177 insertions(+), 90 deletions(-) diff --git a/web3.js/module.d.ts b/web3.js/module.d.ts index cc6af5956e..597323abde 100644 --- a/web3.js/module.d.ts +++ b/web3.js/module.d.ts @@ -623,6 +623,7 @@ declare module '@solana/web3.js' { instructions: Array; recentBlockhash?: Blockhash; nonceInfo?: NonceInformation; + feePayer: PublicKey | null; constructor(opts?: TransactionCtorFields); static from(buffer: Buffer | Uint8Array | Array): Transaction; @@ -635,9 +636,9 @@ declare module '@solana/web3.js' { compileMessage(): Message; serializeMessage(): Buffer; sign(...signers: Array): void; - signPartial(...partialSigners: Array): void; + partialSign(...partialSigners: Array): void; addSignature(pubkey: PublicKey, signature: Buffer): void; - addSigner(signer: Account): void; + setSigners(...signer: Array): void; verifySignatures(): boolean; serialize(): Buffer; } diff --git a/web3.js/module.flow.js b/web3.js/module.flow.js index 8af735579b..6cfd030eef 100644 --- a/web3.js/module.flow.js +++ b/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; recentBlockhash: ?Blockhash; nonceInfo: ?NonceInformation; + feePayer: PublicKey | null; constructor(opts?: TransactionCtorFields): Transaction; static from(buffer: Buffer | Uint8Array | Array): Transaction; @@ -638,9 +640,9 @@ declare module '@solana/web3.js' { compileMessage(): Message; serializeMessage(): Buffer; sign(...signers: Array): void; - signPartial(...partialSigners: Array): void; - addSigner(signer: Account): void; + partialSign(...partialSigners: Array): void; addSignature(pubkey: PublicKey, signature: Buffer): void; + setSigners(...signers: Array): void; verifySignatures(): boolean; serialize(): Buffer; } diff --git a/web3.js/src/message.js b/web3.js/src/message.js index b9bcbd793c..444381c1ec 100644 --- a/web3.js/src/message.js +++ b/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 */ diff --git a/web3.js/src/transaction.js b/web3.js/src/transaction.js index 3cc82a2213..8b1d08e26f 100644 --- a/web3.js/src/transaction.js +++ b/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 => { - const sigPubkeyString = signature.publicKey.toString(); + // Move payer to the front and append other unknown signers as read-only + 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; - } else { + if (isPayer && uniqueIndex !== 0) { + 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 = []; - 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) { + 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) { - this.signPartial(...signers); - } - - /** - * 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) { - if (partialSigners.length === 0) { + if (signers.length === 0) { throw new Error('No signers'); } - function partialSignerPublicKey(accountOrPublicKey: any): PublicKey { - if ('publicKey' in accountOrPublicKey) { - return accountOrPublicKey.publicKey; - } - return accountOrPublicKey; + this.signatures = signers.map(signer => ({ + signature: null, + publicKey: signer.publicKey, + })); + + 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) { + 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 = partialSigners.map( - accountOrPublicKey => ({ - signature: null, - publicKey: partialSignerPublicKey(accountOrPublicKey), - }), - ); - this.signatures = signatures; const signData = this.serializeMessage(); - - partialSigners.forEach((accountOrPublicKey, index) => { - const account = signerAccount(accountOrPublicKey); - if (account) { - const signature = nacl.sign.detached(signData, account.secretKey); - invariant(signature.length === 64); - signatures[index].signature = Buffer.from(signature); - } + signers.forEach(signer => { + const signature = nacl.sign.detached(signData, signer.secretKey); + this.addSignature(signer.publicKey, signature); }); } /** - * Fill in a signature for a partially signed Transaction. The `signer` must - * be the corresponding `Account` for a `PublicKey` that was previously provided to - * `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 + * Add an externally created signature to a transaction. The public key + * must correspond to a public key that was previously provided to `setSigners`. */ addSignature(pubkey: PublicKey, signature: Buffer) { invariant(signature.length === 64); diff --git a/web3.js/test/bpf-loader.test.js b/web3.js/test/bpf-loader.test.js index 37fe081ed7..d81830cd24 100644 --- a/web3.js/test/bpf-loader.test.js +++ b/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(logs).toBeNull(); + expect(err).toEqual('SanitizeFailure'); + + if (logs === null) { + expect(logs).not.toBeNull(); + return; + } + + expect(logs.length).toEqual(0); }); }); diff --git a/web3.js/test/transaction.test.js b/web3.js/test/transaction.test.js index 4d3332ec46..2748fb4863 100644 --- a/web3.js/test/transaction.test.js +++ b/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); - // Signature array populated with null signatures fails. - expectedTransaction.serializeMessage(); + expect(() => { + 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);