fix: ife stop eating wrong side bond (#585)

* fix: return piggyback bond when piggyback wrong side This commit change the behavior to be returning piggyback bond no matter it is on the right side of canoncity or not. issue: #584 * test: add unit test covering fix of IFE piggyback bond * fix: e2e tests (py + truffle) for IFE bond change * docs: add explanation on why not need to filter challenged in/output * docs: better comment sentence by PR review suggestion Co-Authored-By: Kevin Sullivan <4653170+kevsul@users.noreply.github.com> * feat: add changelog instruction to bumping version Also format the README file a bit. * chore: bumping version to 1.0.3 - run `npm version patch -m` - update changelog * fix: update CHANGELOG.md wording for PR review Co-Authored-By: Kevin Sullivan <4653170+kevsul@users.noreply.github.com> * refactor: for loop with in/outputs length instead of MAX_IN/OUTPUT_SIZE Although the inputs and outputs storage is defined to be exactly the max size of payment tx inputs/outputs. However, using the length itself makes the code more self explaining. Co-authored-by: Kevin Sullivan <4653170+kevsul@users.noreply.github.com>
omgnetwork · Feb 19, 2020 · 7c3f796 · 7c3f796
1 parent bfbf7cc
commit 7c3f796
Show file tree

Hide file tree

Showing 8 changed files with 99 additions and 39 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,7 +5,10 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-## Unreleased - 2020-02-13
+## Unreleased - 2020-02-18
+
+## [1.0.3] - 2020-02-18
+- In-flight exit returns all unchallenged piggyback bonds even if user piggybacks the wrong canonicity. ([#585](https://github.com/omisego/plasma-contracts/pull/585))
 
 ## [1.0.2] - 2020-02-13
 

diff --git a/README.md b/README.md
@@ -120,9 +120,10 @@ npx truffle test --network loadTest
 
 ### How to release a new plasma contracts version
 
-# Bumps the version in package.json (the patch part)
-# Creates a commit with specified message
-# Tags that commit with the new version
+- Update the [CHANGELOG.md](./CHANGELOG.md)
+- Bumps the version in package.json (the patch part)
+- Creates a commit with specified message
+- Tags that commit with the new version
 ```bash
 npm version patch -m "Fixed a bug in X"
 ```
diff --git a/plasma_framework/contracts/src/exits/payment/controllers/PaymentProcessInFlightExit.sol b/plasma_framework/contracts/src/exits/payment/controllers/PaymentProcessInFlightExit.sol
@@ -71,44 +71,32 @@ library PaymentProcessInFlightExit {
         // So an IFE is only canonical if all inputs of the in-flight tx are not double spent by competing tx or exit.
         // see: https://github.com/omisego/plasma-contracts/issues/470
         if (!exit.isCanonical || isAnyInputSpent(self.framework, exit, token)) {
-            for (uint16 i = 0; i < PaymentTransactionModel.MAX_INPUT_NUM(); i++) {
+            for (uint16 i = 0; i < exit.inputs.length; i++) {
                 PaymentExitDataModel.WithdrawData memory withdrawal = exit.inputs[i];
 
                 if (shouldWithdrawInput(self, exit, withdrawal, token, i)) {
                     withdrawFromVault(self, withdrawal);
-                    bool success = SafeEthTransfer.transferReturnResult(
-                        withdrawal.exitTarget, withdrawal.piggybackBondSize, self.safeGasStipend
-                    );
-
-                    // we do not want to block a queue if bond return is unsuccessful
-                    if (!success) {
-                        emit InFlightBondReturnFailed(withdrawal.exitTarget, withdrawal.piggybackBondSize);
-                    }
                     emit InFlightExitInputWithdrawn(exitId, i);
                 }
             }
 
             flagOutputsWhenNonCanonical(self.framework, exit, token);
         } else {
-            for (uint16 i = 0; i < PaymentTransactionModel.MAX_OUTPUT_NUM(); i++) {
+            for (uint16 i = 0; i < exit.outputs.length; i++) {
                 PaymentExitDataModel.WithdrawData memory withdrawal = exit.outputs[i];
 
                 if (shouldWithdrawOutput(self, exit, withdrawal, token, i)) {
                     withdrawFromVault(self, withdrawal);
-                    bool success = SafeEthTransfer.transferReturnResult(
-                        withdrawal.exitTarget, withdrawal.piggybackBondSize, self.safeGasStipend
-                    );
-                    // we do not want to block a queue if bond return is unsuccessful
-                    if (!success) {
-                        emit InFlightBondReturnFailed(withdrawal.exitTarget, withdrawal.piggybackBondSize);
-                    }
                     emit InFlightExitOutputWithdrawn(exitId, i);
                 }
             }
 
             flagOutputsWhenCanonical(self.framework, exit, token);
         }
 
+        returnInputPiggybackBonds(self, exit, token);
+        returnOutputPiggybackBonds(self, exit, token);
+
         clearPiggybackInputFlag(exit, token);
         clearPiggybackOutputFlag(exit, token);
 
@@ -135,14 +123,14 @@ library PaymentProcessInFlightExit {
         returns (bool)
     {
         uint256 inputNumOfTheToken;
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_INPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.inputs.length; i++) {
             if (exit.inputs[i].token == token && !exit.isInputEmpty(i)) {
                 inputNumOfTheToken++;
             }
         }
         bytes32[] memory outputIdsOfInputs = new bytes32[](inputNumOfTheToken);
         uint sameTokenIndex = 0;
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_INPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.inputs.length; i++) {
             if (exit.inputs[i].token == token && !exit.isInputEmpty(i)) {
                 outputIdsOfInputs[sameTokenIndex] = exit.inputs[i].outputId;
                 sameTokenIndex++;
@@ -204,15 +192,15 @@ library PaymentProcessInFlightExit {
         private
     {
         uint256 piggybackedInputNumOfTheToken;
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_INPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.inputs.length; i++) {
             if (exit.inputs[i].token == token && exit.isInputPiggybacked(i)) {
                 piggybackedInputNumOfTheToken++;
             }
         }
 
         bytes32[] memory outputIdsToFlag = new bytes32[](piggybackedInputNumOfTheToken);
         uint indexForOutputIds = 0;
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_INPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.inputs.length; i++) {
             if (exit.inputs[i].token == token && exit.isInputPiggybacked(i)) {
                 outputIdsToFlag[indexForOutputIds] = exit.inputs[i].outputId;
                 indexForOutputIds++;
@@ -229,28 +217,28 @@ library PaymentProcessInFlightExit {
         private
     {
         uint256 inputNumOfTheToken;
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_INPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.inputs.length; i++) {
             if (exit.inputs[i].token == token && !exit.isInputEmpty(i)) {
                 inputNumOfTheToken++;
             }
         }
 
         uint256 piggybackedOutputNumOfTheToken;
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_OUTPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.outputs.length; i++) {
             if (exit.outputs[i].token == token && exit.isOutputPiggybacked(i)) {
                 piggybackedOutputNumOfTheToken++;
             }
         }
 
         bytes32[] memory outputIdsToFlag = new bytes32[](inputNumOfTheToken + piggybackedOutputNumOfTheToken);
         uint indexForOutputIds = 0;
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_INPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.inputs.length; i++) {
             if (exit.inputs[i].token == token && !exit.isInputEmpty(i)) {
                 outputIdsToFlag[indexForOutputIds] = exit.inputs[i].outputId;
                 indexForOutputIds++;
             }
         }
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_OUTPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.outputs.length; i++) {
             if (exit.outputs[i].token == token && exit.isOutputPiggybacked(i)) {
                 outputIdsToFlag[indexForOutputIds] = exit.outputs[i].outputId;
                 indexForOutputIds++;
@@ -259,13 +247,61 @@ library PaymentProcessInFlightExit {
         framework.batchFlagOutputsFinalized(outputIdsToFlag);
     }
 
+    function returnInputPiggybackBonds(
+        Controller memory self,
+        PaymentExitDataModel.InFlightExit storage exit,
+        address token
+    )
+        private
+    {
+        for (uint16 i = 0; i < exit.inputs.length; i++) {
+            PaymentExitDataModel.WithdrawData memory withdrawal = exit.inputs[i];
+
+            // If the input has been challenged, isInputPiggybacked() will return false
+            if (token == withdrawal.token && exit.isInputPiggybacked(i)) {
+                bool success = SafeEthTransfer.transferReturnResult(
+                    withdrawal.exitTarget, withdrawal.piggybackBondSize, self.safeGasStipend
+                );
+
+                // we do not want to block a queue if bond return is unsuccessful
+                if (!success) {
+                    emit InFlightBondReturnFailed(withdrawal.exitTarget, withdrawal.piggybackBondSize);
+                }
+            }
+        }
+    }
+
+    function returnOutputPiggybackBonds(
+        Controller memory self,
+        PaymentExitDataModel.InFlightExit storage exit,
+        address token
+    )
+        private
+    {
+        for (uint16 i = 0; i < exit.outputs.length; i++) {
+            PaymentExitDataModel.WithdrawData memory withdrawal = exit.outputs[i];
+
+            // If the output has been challenged, isOutputPiggybacked() will return false
+            if (token == withdrawal.token && exit.isOutputPiggybacked(i)) {
+                bool success = SafeEthTransfer.transferReturnResult(
+                    withdrawal.exitTarget, withdrawal.piggybackBondSize, self.safeGasStipend
+                );
+
+                // we do not want to block a queue if bond return is unsuccessful
+                if (!success) {
+                    emit InFlightBondReturnFailed(withdrawal.exitTarget, withdrawal.piggybackBondSize);
+                }
+            }
+        }
+    }
+
     function clearPiggybackInputFlag(
         PaymentExitDataModel.InFlightExit storage exit,
         address token
     )
         private
     {
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_INPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.inputs.length; i++) {
             if (token == exit.inputs[i].token) {
                 exit.clearInputPiggybacked(i);
             }
@@ -278,20 +314,20 @@ library PaymentProcessInFlightExit {
     )
         private
     {
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_OUTPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.outputs.length; i++) {
             if (token == exit.outputs[i].token) {
                 exit.clearOutputPiggybacked(i);
             }
         }
     }
 
     function allPiggybacksCleared(PaymentExitDataModel.InFlightExit memory exit) private pure returns (bool) {
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_INPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.inputs.length; i++) {
             if (exit.isInputPiggybacked(i))
                 return false;
         }
 
-        for (uint16 i = 0; i < PaymentTransactionModel.MAX_OUTPUT_NUM(); i++) {
+        for (uint16 i = 0; i < exit.outputs.length; i++) {
             if (exit.isOutputPiggybacked(i))
                 return false;
         }

diff --git a/plasma_framework/package-lock.json b/plasma_framework/package-lock.json
diff --git a/plasma_framework/package.json b/plasma_framework/package.json
@@ -1,6 +1,6 @@
 {
   "name": "plasma_framework",
-  "version": "1.0.2",
+  "version": "1.0.3",
   "description": "Plasma Framework",
   "directories": {
     "test": "test"

diff --git a/plasma_framework/python_tests/tests/contracts/root_chain/test_process_exits.py b/plasma_framework/python_tests/tests/contracts/root_chain/test_process_exits.py
@@ -686,7 +686,7 @@ def test_in_flight_exit_is_cleaned_up_even_though_none_of_outputs_exited(testlan
     assert in_flight_exit.exit_map == 0
 
     # assert IFE and piggyback bonds were sent to the owners
-    assert testlang.get_balance(owner) == pre_balance + testlang.root_chain.inFlightExitBond()
+    assert testlang.get_balance(owner) == pre_balance + testlang.root_chain.inFlightExitBond() + testlang.root_chain.piggybackBond()
 
 
 def test_processing_ife_and_se_exit_from_same_output_does_not_fail(testlang):

diff --git a/plasma_framework/test/endToEndTests/PaymentInFlightExit.e2e.test.js b/plasma_framework/test/endToEndTests/PaymentInFlightExit.e2e.test.js
@@ -352,7 +352,7 @@ contract('PaymentExitGame - In-flight Exit - End to End Tests', ([_deployer, _ma
                                                     );
                                                 });
 
-                                                it('should return only funds of output B to Alice (input exited)', async () => {
+                                                it('should return funds of output B with piggyback bond to Alice (input exited)', async () => {
                                                     const postBalanceAlice = new BN(await web3.eth.getBalance(alice));
                                                     const expectedBalance = preBalanceAlice
                                                         .add(new BN(DEPOSIT_VALUE))
@@ -361,9 +361,11 @@ contract('PaymentExitGame - In-flight Exit - End to End Tests', ([_deployer, _ma
                                                     expect(expectedBalance).to.be.bignumber.equal(postBalanceAlice);
                                                 });
 
-                                                it('should NOT return fund to Bob (output not exited)', async () => {
+                                                it('should NOT return funds of output B to Bob (output not exited). However, piggyback bond is returned', async () => {
                                                     const postBalanceBob = new BN(await web3.eth.getBalance(bob));
-                                                    expect(preBalanceBob).to.be.bignumber.equal(postBalanceBob);
+                                                    const expectedBalance = preBalanceBob
+                                                        .add(new BN(this.piggybackBondSize));
+                                                    expect(expectedBalance).to.be.bignumber.equal(postBalanceBob);
                                                 });
                                             });
                                         });

diff --git a/plasma_framework/test/src/exits/payment/controllers/PaymentProcessInFlightExit.test.js b/plasma_framework/test/src/exits/payment/controllers/PaymentProcessInFlightExit.test.js
@@ -386,6 +386,7 @@ contract('PaymentProcessInFlightExit', ([_, ifeBondOwner, inputOwner1, inputOwne
                 await this.exitGame.setInFlightExitOutputPiggybacked(DUMMY_EXIT_ID, 2);
 
                 this.inputOwner3PreBalance = new BN(await web3.eth.getBalance(inputOwner3));
+                this.outputOwner3PreBalance = new BN(await web3.eth.getBalance(outputOwner3));
             });
 
             it('should withdraw ETH from vault for the piggybacked input', async () => {
@@ -464,6 +465,14 @@ contract('PaymentProcessInFlightExit', ([_, ifeBondOwner, inputOwner1, inputOwne
                 expect(postBalance).to.be.bignumber.equal(expectedBalance);
             });
 
+            it('should return piggyback bond to the output owner', async () => {
+                await this.exitGame.processExit(DUMMY_EXIT_ID, VAULT_ID.ERC20, erc20);
+                const postBalance = new BN(await web3.eth.getBalance(outputOwner3));
+                const expectedBalance = this.outputOwner3PreBalance.add(this.piggybackBondSize);
+
+                expect(postBalance).to.be.bignumber.equal(expectedBalance);
+            });
+
             it('should only flag piggybacked inputs with the same token as spent', async () => {
                 await this.exitGame.processExit(DUMMY_EXIT_ID, VAULT_ID.ETH, ETH);
                 // piggybacked input
@@ -508,6 +517,7 @@ contract('PaymentProcessInFlightExit', ([_, ifeBondOwner, inputOwner1, inputOwne
                 await this.exitGame.setInFlightExitOutputPiggybacked(DUMMY_EXIT_ID, 0);
                 await this.exitGame.setInFlightExitOutputPiggybacked(DUMMY_EXIT_ID, 2);
 
+                this.inputOwner3PreBalance = new BN(await web3.eth.getBalance(inputOwner3));
                 this.outputOwner3PreBalance = new BN(await web3.eth.getBalance(outputOwner3));
             });
 
@@ -587,6 +597,14 @@ contract('PaymentProcessInFlightExit', ([_, ifeBondOwner, inputOwner1, inputOwne
                 expect(postBalance).to.be.bignumber.equal(expectedBalance);
             });
 
+            it('should return piggyback bond to the input owner', async () => {
+                await this.exitGame.processExit(DUMMY_EXIT_ID, VAULT_ID.ERC20, erc20);
+                const postBalance = new BN(await web3.eth.getBalance(inputOwner3));
+                const expectedBalance = this.inputOwner3PreBalance.add(this.piggybackBondSize);
+
+                expect(postBalance).to.be.bignumber.equal(expectedBalance);
+            });
+
             it('should flag ALL inputs with the same token as spent', async () => {
                 await this.exitGame.processExit(DUMMY_EXIT_ID, VAULT_ID.ETH, ETH);
                 // same token, both piggybacked and non-piggybacked cases