Address PR feedback

- Align with inbox/outbox naming - Moved returning penalty after progressStakeInternal and collect relevant values before that
OpenST · Jan 28, 2019 · b7dcede · b7dcede
1 parent 3a653ca
commit b7dcede
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 21 deletions.
diff --git a/contracts/gateway/EIP20Gateway.sol b/contracts/gateway/EIP20Gateway.sol
@@ -472,14 +472,8 @@ contract EIP20Gateway is GatewayBase {
 
         // Get the message object
         MessageBus.Message storage message = messages[_messageHash];
-        // Return revert penalty to staker if message is already progressed
-        // and can't be reverted anymore.
-        tryReturnPenaltyToStaker(
-            _messageHash,
-            message.sender,
-            messageBox.outbox[_messageHash], // old message status
-            MessageBus.MessageStatus(_messageStatus) // new message status
-        );
+        MessageBus.MessageStatus outboxMessageStatus =
+          messageBox.outbox[_messageHash];
 
         MessageBus.progressOutboxWithProof(
             messageBox,
@@ -490,12 +484,22 @@ contract EIP20Gateway is GatewayBase {
             MessageBus.MessageStatus(_messageStatus)
         );
 
+        uint256 bountyAmount = stakes[_messageHash].bounty;
         (staker_, stakeAmount_) = progressStakeInternal(
             _messageHash,
             message,
             bytes32(0),
             true
         );
+
+        // Return revert penalty to staker if message is already progressed
+        // and can't be reverted anymore.
+        tryReturnPenaltyToStaker(
+            message.sender,
+            outboxMessageStatus,
+            MessageBus.MessageStatus(_messageStatus), // inbox message status
+            bountyAmount
+        );
     }
 
     /**
@@ -505,28 +509,28 @@ contract EIP20Gateway is GatewayBase {
      * @dev Should only be called from progressStakeWithProof. This function
      *      exists to avoid a stack too deep error.
      *
-     * @param _messageHash Message hash.
      * @param _staker Staker address.
-     * @param _oldMessageStatus Message status before progressing.
-     * @param _newMessageStatus Message status after progressing.
+     * @param _outboxMessageStatus Message status before progressing.
+     * @param _inboxMessageStatus Message status after progressing.
+     * @param _bountyAmount Bounty amount to use for calculating penalty.
      */
     function tryReturnPenaltyToStaker(
-        bytes32 _messageHash,
         address _staker,
-        MessageBus.MessageStatus _oldMessageStatus,
-        MessageBus.MessageStatus _newMessageStatus
+        MessageBus.MessageStatus _outboxMessageStatus,
+        MessageBus.MessageStatus _inboxMessageStatus,
+        uint256 _bountyAmount
     )
       private
     {
-        if (_oldMessageStatus != MessageBus.MessageStatus.DeclaredRevocation) {
+        if (_outboxMessageStatus != MessageBus.MessageStatus.DeclaredRevocation) {
             return;
         }
-        if (_newMessageStatus != MessageBus.MessageStatus.Progressed) {
+        if (_inboxMessageStatus != MessageBus.MessageStatus.Progressed) {
             return;
         }
 
         // Penalty charged to staker for revert stake.
-        uint256 penalty = penaltyFromBounty(stakes[_messageHash].bounty);
+        uint256 penalty = penaltyFromBounty(_bountyAmount);
         // transfer the penalty amount
         require(
             baseToken.transfer(_staker, penalty),
@@ -1124,7 +1128,7 @@ contract EIP20Gateway is GatewayBase {
         // Get the staker address
         staker_ = _message.sender;
 
-        //Get the stake amount.
+        // Get the stake amount.
         stakeAmount_ = stakes[_messageHash].amount;
 
         // Transfer the staked amount to stakeVault.

diff --git a/test/gateway/eip20_gateway/progress_stake_with_proof.js b/test/gateway/eip20_gateway/progress_stake_with_proof.js
@@ -38,6 +38,7 @@ const MessageStatusEnum = messageBus.MessageStatusEnum;
 contract('EIP20Gateway.progressStakeWithProof()', function (accounts) {
 
   let gateway, mockToken, baseToken, stakeData, progressStakeParams, bountyAmount, penaltyAmount;
+  const PENALTY_MULTIPLIER = 1.5;
 
   let setStorageRoot = async function() {
 
@@ -123,7 +124,7 @@ contract('EIP20Gateway.progressStakeWithProof()', function (accounts) {
     let burner = NullAddress;
 
     bountyAmount = new BN(proofData.gateway.constructor.bounty);
-    penaltyAmount = bountyAmount.muln(1.5);
+    penaltyAmount = bountyAmount.muln(PENALTY_MULTIPLIER);
 
     gateway = await Gateway.new(
       mockToken.address,
@@ -591,7 +592,7 @@ contract('EIP20Gateway.progressStakeWithProof()', function (accounts) {
     assert.strictEqual(
       stakerFinalBaseTokenBalance.eq(stakerInitialBaseTokenBalance.add(penaltyAmount)),
       true,
-      `Staker's base token balance ${stakerFinalBaseTokenBalance} must be equal to ${stakerInitialBaseTokenBalance.add(penaltyAmount)}`,
+      `Staker's base token balance ${stakerFinalBaseTokenBalance.toString(10)} must be equal to ${stakerInitialBaseTokenBalance.add(penaltyAmount).toString(10)}`,
     );
 
     assert.strictEqual(
@@ -606,7 +607,7 @@ contract('EIP20Gateway.progressStakeWithProof()', function (accounts) {
         gatewayInitialBaseTokenBalance.sub(bountyAmount).sub(penaltyAmount)
       ),
       true,
-      `Gateway's base token balance ${gatewayFinalBaseTokenBalance} must be equal to ${gatewayInitialBaseTokenBalance.sub(bountyAmount).sub(penaltyAmount)}.`,
+      `Gateway's base token balance ${gatewayFinalBaseTokenBalance.toString(10)} must be equal to ${gatewayInitialBaseTokenBalance.sub(bountyAmount).sub(penaltyAmount).toString(10)}.`,
     );
 
     assert.strictEqual(