Merge pull request #5109 from BitGo/BTC-1582-script-in-address

Handle a transaction with an OP_RETURN

Author: davidkaplanbitgo

modules/abstract-utxo/src/abstractUtxoCoin.ts

@@ -76,7 +76,15 @@ import ScriptType2Of3 = utxolib.bitgo.outputScripts.ScriptType2Of3;
 import { isReplayProtectionUnspent } from './replayProtection';
 import { signAndVerifyPsbt, signAndVerifyWalletTransaction } from './sign';
 import { supportedCrossChainRecoveries } from './config';
-import { explainPsbt, explainTx, getPsbtTxInputs, getTxInputs } from './transaction';
+import {
+  assertValidTransactionRecipient,
+  explainPsbt,
+  explainTx,
+  fromExtendedAddressFormat,
+  getPsbtTxInputs,
+  getTxInputs,
+  isExtendedAddressFormat,
+} from './transaction';
 import { assertDescriptorWalletAddress } from './descriptor/assertDescriptorWalletAddress';
 
 type UtxoCustomSigningFunction<TNumber extends number | bigint> = {
@@ -447,6 +455,20 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
     }
   }
 
+  preprocessBuildParams(params: Record<string, any>): Record<string, any> {
+    if (params.recipients !== undefined) {
+      params.recipients =
+        params.recipients instanceof Array
+          ? params?.recipients?.map((recipient) => {
+              const { address, ...rest } = recipient;
+              return { ...rest, ...fromExtendedAddressFormat(address) };
+            })
+          : params.recipients;
+    }
+
+    return params;
+  }
+
   /**
    * Get the latest block height
    * @param reqId
@@ -459,6 +481,13 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
     return (chainhead as any).height;
   }
 
+  checkRecipient(recipient: { address: string; amount: number | string }): void {
+    assertValidTransactionRecipient(recipient);
+    if (!isExtendedAddressFormat(recipient.address)) {
+      super.checkRecipient(recipient);
+    }
+  }
+
   /**
    * Run custom coin logic after a transaction prebuild has been received from BitGo
    * @param prebuild
@@ -511,6 +540,18 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
     return utxolib.bitgo.createTransactionFromHex<TNumber>(hex, this.network, this.amountType);
   }
 
+  toCanonicalTransactionRecipient(output: { valueString: string; address?: string }): {
+    amount: bigint;
+    address?: string;
+  } {
+    const amount = BigInt(output.valueString);
+    assertValidTransactionRecipient({ amount, address: output.address });
+    if (!output.address) {
+      return { amount };
+    }
+    return { amount, address: this.canonicalAddress(output.address) };
+  }
+
   /**
    * Extract and fill transaction details such as internal/change spend, external spend (explicit vs. implicit), etc.
    * @param params
@@ -567,25 +608,39 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
       assert(txParams.rbfTxIds.length === 1);
 
       const txToBeReplaced = await wallet.getTransaction({ txHash: txParams.rbfTxIds[0], includeRbf: true });
-      expectedOutputs = txToBeReplaced.outputs
-        .filter((output) => output.wallet !== wallet.id()) // For self-sends, the walletId will be the same as the wallet's id
-        .map((output) => {
-          return { amount: BigInt(output.valueString), address: this.canonicalAddress(output.address) };
-        });
+      expectedOutputs = txToBeReplaced.outputs.flatMap(
+        (output: { valueString: string; address?: string; wallet?: string }) => {
+          // For self-sends, the walletId will be the same as the wallet's id
+          if (output.wallet === wallet.id()) {
+            return [];
+          }
+          return [this.toCanonicalTransactionRecipient(output)];
+        }
+      );
     } else {
       // verify that each recipient from txParams has their own output
-      expectedOutputs = _.get(txParams, 'recipients', [] as TransactionRecipient[]).map((output) => {
-        return { ...output, address: this.canonicalAddress(output.address) };
+      expectedOutputs = _.get(txParams, 'recipients', [] as TransactionRecipient[]).flatMap((output) => {
+        if (output.address === undefined) {
+          if (output.amount.toString() !== '0') {
+            throw new Error(`Only zero amounts allowed for non-encodeable scriptPubkeys: ${output}`);
+          }
+          return [output];
+        }
+        return [{ ...output, address: this.canonicalAddress(output.address) }];
       });
       if (params.txParams.allowExternalChangeAddress && params.txParams.changeAddress) {
         // when an external change address is explicitly specified, count all outputs going towards that
         // address in the expected outputs (regardless of the output amount)
         expectedOutputs.push(
-          ...allOutputs
-            .map((output) => {
-              return { ...output, address: this.canonicalAddress(output.address) };
-            })
-            .filter((output) => output.address === this.canonicalAddress(params.txParams.changeAddress as string))
+          ...allOutputs.flatMap((output) => {
+            if (
+              output.address === undefined ||
+              output.address !== this.canonicalAddress(params.txParams.changeAddress as string)
+            ) {
+              return [];
+            }
+            return [{ ...output, address: this.canonicalAddress(output.address) }];
+          })
         );
       }
     }

modules/abstract-utxo/src/parseOutput.ts

@@ -213,6 +213,15 @@ export async function parseOutput({
   const disableNetworking = !!verification.disableNetworking;
   const currentAddress = currentOutput.address;
 
+  if (currentAddress === undefined) {
+    // In the case that the address is undefined, it means that the output has a non-encodeable scriptPubkey
+    // If this is the case, then we need to check that the amount is 0 and we can skip the rest.
+    if (currentOutput.amount.toString() !== '0') {
+      throw new Error('output with undefined address must have amount of 0');
+    }
+    return currentOutput;
+  }
+
   // attempt to grab the address details from either the prebuilt tx, or the verification params.
   // If both of these are empty, then we will try to get the address details from bitgo instead
   const addressDetailsPrebuild = _.get(txPrebuild, `txInfo.walletAddressDetails.${currentAddress}`, {});

modules/abstract-utxo/src/transaction.ts

@@ -11,6 +11,40 @@ import {
 } from './abstractUtxoCoin';
 import { bip32, BIP32Interface, bitgo } from '@bitgo/utxo-lib';
 
+const ScriptRecipientPrefix = 'scriptPubkey:';
+
+/**
+ * An extended address is one that encodes either a regular address or a hex encoded script with the prefix `scriptPubkey:`.
+ * This function converts the extended address format to either a script or an address.
+ * @param extendedAddress
+ */
+export function fromExtendedAddressFormat(extendedAddress: string): { address: string } | { script: string } {
+  if (extendedAddress.startsWith(ScriptRecipientPrefix)) {
+    return { script: extendedAddress.replace(ScriptRecipientPrefix, '') };
+  }
+  return { address: extendedAddress };
+}
+
+export function toExtendedAddressFormat(script: Buffer, network: utxolib.Network): string {
+  return script[0] === utxolib.opcodes.OP_RETURN
+    ? `${ScriptRecipientPrefix}${script.toString('hex')}`
+    : utxolib.address.fromOutputScript(script, network);
+}
+
+export function isExtendedAddressFormat(address: string): boolean {
+  return address.startsWith(ScriptRecipientPrefix);
+}
+
+export function assertValidTransactionRecipient(output: { amount: bigint | number | string; address?: string }): void {
+  // In the case that this is an OP_RETURN output or another non-encodable scriptPubkey, we dont have an address.
+  // We will verify that the amount is zero, and if it isnt then we will throw an error.
+  if (!output.address || output.address.startsWith(ScriptRecipientPrefix)) {
+    if (output.amount.toString() !== '0') {
+      throw new Error(`Only zero amounts allowed for non-encodeable scriptPubkeys: ${JSON.stringify(output)}`);
+    }
+  }
+}
+
 /**
  * Get the inputs for a psbt from a prebuild.
  */
@@ -106,7 +140,9 @@ function explainCommon<TNumber extends number | bigint>(
   const changeAddresses = changeInfo?.map((info) => info.address) ?? [];
 
   tx.outs.forEach((currentOutput) => {
-    const currentAddress = utxolib.address.fromOutputScript(currentOutput.script, network);
+    // Try to encode the script pubkey with an address. If it fails, try to parse it as an OP_RETURN output with the prefix.
+    // If that fails, then it is an unrecognized scriptPubkey and should fail
+    const currentAddress = toExtendedAddressFormat(currentOutput.script, network);
     const currentAmount = BigInt(currentOutput.value);
 
     if (changeAddresses.includes(currentAddress)) {

modules/bitgo/test/v2/unit/wallet.ts

@@ -1859,6 +1859,30 @@ describe('V2 Wallet:', function () {
       postProcessStub.restore();
     });
 
+    it('should pass script outputs with the proper structure to wallet platform', async function () {
+      const script = '6a11223344556677889900';
+      nock(bgUrl)
+        .post(`/api/v2/${wallet.coin()}/wallet/${wallet.id()}/tx/build`, {
+          ...tbtcHotWalletDefaultParams,
+          recipients: [{ script, amount: 1e6 }],
+        })
+        .query({})
+        .reply(200, {});
+
+      const blockHeight = 100;
+      const blockHeightStub = sinon.stub(basecoin, 'getLatestBlockHeight').resolves(blockHeight);
+      const postProcessStub = sinon.stub(basecoin, 'postProcessPrebuild').resolves({});
+      await wallet.prebuildTransaction({ recipients: [{ address: `scriptPubkey:${script}`, amount: 1e6 }] });
+      blockHeightStub.should.have.been.calledOnce();
+      postProcessStub.should.have.been.calledOnceWith({
+        blockHeight: 100,
+        wallet: wallet,
+        buildParams: { ...tbtcHotWalletDefaultParams, recipients: [{ script, amount: 1e6 }] },
+      });
+      blockHeightStub.restore();
+      postProcessStub.restore();
+    });
+
     it('prebuild should call build and getLatestBlockHeight for utxo coins', async function () {
       const params = {};
       nock(bgUrl)

modules/sdk-core/src/bitgo/baseCoin/baseCoin.ts

@@ -225,6 +225,16 @@ export abstract class BaseCoin implements IBaseCoin {
     return bigNumber.toFormat(null as any, null as any, { groupSeparator: '', decimalSeparator: '.' });
   }
 
+  checkRecipient(recipient: { address: string; amount: string | number }): void {
+    const amount = new BigNumber(recipient.amount);
+    if (amount.isNegative()) {
+      throw new Error('invalid argument for amount - positive number greater than zero or numeric string expected');
+    }
+    if (!this.valuelessTransferAllowed() && amount.isZero()) {
+      throw new Error('invalid argument for amount - positive number greater than zero or numeric string expected');
+    }
+  }
+
   /**
    * Convert a currency amount represented in big units (btc, eth, xrp, xlm)
    * to base units (satoshi, wei, atoms, drops, stroops)
@@ -239,6 +249,14 @@ export abstract class BaseCoin implements IBaseCoin {
     return bigNumber.toFixed(0);
   }
 
+  /**
+   * Preprocess the build parameters before sending to the API
+   * @param buildParams
+   */
+  preprocessBuildParams(buildParams: Record<string, any>): Record<string, any> {
+    return buildParams;
+  }
+
   /**
    * Sign message with private key
    *

modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts

@@ -473,6 +473,7 @@ export interface IBaseCoin {
   getFamily(): string;
   getFullName(): string;
   valuelessTransferAllowed(): boolean;
+  checkRecipient(recipient: { address: string; amount: string | number }): void;
   sweepWithSendMany(): boolean;
   transactionDataAllowed(): boolean;
   allowsAccountConsolidations(): boolean;
@@ -484,6 +485,7 @@ export interface IBaseCoin {
   getBaseFactor(): number | string;
   baseUnitsToBigUnits(baseUnits: string | number): string;
   bigUnitsToBaseUnits(bigUnits: string | number): string;
+  preprocessBuildParams(params: Record<string, any>): Record<string, any>;
   signMessage(key: { prv: string }, message: string): Promise<Buffer>;
   createKeySignatures(
     prv: string,

modules/sdk-core/src/bitgo/wallet/wallet.ts

@@ -1747,7 +1747,7 @@ export class Wallet implements IWallet {
     }
 
     // Whitelist params to build tx
-    const whitelistedParams = _.pick(params, this.prebuildWhitelistedParams());
+    const whitelistedParams = this.baseCoin.preprocessBuildParams(_.pick(params, this.prebuildWhitelistedParams()));
     debug('prebuilding transaction: %O', whitelistedParams);
 
     if (params.reqId) {
@@ -2484,13 +2484,7 @@ export class Wallet implements IWallet {
     const coin = this.baseCoin;
     if (_.isObject(params.recipients)) {
       params.recipients.map(function (recipient) {
-        const amount = new BigNumber(recipient.amount);
-        if (amount.isNegative()) {
-          throw new Error('invalid argument for amount - positive number greater than zero or numeric string expected');
-        }
-        if (!coin.valuelessTransferAllowed() && amount.isZero()) {
-          throw new Error('invalid argument for amount - positive number greater than zero or numeric string expected');
-        }
+        coin.checkRecipient(recipient);
       });
     }
 
@@ -3661,7 +3655,7 @@ export class Wallet implements IWallet {
     // extract the whitelisted params from the top level, in case
     // other invalid params are present that would fail encoding
     // and fall back to the body params
-    const whitelistedParams = _.pick(params, whitelistedSendParams);
+    const whitelistedParams = this.baseCoin.preprocessBuildParams(_.pick(params, whitelistedSendParams));
     const reqTracer = reqId || new RequestTracer();
     this.bitgo.setRequestTracer(reqTracer);
     return postWithCodec(