fix(bitcoin): Fix TWAnySignerPlan (#3278)

src/Bitcoin/TransactionBuilder.cpp

@@ -99,7 +99,7 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) {
         auto inputSum = InputSelector<UTXO>::sum(input.utxos);
 
         // select UTXOs
-        plan.amount = input.totalAmount;
+        plan.amount = input.amount;
 
         // if amount requested is the same or more than available amount, it cannot be satisfied, but
         // treat this case as MaxAmount, and send maximum available (which will be less)
@@ -114,14 +114,12 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) {
             output_size = 2 + extraOutputs; // output + change
             if (input.useMaxUtxo) {
                 selectedInputs = inputSelector.selectMaxAmount(input.byteFee);
+            } else if (input.utxos.size() <= SimpleModeLimit &&
+                input.utxos.size() <= MaxUtxosHardLimit) {
+                selectedInputs = inputSelector.select(plan.amount, input.byteFee, output_size);
             } else {
-                if (input.utxos.size() <= SimpleModeLimit &&
-                    input.utxos.size() <= MaxUtxosHardLimit) {
-                    selectedInputs = inputSelector.select(plan.amount, input.byteFee, output_size);
-                } else {
-                    selectedInputs =
-                        inputSelector.selectSimple(plan.amount, input.byteFee, output_size);
-                }
+                selectedInputs =
+                    inputSelector.selectSimple(plan.amount, input.byteFee, output_size);
             }
         } else {
             output_size = 1 + extraOutputs; // output, no change
@@ -137,19 +135,22 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) {
             }
         }
 
-        if (plan.utxos.size() == 0) {
+        if (plan.utxos.empty()) {
             plan.amount = 0;
             plan.error = Common::Proto::Error_not_enough_utxos;
+        } else if (maxAmount && !input.extraOutputs.empty()) {
+            plan.amount = 0;
+            plan.error = Common::Proto::Error_invalid_params;
         } else {
             plan.availableAmount = InputSelector<UTXO>::sum(plan.utxos);
 
             // Compute fee.
             // must preliminary set change so that there is a second output
             if (!maxAmount) {
                 assert(input.totalAmount <= plan.availableAmount);
-                plan.amount = input.totalAmount;
+                plan.amount = input.amount;
                 plan.fee = 0;
-                plan.change = plan.availableAmount - plan.amount;
+                plan.change = plan.availableAmount - input.totalAmount;
             } else {
                 plan.amount = plan.availableAmount;
                 plan.fee = 0;
@@ -167,7 +168,6 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) {
             } else {
                 // max available amount
                 plan.amount = std::max(Amount(0), plan.availableAmount - plan.fee);
-                plan.useMaxAmount = true;
             }
             assert(plan.amount >= 0 && plan.amount <= plan.availableAmount);
 

src/Bitcoin/TransactionBuilder.h

@@ -30,11 +30,7 @@ class TransactionBuilder {
         Transaction tx;
         tx.lockTime = input.lockTime;
 
-        auto outputToAmount = input.amount;
-        if (plan.useMaxAmount) {
-            outputToAmount = plan.amount;
-        }
-        auto outputTo = prepareOutputWithScript(input.toAddress, outputToAmount, input.coinType);
+        auto outputTo = prepareOutputWithScript(input.toAddress, plan.amount, input.coinType);
         if (!outputTo.has_value()) { 
             return Result<Transaction, Common::Proto::SigningError>::failure(Common::Proto::Error_invalid_address);
         }

src/Bitcoin/TransactionPlan.h

@@ -41,9 +41,6 @@ struct TransactionPlan {
 
     Data outputOpReturn;
 
-    /// Check if we use max amount for output address
-    bool useMaxAmount = false;
-
     Common::Proto::SigningError error = Common::Proto::SigningError::OK;
 
     TransactionPlan() = default;

src/Decred/TransactionBuilder.h

@@ -29,18 +29,13 @@ struct TransactionBuilder {
     static Transaction build(const Bitcoin::TransactionPlan& plan, const Bitcoin::SigningInput& input) {
         auto coin = TWCoinTypeDecred;
 
-        auto outputToAmount = input.amount;
-        if (plan.useMaxAmount) {
-            outputToAmount = plan.amount;
-        }
-
         auto lockingScriptTo = Bitcoin::Script::lockScriptForAddress(input.toAddress, coin);
         if (lockingScriptTo.empty()) {
             return {};
         }
 
         Transaction tx;
-        tx.outputs.emplace_back(TransactionOutput(outputToAmount, /* version: */ 0, lockingScriptTo));
+        tx.outputs.emplace_back(TransactionOutput(plan.amount, /* version: */ 0, lockingScriptTo));
 
         if (plan.change > 0) {
             auto lockingScriptChange = Bitcoin::Script::lockScriptForAddress(input.changeAddress, coin);

src/Zen/TransactionBuilder.h

@@ -37,11 +37,7 @@ struct TransactionBuilder {
         auto blockHash = plan.preBlockHash;
         auto blockHeight = plan.preBlockHeight;
 
-        auto outputToAmount = input.amount;
-        if (plan.useMaxAmount) {
-            outputToAmount = plan.amount;
-        }
-        auto outputTo = prepareOutputWithScript(input.toAddress, outputToAmount, input.coinType, blockHash, blockHeight);
+        auto outputTo = prepareOutputWithScript(input.toAddress, plan.amount, input.coinType, blockHash, blockHeight);
         if (!outputTo.has_value()) { 
             return Result<Transaction, Common::Proto::SigningError>::failure(Common::Proto::Error_invalid_address);
         }

tests/chains/Bitcoin/TWBitcoinSigningTests.cpp

@@ -82,6 +82,65 @@ SigningInput buildInputP2PKH(bool omitKey = false) {
     return input;
 }
 
+/// This test only checks if the transaction output will have an expected value.
+/// It doesn't check correctness of the encoded representation.
+/// Issue: https://github.com/trustwallet/wallet-core/issues/3273
+TEST(BitcoinSigning, SignMaxAmount) {
+    const auto privateKey = parse_hex("4646464646464646464646464646464646464646464646464646464646464646");
+    const auto ownAddress = "bc1qhkfq3zahaqkkzx5mjnamwjsfpq2jk7z00ppggv";
+
+    const auto revUtxoHash0 =
+        parse_hex("07c42b969286be06fae38528c85f0a1ce508d4df837eb5ac4cf5f2a7a9d65fa8");
+    const auto inPubKey0 =
+        parse_hex("024bc2a31265153f07e70e0bab08724e6b85e217f8cd628ceb62974247bb493382");
+    const auto inPubKeyHash0 = parse_hex("bd92088bb7e82d611a9b94fbb74a0908152b784f");
+    const auto utxoScript0 = parse_hex("0014bd92088bb7e82d611a9b94fbb74a0908152b784f");
+
+    const auto initialAmount = 10'189'533;
+    const auto availableAmount = 10'189'534;
+    const auto fee = 110;
+    const auto amountWithoutFee = availableAmount - fee;
+    // There shouldn't be any change
+    const auto change = 0;
+
+    Proto::SigningInput signingInput;
+    signingInput.set_coin_type(TWCoinTypeBitcoin);
+    signingInput.set_hash_type(TWBitcoinSigHashTypeAll);
+    signingInput.set_amount(initialAmount);
+    signingInput.set_byte_fee(1);
+    signingInput.set_to_address("bc1q2dsdlq3343vk29runkgv4yc292hmq53jedfjmp");
+    signingInput.set_change_address(ownAddress);
+    signingInput.set_use_max_amount(true);
+
+    *signingInput.add_private_key() = std::string(privateKey.begin(), privateKey.end());
+
+    // Add UTXO
+    auto utxo = signingInput.add_utxo();
+    utxo->set_script(utxoScript0.data(), utxoScript0.size());
+    utxo->set_amount(availableAmount);
+    utxo->mutable_out_point()->set_hash(
+        std::string(revUtxoHash0.begin(), revUtxoHash0.end()));
+    utxo->mutable_out_point()->set_index(0);
+    utxo->mutable_out_point()->set_sequence(UINT32_MAX);
+
+    // Plan
+    Proto::TransactionPlan plan;
+    ANY_PLAN(signingInput, plan, TWCoinTypeBitcoin);
+    // Plan is checked, assume it is accepted
+    EXPECT_EQ(plan.amount(), amountWithoutFee);
+    EXPECT_EQ(plan.available_amount(), availableAmount);
+    EXPECT_EQ(plan.fee(), fee);
+    EXPECT_EQ(plan.change(), change);
+
+    *signingInput.mutable_plan() = plan;
+
+    Proto::SigningOutput output;
+    ANY_SIGN(signingInput, TWCoinTypeBitcoin);
+
+    const auto& output0 = output.transaction().outputs().at(0);
+    EXPECT_EQ(output0.value(), amountWithoutFee);
+}
+
 TEST(BitcoinSigning, SignBRC20TransferCommit) {
     auto privateKey = parse_hex("e253373989199da27c48680e3a3fc0f648d50f9a727ef17a7fe6a4dc3b159129");
     auto fullAmount = 26400;
@@ -589,11 +648,11 @@ TEST(BitcoinSigning, SignP2WPKH_HashAnyoneCanPay_TwoInput) {
 
 TEST(BitcoinSigning, SignP2WPKH_MaxAmount) {
     auto input = buildInputP2WPKH(1'000, TWBitcoinSigHashTypeAll, 625'000'000, 600'000'000, true);
-    input.totalAmount = 1224999773;
+    input.totalAmount = 1'224'999'773;
     {
         // test plan (but do not reuse plan result)
         auto plan = TransactionBuilder::plan(input);
-        EXPECT_TRUE(verifyPlan(plan, {625'000'000, 600'000'000}, 1224999773, 227));
+        EXPECT_TRUE(verifyPlan(plan, {625'000'000, 600'000'000}, 1'224'999'773, 227));
     }
 
     // Sign

tests/chains/Bitcoin/TxComparisonHelper.cpp

@@ -43,6 +43,7 @@ UTXOs buildTestUTXOs(const std::vector<int64_t>& amounts) {
 
 SigningInput buildSigningInput(Amount amount, int byteFee, const UTXOs& utxos, bool useMaxAmount, enum TWCoinType coin, bool omitPrivateKey) {
     SigningInput input;
+    input.amount = amount;
     input.totalAmount = amount;
     input.byteFee = byteFee;
     input.useMaxAmount = useMaxAmount;

tests/chains/Zen/TransactionBuilderTests.cpp

@@ -66,7 +66,6 @@ TEST(ZenTransactionBuilder, Build) {
     ASSERT_EQ(plan.fee, 294);
     plan.preBlockHash = blockHash;
     plan.preBlockHeight = blockHeight;
-    plan.useMaxAmount = true;
 
     // plan1
     auto result = Zen::TransactionBuilder::build<Bitcoin::Transaction>(plan, input).payload();
@@ -75,7 +74,6 @@ TEST(ZenTransactionBuilder, Build) {
     ASSERT_EQ(result.outputs[0].value, plan.amount);
 
     // plan2
-    plan.useMaxAmount = false;
     result = Zen::TransactionBuilder::build<Bitcoin::Transaction>(plan, input).payload();
 
     ASSERT_EQ(result.outputs.size(), 4ul);