From d55617bf52a96ef327d5be1781284ea4805905e0 Mon Sep 17 00:00:00 2001
From: julia-zack <julia.zack@iovlabs.org>
Date: Thu, 21 Nov 2024 11:57:43 -0300
Subject: [PATCH] Refactor tests and improve comments

---
 .../peg/federation/FederationSupportImpl.java |  16 ++-
 .../java/co/rsk/peg/BridgeSupportSvpTest.java | 123 +++++++++++++-----
 .../federation/VoteFederationChangeTest.java  |  60 +++++----
 3 files changed, 132 insertions(+), 67 deletions(-)

diff --git a/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java b/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
index b07d83186f..6c6e2ae82e 100644
--- a/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
+++ b/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
@@ -693,7 +693,7 @@ private FederationChangeResponseCode commitPendingFederationAccordingToActivatio
      */
     private FederationChangeResponseCode legacyCommitPendingFederation(PendingFederation currentPendingFederation, BridgeEventLogger eventLogger) {
         Federation newFederation = buildFederationFromPendingFederation(currentPendingFederation);
-        handoverFromOldToNewFederation(getActiveFederation(), newFederation);
+        handoverToNewFederation(newFederation);
 
         clearPendingFederationVoting();
 
@@ -708,14 +708,14 @@ public void commitProposedFederation() {
         Federation proposedFederation = provider.getProposedFederation(constants, activations)
                 .orElseThrow(IllegalStateException::new);
 
-        handoverFromOldToNewFederation(getActiveFederation(), proposedFederation);
+        handoverToNewFederation(proposedFederation);
         clearProposedFederation();
     }
 
-    private void handoverFromOldToNewFederation(Federation oldFederation, Federation newFederation) {
+    private void handoverToNewFederation(Federation newFederation) {
         moveUTXOsFromNewToOldFederation();
 
-        setOldAndNewFederations(oldFederation, newFederation);
+        setOldAndNewFederations(getActiveFederation(), newFederation);
 
         if (activations.isActive(RSKIP186)) {
             saveLastRetiredFederationScript();
@@ -729,15 +729,17 @@ private void setOldAndNewFederations(Federation oldFederation, Federation newFed
     }
 
     private void moveUTXOsFromNewToOldFederation() {
-        List<UTXO> utxosToMove = new ArrayList<>(provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations));
+        // since the current active fed reference will change from being 'new' to 'old',
+        // we have to change the UTXOs reference to match it
+        List<UTXO> activeFederationUTXOs = new ArrayList<>(provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations));
 
         // Clear new and old federation's UTXOs
         provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations).clear();
         List<UTXO> oldFederationUTXOs = provider.getOldFederationBtcUTXOs();
         oldFederationUTXOs.clear();
 
-        // Move UTXOs from the new federation into the old federation
-        oldFederationUTXOs.addAll(utxosToMove);
+        // Move UTXOs reference to the old federation
+        oldFederationUTXOs.addAll(activeFederationUTXOs);
     }
 
     /**
diff --git a/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java b/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java
index 23e476004b..c4ef1bf58c 100644
--- a/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java
+++ b/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java
@@ -963,8 +963,9 @@ void registerBtcTransaction_whenValidationPeriodEnded_shouldNotProcessSpendTx()
             arrangeSvpSpendTransaction();
             setUpForTransactionRegistration(svpSpendTransaction);
 
-            // act
             int activeFederationUtxosSizeBeforeRegisteringTx = federationSupport.getActiveFederationBtcUTXOs().size();
+
+            // act
             bridgeSupport.registerBtcTransaction(
                 rskTx,
                 svpSpendTransaction.bitcoinSerialize(),
@@ -974,9 +975,14 @@ void registerBtcTransaction_whenValidationPeriodEnded_shouldNotProcessSpendTx()
             bridgeStorageProvider.save();
 
             // assert
+            // tx was not registered
             assertActiveFederationUtxosSize(activeFederationUtxosSizeBeforeRegisteringTx);
             assertTransactionWasNotProcessed(svpSpendTransaction.getHash());
+
+            // svp success was not processed
             assertSvpSpendTxHashUnsignedIsSavedInStorage();
+            assertNoHandoverToNewFederation();
+            assertProposedFederation();
         }
 
         @Test
@@ -990,8 +996,9 @@ void registerBtcTransaction_whenIsNotTheSpendTransaction_shouldNotProcessSpendTx
             signInputs(pegout);
             setUpForTransactionRegistration(pegout);
 
-            // act
             int activeFederationUtxosSizeBeforeRegisteringTx = federationSupport.getActiveFederationBtcUTXOs().size();
+
+            // act
             bridgeSupport.registerBtcTransaction(
                 rskTx,
                 pegout.bitcoinSerialize(),
@@ -1001,9 +1008,16 @@ void registerBtcTransaction_whenIsNotTheSpendTransaction_shouldNotProcessSpendTx
             bridgeStorageProvider.save();
 
             // assert
+            // pegout was registered
             assertActiveFederationUtxosSize(activeFederationUtxosSizeBeforeRegisteringTx + 1);
+            assertTransactionWasProcessed(pegout.getHash());
+            // but spend tx was not
             assertTransactionWasNotProcessed(svpSpendTransaction.getHash());
+
+            // svp success was not processed
             assertSvpSpendTxHashUnsignedIsSavedInStorage();
+            assertNoHandoverToNewFederation();
+            assertProposedFederation();
         }
 
         @Test
@@ -1012,8 +1026,9 @@ void registerBtcTransaction_whenSpendTransactionHashIsNotSaved_shouldNotProcessS
             recreateSvpSpendTransactionUnsigned();
             setUpForTransactionRegistration(svpSpendTransaction);
 
-            // act
             int activeFederationUtxosSizeBeforeRegisteringTx = federationSupport.getActiveFederationBtcUTXOs().size();
+
+            // act
             bridgeSupport.registerBtcTransaction(
                 rskTx,
                 svpSpendTransaction.bitcoinSerialize(),
@@ -1023,8 +1038,17 @@ void registerBtcTransaction_whenSpendTransactionHashIsNotSaved_shouldNotProcessS
             bridgeStorageProvider.save();
 
             // assert
+            // spend tx was not registered
             assertActiveFederationUtxosSize(activeFederationUtxosSizeBeforeRegisteringTx);
             assertTransactionWasNotProcessed(svpSpendTransaction.getHash());
+
+            // svp success was not processed
+            assertNoHandoverToNewFederation();
+            assertProposedFederation();
+        }
+
+        private void assertProposedFederation() {
+            assertTrue(federationSupport.getProposedFederation().isPresent());
         }
 
         @Test
@@ -1033,8 +1057,9 @@ void registerBtcTransaction_whenIsTheSpendTransaction_shouldProcessSpendTxAndSvp
             arrangeSvpSpendTransaction();
             setUpForTransactionRegistration(svpSpendTransaction);
 
+            List<UTXO> activeFederationUtxosBeforeRegisteringTx = new ArrayList<>(federationSupport.getActiveFederationBtcUTXOs());
+
             // act
-            List<UTXO> activeFedUtxosBeforeRegisteringTx = new ArrayList<>(federationSupport.getActiveFederationBtcUTXOs());
             bridgeSupport.registerBtcTransaction(
                 rskTx,
                 svpSpendTransaction.bitcoinSerialize(),
@@ -1045,18 +1070,13 @@ void registerBtcTransaction_whenIsTheSpendTransaction_shouldProcessSpendTxAndSvp
 
             // assert
             // tx registration
-            assertActiveFederationUtxosSize(activeFedUtxosBeforeRegisteringTx.size() + 1);
+            assertActiveFederationUtxosSize(activeFederationUtxosBeforeRegisteringTx.size() + 1);
             assertTransactionWasProcessed(svpSpendTransaction.getHash());
-            assertNoSvpSpendTxHash();
 
             // svp success
+            assertNoSvpSpendTxHash();
             assertNoProposedFederation();
 
-            assertNewActiveFederationCreationBlockHeightWasSet();
-            assertLastRetiredFederationScriptWasSet();
-
-            assertNewAndOldFederationsWereSet();
-
             int outputSentToActiveFedIndex = 0;
             TransactionOutput spendTxOutputSentToActiveFed = svpSpendTransaction.getOutput(outputSentToActiveFedIndex);
             UTXO utxoSentToActiveFederation = new UTXO(
@@ -1067,21 +1087,43 @@ void registerBtcTransaction_whenIsTheSpendTransaction_shouldProcessSpendTxAndSvp
                 svpSpendTransaction.isCoinBase(),
                 spendTxOutputSentToActiveFed.getScriptPubKey()
             );
-            activeFedUtxosBeforeRegisteringTx.add(utxoSentToActiveFederation);
-            assertUTXOsWereMovedFromNewToOldFederation(activeFedUtxosBeforeRegisteringTx);
+            activeFederationUtxosBeforeRegisteringTx.add(utxoSentToActiveFederation);
+
+            assertHandoverToNewFederation(activeFederationUtxosBeforeRegisteringTx);
         }
 
-        private void assertNewActiveFederationCreationBlockHeightWasSet() {
-            Optional<Long> nextFederationCreationBlockHeight = federationStorageProvider.getNextFederationCreationBlockHeight(allActivations);
-            assertTrue(nextFederationCreationBlockHeight.isPresent());
-            assertEquals(proposedFederation.getCreationBlockNumber(), nextFederationCreationBlockHeight.get());
+        private void assertHandoverToNewFederation(List<UTXO> utxosToMove) {
+            assertUTXOsWereMovedFromNewToOldFederation(utxosToMove);
+            assertNewAndOldFederationsWereSet();
+            assertLastRetiredFederationScriptWasSet();
+            assertNewActiveFederationCreationBlockHeightWasSet();
         }
 
-        private void assertLastRetiredFederationScriptWasSet() {
-            Script activeFederationMembersP2SHScript = getFederationMembersP2SHScript(allActivations, federationSupport.getActiveFederation());
-            Optional<Script> lastRetiredFederationP2SHScript = federationStorageProvider.getLastRetiredFederationP2SHScript(allActivations);
-            assertTrue(lastRetiredFederationP2SHScript.isPresent());
-            assertEquals(activeFederationMembersP2SHScript, lastRetiredFederationP2SHScript.get());
+        private void assertNoHandoverToNewFederation() {
+            assertUTXOsWereNotMovedToOldFederation();
+            assertNewAndOldFederationsWereNotSet();
+            assertLastRetiredFederationScriptWasNotSet();
+            assertNewActiveFederationCreationBlockHeightWasNotSet();
+        }
+
+        private void assertUTXOsWereMovedFromNewToOldFederation(List<UTXO> utxosToMove) {
+            // assert utxos were moved from new federation to old federation
+            List<UTXO> oldFederationUTXOs = federationStorageProvider.getOldFederationBtcUTXOs();
+            assertEquals(utxosToMove, oldFederationUTXOs);
+
+            // assert new federation utxos were cleaned
+            List<UTXO> newFederationUTXOs = federationStorageProvider.getNewFederationBtcUTXOs(btcMainnetParams, allActivations);
+            assertTrue(newFederationUTXOs.isEmpty());
+        }
+
+        private void assertUTXOsWereNotMovedToOldFederation() {
+            // assert old federation utxos are still empty
+            List<UTXO> oldFederationUTXOs = federationStorageProvider.getOldFederationBtcUTXOs();
+            assertTrue(oldFederationUTXOs.isEmpty());
+
+            // assert new federation utxos are not empty
+            List<UTXO> newFederationUTXOs = federationStorageProvider.getNewFederationBtcUTXOs(btcMainnetParams, allActivations);
+            assertFalse(newFederationUTXOs.isEmpty());
         }
 
         private void assertNewAndOldFederationsWereSet() {
@@ -1094,14 +1136,37 @@ private void assertNewAndOldFederationsWereSet() {
             assertEquals(proposedFederation, newFederation);
         }
 
-        private void assertUTXOsWereMovedFromNewToOldFederation(List<UTXO> utxosToMove) {
-            // assert utxos were moved from new federation to old federation
-            List<UTXO> oldFederationUTXOs = federationStorageProvider.getOldFederationBtcUTXOs();
-            assertEquals(utxosToMove, oldFederationUTXOs);
+        private void assertNewAndOldFederationsWereNotSet() {
+            // assert old federation is still null
+            Federation oldFederation = federationStorageProvider.getOldFederation(federationMainNetConstants, allActivations);
+            assertNull(oldFederation);
 
-            // assert new federation utxos were cleaned
-            List<UTXO> newFederationUTXOs = federationStorageProvider.getNewFederationBtcUTXOs(btcMainnetParams, allActivations);
-            assertTrue(newFederationUTXOs.isEmpty());
+            // assert new federation is still the active federation
+            Federation newFederation = federationStorageProvider.getNewFederation(federationMainNetConstants, allActivations);
+            assertEquals(activeFederation, newFederation);
+        }
+
+        private void assertLastRetiredFederationScriptWasSet() {
+            Script activeFederationMembersP2SHScript = getFederationMembersP2SHScript(allActivations, federationSupport.getActiveFederation());
+            Optional<Script> lastRetiredFederationP2SHScript = federationStorageProvider.getLastRetiredFederationP2SHScript(allActivations);
+            assertTrue(lastRetiredFederationP2SHScript.isPresent());
+            assertEquals(activeFederationMembersP2SHScript, lastRetiredFederationP2SHScript.get());
+        }
+
+        private void assertLastRetiredFederationScriptWasNotSet() {
+            Optional<Script> lastRetiredFederationP2SHScript = federationStorageProvider.getLastRetiredFederationP2SHScript(allActivations);
+            assertFalse(lastRetiredFederationP2SHScript.isPresent());
+        }
+
+        private void assertNewActiveFederationCreationBlockHeightWasSet() {
+            Optional<Long> nextFederationCreationBlockHeight = federationStorageProvider.getNextFederationCreationBlockHeight(allActivations);
+            assertTrue(nextFederationCreationBlockHeight.isPresent());
+            assertEquals(proposedFederation.getCreationBlockNumber(), nextFederationCreationBlockHeight.get());
+        }
+
+        private void assertNewActiveFederationCreationBlockHeightWasNotSet() {
+            Optional<Long> nextFederationCreationBlockHeight = federationStorageProvider.getNextFederationCreationBlockHeight(allActivations);
+            assertFalse(nextFederationCreationBlockHeight.isPresent());
         }
     }
 
diff --git a/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java b/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java
index 97d6a01b7e..8270e8499e 100644
--- a/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java
+++ b/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java
@@ -38,7 +38,6 @@
 import static co.rsk.peg.BridgeUtils.getFederationMembersP2SHScript;
 import static co.rsk.peg.bitcoin.BitcoinTestUtils.flatKeysAsByteArray;
 import static co.rsk.peg.federation.FederationStorageIndexKey.NEW_FEDERATION_BTC_UTXOS_KEY;
-import static org.ethereum.config.blockchain.upgrades.ConsensusRule.RSKIP377;
 import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.Mockito.mock;
 
@@ -387,9 +386,11 @@ void voteCommitFederation_preRSKIP186_whenPendingFederationIsSet_shouldPerformLe
             pendingFederationToBe.buildFederation(Instant.ofEpochMilli(RSK_EXECUTION_BLOCK_TIMESTAMP), RSK_EXECUTION_BLOCK_NUMBER, federationMainnetConstants, activations);
         assertNewAndOldFederationsWereSet(federationBuiltFromPendingFederation);
 
-        assertPendingFederationVotingWasCleaned();
+        // pre rskip186 these values are not being set
+        assertNewActiveFederationCreationBlockHeightWasNotSet();
+        assertLastRetiredFederationScriptWasNotSet();
 
-        assertFederationChangeInfoWasNotSet();
+        assertPendingFederationVotingWasCleaned();
 
         Federation oldFederation = storageProvider.getOldFederation(federationMainnetConstants, activations);
         Federation newFederation = storageProvider.getNewFederation(federationMainnetConstants, activations);
@@ -416,16 +417,12 @@ void voteCommitFederation_postRSKIP186_preRSKIP419_whenPendingFederationIsSet_sh
 
         // assertions
         List<UTXO> utxosToMove = new ArrayList<>(storageProvider.getNewFederationBtcUTXOs(federationMainnetConstants.getBtcParams(), activations));
-        assertUTXOsWereMovedFromNewToOldFederation(utxosToMove);
-
         Federation federationBuiltFromPendingFederation =
             pendingFederationToBe.buildFederation(Instant.ofEpochMilli(RSK_EXECUTION_BLOCK_TIMESTAMP), RSK_EXECUTION_BLOCK_NUMBER, federationMainnetConstants, activations);
-        assertNewAndOldFederationsWereSet(federationBuiltFromPendingFederation);
+        assertHandoverToNewFederation(utxosToMove, federationBuiltFromPendingFederation);
 
         assertPendingFederationVotingWasCleaned();
 
-        assertFederationChangeInfoWasSet(RSK_EXECUTION_BLOCK_NUMBER);
-
         Federation oldFederation = storageProvider.getOldFederation(federationMainnetConstants, activations);
         Federation newFederation = storageProvider.getNewFederation(federationMainnetConstants, activations);
         assertLogCommitFederation(oldFederation, newFederation);
@@ -435,6 +432,9 @@ void voteCommitFederation_postRSKIP186_preRSKIP419_whenPendingFederationIsSet_sh
     void voteCommitFederation_postRSKIP419_whenPendingFederationIsSet_shouldPerformCommitFederationActions() {
         // arrange
         Federation activeFederation = federationSupport.getActiveFederation();
+        List<UTXO> activeFederationUTXOs = BitcoinTestUtils.createUTXOs(10, activeFederation.getAddress());
+        bridgeStorageAccessor.saveToRepository(NEW_FEDERATION_BTC_UTXOS_KEY.getKey(), activeFederationUTXOs, BridgeSerializationUtils::serializeUTXOList);
+
         Federation federationBuiltFromPendingFederation =
             pendingFederationToBe.buildFederation(Instant.ofEpochMilli(RSK_EXECUTION_BLOCK_TIMESTAMP), RSK_EXECUTION_BLOCK_NUMBER, federationMainnetConstants, activations);
 
@@ -454,10 +454,7 @@ void voteCommitFederation_postRSKIP419_whenPendingFederationIsSet_shouldPerformC
 
         assertLogCommitFederation(activeFederation, proposedFederation.get());
 
-        // assert some values were not set
-        assertFederationChangeInfoWasNotSet();
-        assertNewAndOldFederationsWereNotSet();
-        assertUTXOsWereNotMovedFromNewToOldFederation();
+        assertNoHandoverToNewFederation();
     }
 
     @Test
@@ -477,11 +474,7 @@ void commitProposedFederation_shouldPerformCommitProposedFederationActions() {
 
         // assert
         assertFalse(federationSupport.getProposedFederation().isPresent());
-
-        assertFederationChangeInfoWasSet(proposedFederation.getCreationBlockNumber());
-
-        assertNewAndOldFederationsWereSet(proposedFederation);
-        assertUTXOsWereMovedFromNewToOldFederation(activeFederationUTXOs);
+        assertHandoverToNewFederation(activeFederationUTXOs, proposedFederation);
     }
 
     private void voteAndAssertAddFederationMembersPublicKeysToPendingFederation(List<FederationMember> federationMembers) {
@@ -494,6 +487,20 @@ private void voteAndAssertAddFederationMembersPublicKeysToPendingFederation(List
         }
     }
 
+    private void assertHandoverToNewFederation(List<UTXO> utxosToMove, Federation newFederation) {
+        assertUTXOsWereMovedFromNewToOldFederation(utxosToMove);
+        assertNewAndOldFederationsWereSet(newFederation);
+        assertLastRetiredFederationScriptWasSet();
+        assertNewActiveFederationCreationBlockHeightWasSet(newFederation.getCreationBlockNumber());
+    }
+
+    private void assertNoHandoverToNewFederation() {
+        assertUTXOsWereNotMovedFromNewToOldFederation();
+        assertNewAndOldFederationsWereNotSet();
+        assertLastRetiredFederationScriptWasNotSet();
+        assertNewActiveFederationCreationBlockHeightWasNotSet();
+    }
+
     private void assertUTXOsWereMovedFromNewToOldFederation(List<UTXO> utxosToMove) {
         // assert utxos were moved from new federation to old federation
         List<UTXO> oldFederationUTXOs = storageProvider.getOldFederationBtcUTXOs();
@@ -505,12 +512,13 @@ private void assertUTXOsWereMovedFromNewToOldFederation(List<UTXO> utxosToMove)
     }
 
     private void assertUTXOsWereNotMovedFromNewToOldFederation() {
-        // assert old and new federation utxos are still empty
+        // assert old federation utxos are still empty
         List<UTXO> oldFederationUTXOs = storageProvider.getOldFederationBtcUTXOs();
-        List<UTXO> newFederationUTXOs = storageProvider.getNewFederationBtcUTXOs(federationMainnetConstants.getBtcParams(), activations);
-
         assertTrue(oldFederationUTXOs.isEmpty());
-        assertTrue(newFederationUTXOs.isEmpty());
+
+        // assert new federation utxos are not empty
+        List<UTXO> newFederationUTXOs = storageProvider.getNewFederationBtcUTXOs(federationMainnetConstants.getBtcParams(), activations);
+        assertFalse(newFederationUTXOs.isEmpty());
     }
 
     private void assertNewAndOldFederationsWereSet(Federation expectedNewFederation) {
@@ -539,11 +547,6 @@ private void assertPendingFederationVotingWasCleaned() {
         assertTrue(federationElectionVotes.isEmpty());
     }
 
-    private void assertFederationChangeInfoWasSet(long expectedNextFederationCreationBlockHeight) {
-        assertNewActiveFederationCreationBlockHeightWasSet(expectedNextFederationCreationBlockHeight);
-        assertLastRetiredFederationScriptWasSet();
-    }
-
     private void assertNewActiveFederationCreationBlockHeightWasSet(long expectedNextFederationCreationBlockHeight) {
         Optional<Long> nextFederationCreationBlockHeight = storageProvider.getNextFederationCreationBlockHeight(activations);
         assertTrue(nextFederationCreationBlockHeight.isPresent());
@@ -567,11 +570,6 @@ private void assertLastRetiredFederationScriptWasNotSet() {
         assertFalse(lastRetiredFederationP2SHScript.isPresent());
     }
 
-    private void assertFederationChangeInfoWasNotSet() {
-        assertNewActiveFederationCreationBlockHeightWasNotSet();
-        assertLastRetiredFederationScriptWasNotSet();
-    }
-
     private void assertLogCommitFederation(Federation federationToBeRetired, Federation votedFederation) {
         List<DataWord> encodedTopics = getEncodedTopics();
         byte[] encodedData = getEncodedData(federationToBeRetired, votedFederation);