Implement getProposedFederation in FederationProviderFromFederatorSupport #329

apancorb · 2024-10-25T11:19:23Z

Summary

This PR includes the implementation of getProposedFederation. As well as bridge RPC methods for specific proposed federation implementations for getFederationSize, getFederatorPublicKeyOfType,getFederationCreationTime, and getFederationCreationBlockNumber.

Motivation and Context

https://github.com/rsksmart/RSKIPs/blob/master/IPs/RSKIP419.md

…upport

…port

…atorSupport

…face

…tionProviderFromFederatorSupport

…nal check for building retiring federation

…Number

src/main/java/co/rsk/federate/FederationProvider.java

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java

src/main/java/co/rsk/federate/FederatorSupport.java

nathanieliov · 2024-10-29T18:56:34Z

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java

+        );
+
+        Federation initialFederation = FederationFactory.buildStandardMultiSigFederation(federationArgs);
+        Federation expectedFederation = getExpectedFederation(initialFederation, proposedFederationAddress.get());


I see how getExpectedFederation builds the federation, and I think this method is unsuitable for this case. At the beginning, you have already checked if RSKIP419 is active, so we can be sure that the proposed federation will be a P2shFederation. if not, I would like to understand why you would be a different type?

Thanks for pointing this out :) , I have made the changes. Let me know if this looks good now.

src/test/java/co/rsk/federate/FederationProviderFromFederatorSupportTest.java

apancorb · 2024-10-30T11:17:08Z

src/main/java/co/rsk/federate/FederationProvider.java

+     * and the retiring federation, if one exists.
+     *
+     * @return a {@link List} of live {@link Federation} instances
+     */
    List<Federation> getLiveFederations();


We can probably remove this method, is not being used anywhere at the moment. Maybe in another PR.

Lets create a task for that

…exists

…deration exists any related data is missing

…d ones

…s a P2shErpFederation

…realistic

… var

…eException::new'

src/main/java/co/rsk/federate/FederationProvider.java

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java

…ate exisiting fed

…eing called to check if proposed fed exists

nathanieliov · 2024-10-31T12:11:26Z

src/main/java/co/rsk/federate/FederationProvider.java

+     * and the retiring federation, if one exists.
+     *
+     * @return a {@link List} of live {@link Federation} instances
+     */
    List<Federation> getLiveFederations();


src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java

src/test/java/co/rsk/federate/FederationProviderFromFederatorSupportTest.java

src/test/java/co/rsk/federate/FederatorSupportTest.java

…able to be built

…t because some data is missing from the Bridge

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java

julia-zack · 2024-10-31T16:30:35Z

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java

+        Integer federationSize = federatorSupport.getProposedFederationSize()
+            .orElse(FEDERATION_NON_EXISTENT.getCode());
+        if (federationSize == FEDERATION_NON_EXISTENT.getCode()) {
+            return Optional.empty();
+        }


Suggested change

Integer federationSize = federatorSupport.getProposedFederationSize()

.orElse(FEDERATION_NON_EXISTENT.getCode());

if (federationSize == FEDERATION_NON_EXISTENT.getCode()) {

return Optional.empty();

}

Optional<Integer> federationSize = federatorSupport.getProposedFederationSize();

if (federationSize.isEmpty()) {

return Optional.empty();

}

wdyt?

This would work if the Bridge returned only a null value for the federation size if the proposed fed did not exist. But it can return FEDERATION_NON_EXISTENT which would be present in the optional. We would have to check this before continuing.

pretty sure the FEDERATION_NON_EXISTENT code is not being used for the proposed fed, but for the pending and retiring.

It was implemented here.

my bad, you right. But in that case it wont be necessary to have an optional right? Because you are always getting a value from the Bridge

src/test/java/co/rsk/federate/FederatorSupportTest.java

…ception while building the proposed federation

…osed federation

sonarcloud · 2024-11-04T16:01:11Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
86.9% Coverage on New Code
1.4% Duplication on New Code

See analysis details on SonarCloud

julia-zack · 2024-11-04T17:01:52Z

src/test/java/co/rsk/federate/FederationProviderFromFederatorSupportTest.java

+    @Test
+    void getProposedFederation_whenRSKIP419IsNotActivated_shouldReturnEmptyOptional() {
+        // Arrange
+        ActivationConfig.ForBlock configMock = mock(ActivationConfig.ForBlock.class);
+        when(federatorSupportMock.getConfigForBestBlock()).thenReturn(configMock);
+
+        // Act & Assert
+        assertEquals(Optional.empty(), federationProvider.getProposedFederation());
+    }


probably this test can be removed

apancorb added 12 commits October 25, 2024 09:56

feat(federate): add getProposedFederationSize to FederatorSupport

493d605

feat(federate): add getProposedFederatorPublicKeyOfType to FederatorS…

3f34e6f

…upport

feat(federate): add getProposedFederationCreationTime to FederatorSup…

de08dff

…port

feat(federate): add getProposedFederationCreationBlockNumber to Feder…

7aaac68

…atorSupport

feat(federate): add getProposedFederation to FederationProvider inter…

e3f9281

…face

feat(federate): add implementation of getProposedFederation to Federa…

d9b22f0

…tionProviderFromFederatorSupport

refactor(federate): use FEDERATION_NON_EXISTENT enum code in conditio…

cf67d6d

…nal check for building retiring federation

feat(federate): add unit tests for getProposedFederationSize

3701805

feat(federate): add unit tests for getProposedFederatorPublicKeyOfType

0c0cd75

feat(federate): add unit tests for getProposedFederationCreationTime

ba9e51f

feat(federate): add unit tests for getProposedFederationCreationBlock…

cda8673

…Number

feat(federate): add unit tests for getProposedFederation

f91e644

apancorb self-assigned this Oct 25, 2024

fix(federate): use proper activation rule for svp

7fb3754

apancorb marked this pull request as ready for review October 25, 2024 12:50

apancorb requested a review from a team as a code owner October 25, 2024 12:50

jeremy-then reviewed Oct 28, 2024

View reviewed changes

src/main/java/co/rsk/federate/FederationProvider.java Outdated Show resolved Hide resolved

nathanieliov reviewed Oct 29, 2024

View reviewed changes

src/test/java/co/rsk/federate/FederationProviderFromFederatorSupportTest.java Outdated Show resolved Hide resolved

nathanieliov reviewed Oct 29, 2024

View reviewed changes

src/test/java/co/rsk/federate/FederationProviderFromFederatorSupportTest.java Outdated Show resolved Hide resolved

refactor(federate): improve javadocs for FederationProvider interface

9b883fd

apancorb commented Oct 30, 2024

View reviewed changes

apancorb added 7 commits October 30, 2024 11:20

refactor(federate): use isEmpty when checking if retiring federation …

23c0dea

…exists

feat(federate): throw exception if after proving that the proposed fe…

9f6499e

…deration exists any related data is missing

refactor(federate): method call consistent with all the other propose…

b4c5cf2

…d ones

feat(federate): after rskip419 the only possible type of federation i…

88515c5

…s a P2shErpFederation

feat(federate): remove unit tests for proposed federation that are un…

5b6b6d8

…realistic

fix(federate): update var name to not shadow global federator support…

d8f5ac1

… var

refactor(federate): replace lambda with method reference 'IllegalStat…

6fcf573

…eException::new'

refactor(federate): improve sonar complaints of FederateSupport

ef5f2b3

julia-zack reviewed Oct 30, 2024

View reviewed changes

src/main/java/co/rsk/federate/FederationProvider.java Outdated Show resolved Hide resolved

julia-zack reviewed Oct 30, 2024

View reviewed changes

src/main/java/co/rsk/federate/FederationProvider.java Outdated Show resolved Hide resolved

julia-zack reviewed Oct 30, 2024

View reviewed changes

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java Show resolved Hide resolved

julia-zack reviewed Oct 30, 2024

View reviewed changes

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java Outdated Show resolved Hide resolved

apancorb added 3 commits October 31, 2024 10:24

refactor(federate): make FederationProvider javadoc more concise

a710693

feat(federate): remove checking the proposed federation addr to valid…

9342f3a

…ate exisiting fed

feat(federate): remove unit tests verifying if proposed fed addr is b…

11174e5

…eing called to check if proposed fed exists

nathanieliov reviewed Oct 31, 2024

View reviewed changes

apancorb added 2 commits October 31, 2024 13:18

feat(federate): add try-catch block in case proposed federation is un…

78a6d40

…able to be built

feat(federate): add unit test when proposed federation cannot be buil…

ff2a59a

…t because some data is missing from the Bridge

nathanieliov approved these changes Oct 31, 2024

View reviewed changes

julia-zack reviewed Oct 31, 2024

View reviewed changes

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java Outdated Show resolved Hide resolved

julia-zack reviewed Oct 31, 2024

View reviewed changes

src/test/java/co/rsk/federate/FederatorSupportTest.java Outdated Show resolved Hide resolved

julia-zack reviewed Oct 31, 2024

View reviewed changes

src/test/java/co/rsk/federate/FederatorSupportTest.java Outdated Show resolved Hide resolved

julia-zack reviewed Oct 31, 2024

View reviewed changes

src/test/java/co/rsk/federate/FederatorSupportTest.java Show resolved Hide resolved

apancorb added 5 commits November 1, 2024 10:55

refactor(federate): rename unit tests

3889525

refactor(federate): use ECKey class to build pub key in unit tests

d39960f

feat(federate): change catch exception class to catch all possible ex…

e2d9b98

…ception while building the proposed federation

refactor(federate): remove checking if RSKIP419 is activated for prop…

5f116c1

…osed federation

feat(federate): remove RSKIP419 reference for unit tests

d14d1ba

julia-zack reviewed Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement getProposedFederation in FederationProviderFromFederatorSupport #329

Implement getProposedFederation in FederationProviderFromFederatorSupport #329

apancorb commented Oct 25, 2024

nathanieliov Oct 29, 2024

apancorb Oct 30, 2024

apancorb Oct 30, 2024 •

edited

Loading

julia-zack Oct 30, 2024

nathanieliov Oct 31, 2024

nathanieliov Oct 31, 2024

julia-zack Oct 31, 2024

apancorb Nov 1, 2024

julia-zack Nov 4, 2024

apancorb Nov 4, 2024

julia-zack Nov 4, 2024

sonarcloud bot commented Nov 4, 2024

julia-zack Nov 4, 2024

Implement getProposedFederation in FederationProviderFromFederatorSupport #329

Are you sure you want to change the base?

Implement getProposedFederation in FederationProviderFromFederatorSupport #329

Conversation

apancorb commented Oct 25, 2024

Summary

Motivation and Context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apancorb Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 4, 2024

Quality Gate passed

Choose a reason for hiding this comment

apancorb Oct 30, 2024 •

edited

Loading