turn getRetiringFederationAddress to return an optional, added two tests to BridgeIT to check the returned string is correct

Author: julianlen

rskj-core/src/main/java/co/rsk/peg/Bridge.java

@@ -860,14 +860,11 @@ public long getFederationCreationBlockNumber(Object[] args) {
     public String getRetiringFederationAddress(Object[] args) {
         logger.trace("getRetiringFederationAddress");
 
-        Address address = bridgeSupport.getRetiringFederationAddress();
+        Optional<Address> address = bridgeSupport.getRetiringFederationAddress();
 
-        if (address == null) {
-            // When there's no address, empty string is returned
-            return "";
-        }
+        // When there's no address, empty string is returned
+        return address.map(Address::toBase58).orElse("");
 
-        return address.toBase58();
     }
 
     public Integer getRetiringFederationSize(Object[] args) {

rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java

@@ -1271,10 +1271,11 @@ private void processFundsMigration(Transaction rskTx) throws IOException {
                         availableUTXOs
                     );
                 } catch (Exception e) {
+                    // Funds cannot be migrated if there is no retiring federation
                     logger.error(
                         "[processFundsMigration] Unable to complete retiring federation migration. Balance left: {} in {}",
                         retiringFederationWallet.getBalance().toFriendlyString(),
-                        getRetiringFederationAddress()
+                        getRetiringFederationAddress().get()
                     );
                     panicProcessor.panic("updateCollection", "Unable to complete retiring federation migration.");
                 }
@@ -2334,7 +2335,7 @@ public long getActiveFederationCreationBlockNumber() {
         return federationSupport.getActiveFederationCreationBlockNumber();
     }
 
-    public Address getRetiringFederationAddress() {
+    public Optional<Address> getRetiringFederationAddress() {
         return federationSupport.getRetiringFederationAddress();
     }
 

rskj-core/src/main/java/co/rsk/peg/federation/FederationSupport.java

@@ -83,10 +83,12 @@ public interface FederationSupport {
     Optional<Federation> getRetiringFederation();
 
     /**
-     * Returns the retiring federation bitcoin address.
-     * @return the retiring federation bitcoin address, null if no retiring federation exists
+     * Returns the retiring federation bitcoin address, if it exists.
+     *
+     * @return An {@link Optional} containing the retiring federation address,
+     * or an empty {@link Optional} if no retiring federation exists
      */
-    Address getRetiringFederationAddress();
+    Optional<Address> getRetiringFederationAddress();
 
     /**
      * Returns the currently retiring federation size, if it exists.

rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java

@@ -215,9 +215,9 @@ private StorageFederationReference getRetiringFederationReference() {
     }
 
     @Override
-    public Address getRetiringFederationAddress() {
+    public Optional<Address> getRetiringFederationAddress() {
         Optional<Federation> retiringFederation = getRetiringFederation();
-        return retiringFederation.map(Federation::getAddress).orElse(null);
+        return retiringFederation.map(Federation::getAddress);
     }
 
     @Override

rskj-core/src/test/java/co/rsk/peg/BridgeIT.java

@@ -1968,6 +1968,25 @@ void getRetiringFederationCreationBlockNumber() {
         MatcherAssert.assertThat(bridge.getRetiringFederationCreationBlockNumber(new Object[]{}), is(retiringFederationCreationBlockNumber));
     }
 
+    @Test
+    void getRetiringFederationAddress_withNoRetiringFederation_shouldReturnEmptyString() {
+        Bridge bridge = new Bridge(BRIDGE_ADDRESS, constants, activationConfig, null, signatureCache);
+        BridgeSupport bridgeSupportMock = mock(BridgeSupport.class);
+        TestUtils.setInternalState(bridge, "bridgeSupport", bridgeSupportMock);
+        when(bridgeSupportMock.getRetiringFederation()).thenReturn(Optional.empty());
+        MatcherAssert.assertThat(bridge.getRetiringFederationAddress(new Object[]{}), is(""));
+    }
+
+    @Test
+    void getRetiringFederationAddress_withRetiringFederation_shouldReturnTheCorrectRetiringFederationAddress() {
+        Bridge bridge = new Bridge(BRIDGE_ADDRESS, constants, activationConfig, null, signatureCache);
+        BridgeSupport bridgeSupportMock = mock(BridgeSupport.class);
+        TestUtils.setInternalState(bridge, "bridgeSupport", bridgeSupportMock);
+        Address retiringFederationAddress = new BtcECKey().toAddress(regtestParameters);
+        when(bridgeSupportMock.getRetiringFederationAddress()).thenReturn(Optional.of(retiringFederationAddress));
+        MatcherAssert.assertThat(bridge.getRetiringFederationAddress(new Object[]{}), is(retiringFederationAddress.toBase58()));
+    }
+
     @Test
     void getRetiringFederatorPublicKey_beforeMultikey() throws Exception {
         doReturn(false).when(activationConfig).isActive(eq(RSKIP123), anyLong());

rskj-core/src/test/java/co/rsk/peg/BridgeSupportIT.java

@@ -2489,7 +2489,9 @@ void getRetiringFederationMethods_presentNewActive() {
         assertTrue(retiringFederationCreationTime.isPresent());
         assertEquals(1000, retiringFederationCreationTime.get().toEpochMilli());
 
-        assertEquals(mockedOldFederation.getAddress().toString(), bridgeSupport.getRetiringFederationAddress().toString());
+        Optional<Address> retiringFederationAddress = bridgeSupport.getRetiringFederationAddress();
+        assertTrue(retiringFederationAddress.isPresent());
+        assertEquals(mockedOldFederation.getAddress().toString(), retiringFederationAddress.get().toString());
         List<FederationMember> members = FederationTestUtils.getFederationMembers(4);
         for (int i = 0; i < 4; i++) {
             assertArrayEquals(

rskj-core/src/test/java/co/rsk/peg/BridgeSupportTest.java

@@ -469,8 +469,9 @@ void getRetiringFederation() {
         void getRetiringFederationAddress() {
             Address address = federation.getAddress();
 
-            when(federationSupport.getRetiringFederationAddress()).thenReturn(address);
-            assertThat(bridgeSupport.getRetiringFederationAddress(), is(address));
+            Optional<Address> retiringFederationAddressOptional = Optional.of(address);
+            when(federationSupport.getRetiringFederationAddress()).thenReturn(retiringFederationAddressOptional);
+            assertThat(bridgeSupport.getRetiringFederationAddress(), is(retiringFederationAddressOptional));
         }
 
         @Test

rskj-core/src/test/java/co/rsk/peg/BridgeTest.java

@@ -2040,7 +2040,7 @@ void getRetiringFederationAddress(MessageCall.MsgType msgType, ActivationConfig
 
         BridgeSupport bridgeSupportMock = mock(BridgeSupport.class);
         Address retiringFederationAddress = Address.fromBase58(networkParameters, "32Bhwee9FzQbuaG29RcXpdrvYnvZeMk11M");
-        when(bridgeSupportMock.getRetiringFederationAddress()).thenReturn(retiringFederationAddress);
+        when(bridgeSupportMock.getRetiringFederationAddress()).thenReturn(Optional.of(retiringFederationAddress));
 
         Bridge bridge = bridgeBuilder
             .transaction(rskTxMock)

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

@@ -879,7 +879,9 @@ private void assertNewAndOldFederationsReferences(Federation expectedNewFederati
 
     private void assertActiveAndRetiringFederationsHaveExpectedAddress(Address expectedNewFederationAddress, Address expectedOldFederationAddress) {
         assertEquals(expectedNewFederationAddress, bridgeSupport.getActiveFederationAddress());
-        assertEquals(expectedOldFederationAddress, bridgeSupport.getRetiringFederationAddress());
+        Optional<Address> retiringFederationAddress = bridgeSupport.getRetiringFederationAddress();
+        assertTrue(retiringFederationAddress.isPresent());
+        assertEquals(expectedOldFederationAddress, retiringFederationAddress.get());
     }
 
     private void assertNextFederationCreationBlockHeight(long newFederationCreationBlockNumber) {
@@ -907,7 +909,8 @@ private void assertMigrationHasStarted() throws Exception {
     private void assertOnlyActiveFedIsLive(Federation newFederation) {
         // New active federation still there, retiring federation no longer there
         assertEquals(newFederation, bridgeSupport.getActiveFederation());
-        assertNull(bridgeSupport.getRetiringFederationAddress());
+        Optional<Address> retiringFederationAddress = bridgeSupport.getRetiringFederationAddress();
+        assertTrue(retiringFederationAddress.isEmpty());
     }
 
     private void assertLastRetiredFederationP2SHScriptMatchesWithOriginalFederation(

rskj-core/src/test/java/co/rsk/peg/federation/FederationSupportImplTest.java

@@ -1277,9 +1277,9 @@ void getRetiringFederation_returnsEmpty() {
 
         @Test
         @Tag("getRetiringFederationAddress")
-        void getRetiringFederationAddress_returnsNull() {
-            Address retiringFederationAddress = federationSupport.getRetiringFederationAddress();
-            assertThat(retiringFederationAddress, is(nullValue()));
+        void getRetiringFederationAddress_returnsEmpty() {
+            Optional<Address> retiringFederationAddress = federationSupport.getRetiringFederationAddress();
+            assertTrue(retiringFederationAddress.isEmpty());
         }
 
         @Test
@@ -1366,9 +1366,9 @@ void getRetiringFederation_returnsEmpty() {
 
         @Test
         @Tag("getRetiringFederationAddress")
-        void getRetiringFederationAddress_returnsNull() {
-            Address retiringFederationAddress = federationSupport.getRetiringFederationAddress();
-            assertThat(retiringFederationAddress, is(nullValue()));
+        void getRetiringFederationAddress_returnsEmpty() {
+            Optional<Address> retiringFederationAddress = federationSupport.getRetiringFederationAddress();
+            assertTrue(retiringFederationAddress.isEmpty());
         }
 
         @Test
@@ -1518,7 +1518,7 @@ void getRetiringFederation_withNewFederationActive_returnsOldFederation(
         @ParameterizedTest
         @Tag("getRetiringFederationAddress")
         @MethodSource("newFederationNotActiveActivationArgs")
-        void getRetiringFederationAddress_withNewFederationNotActive_returnsNull(
+        void getRetiringFederationAddress_withNewFederationNotActive_returnsEmpty(
             long currentBlock,
             ActivationConfig.ForBlock activations) {
 
@@ -1532,8 +1532,8 @@ void getRetiringFederationAddress_withNewFederationNotActive_returnsNull(
                 .withActivations(activations)
                 .build();
 
-            Address retiringFederationAddress = federationSupport.getRetiringFederationAddress();
-            assertThat(retiringFederationAddress, is(nullValue()));
+            Optional<Address> retiringFederationAddress = federationSupport.getRetiringFederationAddress();
+            assertTrue(retiringFederationAddress.isEmpty());
         }
 
         @ParameterizedTest
@@ -1553,8 +1553,9 @@ void getRetiringFederationAddress_withNewFederationActive_returnsOldFederationAd
                 .withActivations(activations)
                 .build();
 
-            Address retiringFederationAddress = federationSupport.getRetiringFederationAddress();
-            assertThat(retiringFederationAddress, is(oldFederation.getAddress()));
+            Optional<Address> retiringFederationAddress = federationSupport.getRetiringFederationAddress();
+            assertTrue(retiringFederationAddress.isPresent());
+            assertThat(retiringFederationAddress.get(), is(oldFederation.getAddress()));
         }
 
         @ParameterizedTest