From 7abc5ba241773224d8916e2a69bfba97757801d5 Mon Sep 17 00:00:00 2001 From: Keith Date: Fri, 8 Dec 2023 13:31:35 +0900 Subject: [PATCH] refactor: refactor sign method to allow unidentified lock scripts The sign method is refactored to allow unidentified lock scripts while skipped lock scripts are prevented outside the sign method Notice that skipped lock scripts validation is not adopted when offline-sign a multisig transaction because it's explictly thrown when no private key is found. It may be intended. --- .../src/controllers/offline-sign.ts | 20 ++++-- .../src/services/hardware/hardware.ts | 20 +++++- .../src/services/transaction-sender.ts | 51 +++++++++++---- .../services/tx/transaction-sender.test.ts | 65 +++++++++++++++---- 4 files changed, 124 insertions(+), 32 deletions(-) diff --git a/packages/neuron-wallet/src/controllers/offline-sign.ts b/packages/neuron-wallet/src/controllers/offline-sign.ts index 1ca40ba019..c31bb8865b 100644 --- a/packages/neuron-wallet/src/controllers/offline-sign.ts +++ b/packages/neuron-wallet/src/controllers/offline-sign.ts @@ -94,13 +94,19 @@ export default class OfflineSignController { context ) } else { - tx = await new TransactionSender().sign( - walletID, - Transaction.fromObject(transaction), - password, - type === SignType.SendSUDT, - context - ) + tx = await new TransactionSender() + .sign(walletID, Transaction.fromObject(transaction), password, type === SignType.SendSUDT, context) + .then(({ tx: t, metadata }) => { + // TODO: maybe unidentified inputs can be skipped in offline sign + if (metadata.locks.skipped.size) { + throw new Error( + `Fail to sign transaction, following lock scripts cannot be identified: ${[ + ...metadata.locks.skipped.values(), + ]}` + ) + } + return t + }) } const signer = OfflineSign.fromJSON({ diff --git a/packages/neuron-wallet/src/services/hardware/hardware.ts b/packages/neuron-wallet/src/services/hardware/hardware.ts index 417d8d6dcb..2c5801a4cc 100644 --- a/packages/neuron-wallet/src/services/hardware/hardware.ts +++ b/packages/neuron-wallet/src/services/hardware/hardware.ts @@ -67,6 +67,13 @@ export abstract class Hardware { const lockHashes = new Set(witnessSigningEntries.map(w => w.lockHash)) + const metadata = { + locks: { + skipped: new Set(), + }, + skipLastInput, + } + for (const [index, lockHash] of [...lockHashes].entries()) { DeviceSignIndexSubject.next(index) const witnessesArgs = witnessSigningEntries.filter(w => w.lockHash === lockHash) @@ -75,6 +82,17 @@ export abstract class Hardware { const path = findPath(witnessesArgs[0].lockArgs) + if (!path) { + metadata.locks.skipped.add(lockHash) + witnessSigningEntries.forEach((entry, idx) => { + if (entry.lockHash === lockHash) { + const rawWitness = tx.witnesses[idx] + entry.witness = typeof rawWitness === 'string' ? rawWitness : serializeWitnessArgs(rawWitness) + } + }) + continue + } + if (isMultisig) { const serializedWitnesses = witnessesArgs.map(value => { const args = value.witnessArgs @@ -131,7 +149,7 @@ export abstract class Hardware { tx.witnesses = witnessSigningEntries.map(w => w.witness || '0x') tx.hash = txHash - return tx + return { tx, metadata } } public abstract getPublicKey(path: string): Promise diff --git a/packages/neuron-wallet/src/services/transaction-sender.ts b/packages/neuron-wallet/src/services/transaction-sender.ts index ff2a551f26..614f179351 100644 --- a/packages/neuron-wallet/src/services/transaction-sender.ts +++ b/packages/neuron-wallet/src/services/transaction-sender.ts @@ -72,7 +72,16 @@ export default class TransactionSender { ) { const tx = skipSign ? Transaction.fromObject(transaction) - : await this.sign(walletID, transaction, password, skipLastInput) + : await this.sign(walletID, transaction, password, skipLastInput).then(({ tx, metadata }) => { + if (metadata.locks.skipped.size) { + throw new Error( + `Fail to send transaction, following lock scripts cannot be identified: ${[ + ...metadata.locks.skipped.values(), + ]} ` + ) + } + return tx + }) return this.broadcastTx(walletID, tx) } @@ -115,6 +124,7 @@ export default class TransactionSender { const wallet = this.walletService.get(walletID) const tx = Transaction.fromObject(transaction) const txHash: string = tx.computeHash() + if (wallet.isHardware()) { let device = HardwareWalletService.getInstance().getCurrent() if (!device) { @@ -151,19 +161,15 @@ export default class TransactionSender { const findPrivateKey = (args: string) => { let path: string | undefined if (args.length === TransactionSender.MULTI_SIGN_ARGS_LENGTH) { - path = multiSignBlake160s.find(i => args.slice(0, 42) === i.multiSignBlake160)!.path + path = multiSignBlake160s.find(i => args.slice(0, 42) === i.multiSignBlake160)?.path } else if (args.length === 42) { - path = addressInfos.find(i => i.blake160 === args)!.path + path = addressInfos.find(i => i.blake160 === args)?.path } else { const addressInfo = AssetAccountInfo.findSignPathForCheque(addressInfos, args) path = addressInfo?.path } - const pathAndPrivateKey = pathAndPrivateKeys.find(p => p.path === path) - if (!pathAndPrivateKey) { - throw new Error('no private key found') - } - return pathAndPrivateKey.privateKey + return pathAndPrivateKeys.find(p => p.path === path)?.privateKey } const witnessSigningEntries: SignInfo[] = tx.inputs @@ -183,13 +189,32 @@ export default class TransactionSender { const lockHashes = new Set(witnessSigningEntries.map(w => w.lockHash)) + const metadata = { + locks: { + skipped: new Set(), + }, + skipLastInput, + } + for (const lockHash of lockHashes) { const witnessesArgs = witnessSigningEntries.filter(w => w.lockHash === lockHash) - // A 65-byte empty signature used as placeholder - witnessesArgs[0].witnessArgs.setEmptyLock() const privateKey = findPrivateKey(witnessesArgs[0].lockArgs) + if (!privateKey) { + metadata.locks.skipped.add(lockHash) + witnessSigningEntries.forEach((entry, idx) => { + if (entry.lockHash === lockHash) { + const rawWitness = tx.witnesses[idx] + entry.witness = typeof rawWitness === 'string' ? rawWitness : serializeWitnessArgs(rawWitness) + } + }) + continue + } + + // A 65-byte empty signature used as placeholder + witnessesArgs[0].witnessArgs.setEmptyLock() + const serializedWitnesses: (WitnessArgs | string)[] = witnessesArgs.map((value: SignInfo, index: number) => { const args = value.witnessArgs if (index === 0) { @@ -238,7 +263,7 @@ export default class TransactionSender { tx.witnesses = witnessSigningEntries.map(w => w.witness) tx.hash = txHash - return tx + return { tx, metadata } } public async signMultisig( @@ -266,7 +291,7 @@ export default class TransactionSender { } else { pathAndPrivateKeys = this.getPrivateKeys(wallet, paths, password) } - const findPrivateKeyAndBlake160 = (argsList: string[], signedBlake160s?: string[]) => { + const findPrivateKeyAndBlake160 = (argsList: string[], signedBlake160s?: string[]): [string, string] => { let path: string | undefined let matchArgs: string | undefined argsList.some(args => { @@ -284,7 +309,7 @@ export default class TransactionSender { } return !!path }) - if (!path) { + if (!path || !matchArgs) { throw new NoMatchAddressForSign() } if (!pathAndPrivateKeys) { diff --git a/packages/neuron-wallet/tests/services/tx/transaction-sender.test.ts b/packages/neuron-wallet/tests/services/tx/transaction-sender.test.ts index d52372dc3a..62974c8b29 100644 --- a/packages/neuron-wallet/tests/services/tx/transaction-sender.test.ts +++ b/packages/neuron-wallet/tests/services/tx/transaction-sender.test.ts @@ -204,6 +204,7 @@ import MultisigConfigModel from '../../../src/models/multisig-config' import Multisig from '../../../src/models/multisig' import { addressToScript } from '../../../src/utils/scriptAndAddress' import { serializeWitnessArgs } from '../../../src/utils/serialization' +import { signWitnesses } from '../../../src/utils/signWitnesses' const fakeScript = new Script( '0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8', @@ -343,7 +344,7 @@ describe('TransactionSender Test', () => { describe('#sign', () => { describe('single sign', () => { - const tx = Transaction.fromObject({ + const tx = { version: '0x0', cellDeps: [ CellDep.fromObject({ @@ -393,13 +394,49 @@ describe('TransactionSender Test', () => { witnesses: [ '0x55000000100000005500000055000000410000003965f54cc684d35d886358ad57214e5f4a5fd13ecc7aba67950495b9be7740267a1d6bb14f1c215e3bc926f9655648b75e173ce6f5fd1e60218383b45503c30301', ], - hash: '0x230ab250ee0ae681e88e462102e5c01a9994ac82bf0effbfb58d6c11a86579f1', - }) + } it('success', async () => { - const ntx = await transactionSender.sign(fakeWallet.id, tx, '1234', false) + const res = await transactionSender.sign(fakeWallet.id, Transaction.fromObject(tx), '1234', false) + expect(res.tx.witnesses[0]).toEqual(tx.witnesses[0]) + expect(res.metadata.locks.skipped.size).toEqual(0) + expect(res.metadata.skipLastInput).toEqual(false) + }) + + it('sign with unidentified lock scripts skipped', async () => { + const FIXTURE = { + lock: Input.fromObject({ + previousOutput: OutPoint.fromObject({ + txHash: '0x1879851943fa686af29bed5c95acd566d0244e7b3ca89cf7c435622a5a5b4cb3', + index: '0x0', + }), + since: '0x0', + lock: Script.fromObject({ + args: '0x0000000000000000000000000000000000000000', + codeHash: '0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8', + hashType: 'type' as ScriptHashType, + }), + }), + witness: new WitnessArgs('0x01'), + } + + const txObj = Transaction.fromObject({ + ...tx, + inputs: [...tx.inputs, FIXTURE.lock], + witnesses: [new WitnessArgs('0x00'), FIXTURE.witness], + }) + + const signedWitnesses = signWitnesses({ + witnesses: txObj.witnesses.slice(0, -1), + transactionHash: txObj.computeHash(), + privateKey: pathAndPrivateKey.privateKey, + }) - expect(ntx.witnesses[0]).toEqual(tx.witnesses[0]) + const res = await transactionSender.sign(fakeWallet.id, txObj, '1234', false) + expect(res.tx.witnesses.slice(0, -1)).toEqual(signedWitnesses) + expect(res.tx.witnesses.slice(-1)).toEqual([serializeWitnessArgs(FIXTURE.witness)]) + expect([...res.metadata.locks.skipped.values()]).toEqual([FIXTURE.lock.lockHash]) + expect(res.metadata.skipLastInput).toEqual(false) }) }) @@ -453,9 +490,11 @@ describe('TransactionSender Test', () => { it('success', async () => { // @ts-ignore: Private method - const ntx = await transactionSender.sign(fakeWallet.id, tx, '1234', false) + const res = await transactionSender.sign(fakeWallet.id, tx, '1234', false) - expect(ntx.witnesses[0]).toEqual(expectedWitness[0]) + expect(res.tx.witnesses[0]).toEqual(expectedWitness[0]) + expect(res.metadata.locks.skipped.size).toEqual(0) + expect(res.metadata.skipLastInput).toEqual(false) }) }) @@ -491,9 +530,11 @@ describe('TransactionSender Test', () => { }) it('success', async () => { // @ts-ignore: Private method - const ntx = await transactionSender.sign(fakeWallet.id, tx, '1234', false) + const res = await transactionSender.sign(fakeWallet.id, tx, '1234', false) - expect(ntx.witnesses[0]).toEqual(tx.witnesses[0]) + expect(res.tx.witnesses[0]).toEqual(tx.witnesses[0]) + expect(res.metadata.locks.skipped.size).toEqual(0) + expect(res.metadata.skipLastInput).toEqual(false) }) }) describe('when not matched receiver lock hash', () => { @@ -546,9 +587,11 @@ describe('TransactionSender Test', () => { tx.inputs[0].lock = chequeLock }) it('success', async () => { - const ntx = await transactionSender.sign(fakeWallet.id, tx, '1234', false) + const res = await transactionSender.sign(fakeWallet.id, tx, '1234', false) - expect(ntx.witnesses[0]).toEqual(tx.witnesses[0]) + expect(res.tx.witnesses[0]).toEqual(tx.witnesses[0]) + expect(res.metadata.locks.skipped.size).toEqual(0) + expect(res.metadata.skipLastInput).toEqual(false) }) }) describe('when not matched sender lock hash', () => {