[BTC]: Fix change value calculation in plan when extra outputs provid…

…ed (#3337)
trustwallet · Jul 28, 2023 · 2ab331d · 2ab331d
1 parent 019ada6
commit 2ab331d
Show file tree

Hide file tree

Showing 6 changed files with 192 additions and 33 deletions.
diff --git a/samples/kmp/shared/build.gradle.kts b/samples/kmp/shared/build.gradle.kts
@@ -35,7 +35,7 @@ kotlin {
     sourceSets {
         val commonMain by getting {
             dependencies {
-                implementation("com.trustwallet:wallet-core-kotlin:3.2.5")
+                implementation("com.trustwallet:wallet-core-kotlin:3.2.6")
             }
         }
         val commonTest by getting {

diff --git a/src/Bitcoin/SigningInput.cpp b/src/Bitcoin/SigningInput.cpp
@@ -34,9 +34,9 @@ SigningInput::SigningInput(const Proto::SigningInput& input) {
     lockTime = input.lock_time();
     time = input.time();
 
-    totalAmount = amount;
+    extraOutputsAmount = 0;
     for (auto& output: input.extra_outputs()) {
-        totalAmount += output.amount();
+        extraOutputsAmount += output.amount();
         extraOutputs.push_back(std::make_pair(output.to_address(), output.amount()));
     }
 }

diff --git a/src/Bitcoin/SigningInput.h b/src/Bitcoin/SigningInput.h
@@ -72,8 +72,8 @@ class SigningInput {
     // we have other outputs that include address and value
     std::vector<std::pair<std::string, int64_t>> extraOutputs;
 
-    // Total amount to send, including all outputs amount
-    Amount totalAmount = 0;
+    // Total amount of the `extraOutputs`.
+    Amount extraOutputsAmount = 0;
 
 public:
     SigningInput() = default;

diff --git a/src/Bitcoin/TransactionBuilder.cpp b/src/Bitcoin/TransactionBuilder.cpp
@@ -89,7 +89,9 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) {
     }
 
     bool maxAmount = input.useMaxAmount;
-    if (input.totalAmount == 0 && !maxAmount) {
+    Amount totalAmount = input.amount + input.extraOutputsAmount;
+
+    if (totalAmount == 0 && !maxAmount) {
         plan.error = Common::Proto::Error_zero_amount_requested;
     } else if (input.utxos.empty()) {
         plan.error = Common::Proto::Error_missing_input_utxos;
@@ -103,7 +105,7 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) {
 
         // 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)
-        if (!maxAmount && static_cast<uint64_t>(input.totalAmount) >= inputSum) {
+        if (!maxAmount && static_cast<uint64_t>(totalAmount) >= inputSum) {
             maxAmount = true;
         }
 
@@ -116,10 +118,10 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) {
                 selectedInputs = inputSelector.selectMaxAmount(input.byteFee);
             } else if (input.utxos.size() <= SimpleModeLimit &&
                 input.utxos.size() <= MaxUtxosHardLimit) {
-                selectedInputs = inputSelector.select(plan.amount, input.byteFee, output_size);
+                selectedInputs = inputSelector.select(totalAmount, input.byteFee, output_size);
             } else {
                 selectedInputs =
-                    inputSelector.selectSimple(plan.amount, input.byteFee, output_size);
+                    inputSelector.selectSimple(totalAmount, input.byteFee, output_size);
             }
         } else {
             output_size = 1 + extraOutputs; // output, no change
@@ -139,6 +141,7 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) {
             plan.amount = 0;
             plan.error = Common::Proto::Error_not_enough_utxos;
         } else if (maxAmount && !input.extraOutputs.empty()) {
+            // As of now, we don't support `max` amount **and** extra outputs.
             plan.amount = 0;
             plan.error = Common::Proto::Error_invalid_params;
         } else {
@@ -147,10 +150,10 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) {
             // Compute fee.
             // must preliminary set change so that there is a second output
             if (!maxAmount) {
-                assert(input.totalAmount <= plan.availableAmount);
+                assert(totalAmount <= plan.availableAmount);
                 plan.amount = input.amount;
                 plan.fee = 0;
-                plan.change = plan.availableAmount - input.totalAmount;
+                plan.change = plan.availableAmount - totalAmount;
             } else {
                 plan.amount = plan.availableAmount;
                 plan.fee = 0;
@@ -171,14 +174,25 @@ TransactionPlan TransactionBuilder::plan(const SigningInput& input) {
             }
             assert(plan.amount >= 0 && plan.amount <= plan.availableAmount);
 
-            // compute change
-            plan.change = plan.availableAmount - plan.amount - plan.fee;
+            // The total amount that will be spent.
+            Amount totalSpendAmount = plan.amount + input.extraOutputsAmount + plan.fee;
+
+            // Make sure that we have enough available UTXOs to spend `fee`, `amount` and `extraOutputsAmount`.
+            if (plan.availableAmount < totalSpendAmount) {
+                plan.amount = 0;
+                plan.error = Common::Proto::Error_not_enough_utxos;
+            } else {
+                // compute change
+                plan.change = plan.availableAmount - totalSpendAmount;
+            }
         }
     }
     assert(plan.change >= 0 && plan.change <= plan.availableAmount);
     assert(!maxAmount || plan.change == 0); // change is 0 in max amount case
 
-    assert(plan.amount + plan.change + plan.fee == plan.availableAmount);
+    assert(plan.error != Common::Proto::OK
+           // `plan.error` is OK, check if the values are expected.
+           || plan.amount + input.extraOutputsAmount + plan.change + plan.fee == plan.availableAmount);
 
     return plan;
 }