Skip to content

Turn retiring federation creation block number optional #3427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
marcos-iov merged 2 commits into from
Dec 22, 2025
Merged

Turn retiring federation creation block number optional #3427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6b9f81
turn getRetiringFederationCreationBlockNumber to return an optional
julianlen Dec 19, 2025
a662892
fixed some tests
julianlen Dec 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion rskj-core/src/main/java/co/rsk/peg/Bridge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,8 @@ private Long getFederationCreationTimeEpochBasedOnActivation(Instant retiringFed

Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method documentation should be updated to clarify that it returns the sentinel value FEDERATION_NON_EXISTENT.getCode() when no retiring federation exists, since this behavior is no longer obvious from the implementation.

Suggested change
/**
* Returns the block number at which the current retiring federation was created.
* <p>
* If there is no retiring federation, this method returns the sentinel value
* {@link FederationChangeResponseCode#FEDERATION_NON_EXISTENT#getCode()}.
*
* @param args unused arguments array required by the precompiled contract interface
* @return the retiring federation creation block number, or
* {@code FederationChangeResponseCode.FEDERATION_NON_EXISTENT.getCode()} if
* no retiring federation exists
*/

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@julianlen julianlen Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wat

Copy link
Contributor

@jeremy-then jeremy-then Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is suggesting this because other functions returning 'orElse' have some documentation.
What do you think if we just add some simple doc?

getRetiringFederationCreationTime, getRetiringFederationCreationBlockNumber.

There are others like getLockWhitelistAddress and getActivePowpegRedeemScript but are out of the scope of this ticket.

What do you think?

public long getRetiringFederationCreationBlockNumber(Object[] args) {
logger.trace("getRetiringFederationCreationBlockNumber");
return bridgeSupport.getRetiringFederationCreationBlockNumber();
return bridgeSupport.getRetiringFederationCreationBlockNumber()
.orElse((long) FederationChangeResponseCode.FEDERATION_NON_EXISTENT.getCode());
}

public Integer createFederation(Object[] args) {
Expand Down
2 changes: 1 addition & 1 deletion rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2358,7 +2358,7 @@ public Optional<Instant> getRetiringFederationCreationTime() {
return federationSupport.getRetiringFederationCreationTime();
}

public long getRetiringFederationCreationBlockNumber() {
public Optional<Long> getRetiringFederationCreationBlockNumber() {
return federationSupport.getRetiringFederationCreationBlockNumber();
}

Expand Down
9 changes: 5 additions & 4 deletions rskj-core/src/main/java/co/rsk/peg/federation/FederationSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,12 @@ public interface FederationSupport {
Optional<Instant> getRetiringFederationCreationTime();

/**
* Returns the retiring federation's creation block number
* @return the retiring federation creation block number,
* -1 if no retiring federation exists
* Returns the retiring federation's creation block number, if it exists.
*
* @return An {@link Optional} containing the retiring federation's creation block number,
* or an empty {@link Optional} if no retiring federation exists.
*/
long getRetiringFederationCreationBlockNumber();
Optional<Long> getRetiringFederationCreationBlockNumber();

/**
* Returns the public key of the retiring federation's federator at the given index
Expand Down
5 changes: 2 additions & 3 deletions rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,10 +239,9 @@ public Optional<Instant> getRetiringFederationCreationTime() {
}

@Override
public long getRetiringFederationCreationBlockNumber() {
public Optional<Long> getRetiringFederationCreationBlockNumber() {
Optional<Federation> retiringFederation = getRetiringFederation();
return retiringFederation.map(Federation::getCreationBlockNumber)
.orElse((long) FEDERATION_NON_EXISTENT.getCode());
return retiringFederation.map(Federation::getCreationBlockNumber);
}

@Override
Expand Down
6 changes: 3 additions & 3 deletions rskj-core/src/test/java/co/rsk/peg/BridgeIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1963,9 +1963,9 @@ void getRetiringFederationCreationBlockNumber() {
Bridge bridge = new Bridge(BRIDGE_ADDRESS, constants, activationConfig, null, signatureCache);
BridgeSupport bridgeSupportMock = mock(BridgeSupport.class);
TestUtils.setInternalState(bridge, "bridgeSupport", bridgeSupportMock);
when(bridgeSupportMock.getRetiringFederationCreationBlockNumber()).thenReturn(42L);

MatcherAssert.assertThat(bridge.getRetiringFederationCreationBlockNumber(new Object[]{}), is(42L));
long retiringFederationCreationBlockNumber = 42L;
when(bridgeSupportMock.getRetiringFederationCreationBlockNumber()).thenReturn(Optional.of(retiringFederationCreationBlockNumber));
MatcherAssert.assertThat(bridge.getRetiringFederationCreationBlockNumber(new Object[]{}), is(retiringFederationCreationBlockNumber));
}

@Test
Expand Down
5 changes: 3 additions & 2 deletions rskj-core/src/test/java/co/rsk/peg/BridgeSupportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,8 +507,9 @@ void getRetiringFederationCreationTime() {
void getRetiringFederationCreationBlockNumber() {
long creationBlockNumber = federation.getCreationBlockNumber();

when(federationSupport.getRetiringFederationCreationBlockNumber()).thenReturn(creationBlockNumber);
assertThat(bridgeSupport.getRetiringFederationCreationBlockNumber(), is(creationBlockNumber));
Optional<Long> creationBlockNumberOptional = Optional.of(creationBlockNumber);
when(federationSupport.getRetiringFederationCreationBlockNumber()).thenReturn(creationBlockNumberOptional);
assertThat(bridgeSupport.getRetiringFederationCreationBlockNumber(), is(creationBlockNumberOptional));
}

@Test
Expand Down
23 changes: 12 additions & 11 deletions rskj-core/src/test/java/co/rsk/peg/federation/FederationSupportImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,9 +1305,9 @@ void getRetiringFederationCreationTime_returnsEmpty() {

@Test
@Tag("getRetiringFederationCreationBlockNumber")
void getRetiringFederationCreationBlockNumber_returnsRetiringFederationNonExistentResponseCode() {
long retiringFederationCreationBlockNumber = federationSupport.getRetiringFederationCreationBlockNumber();
assertThat(retiringFederationCreationBlockNumber, is((long) FederationChangeResponseCode.FEDERATION_NON_EXISTENT.getCode()));
void getRetiringFederationCreationBlockNumber_returnsEmpty() {
Optional<Long> retiringFederationCreationBlockNumber = federationSupport.getRetiringFederationCreationBlockNumber();
assertTrue(retiringFederationCreationBlockNumber.isEmpty());
}

@Test
Expand Down Expand Up @@ -1394,9 +1394,9 @@ void getRetiringFederationCreationTime_returnsEmpty() {

@Test
@Tag("getRetiringFederationCreationBlockNumber")
void getRetiringFederationCreationBlockNumber_returnsRetiringFederationNonExistentResponseCode() {
long retiringFederationCreationBlockNumber = federationSupport.getRetiringFederationCreationBlockNumber();
assertThat(retiringFederationCreationBlockNumber, is((long) FederationChangeResponseCode.FEDERATION_NON_EXISTENT.getCode()));
void getRetiringFederationCreationBlockNumber_returnsEmpty() {
Optional<Long> retiringFederationCreationBlockNumber = federationSupport.getRetiringFederationCreationBlockNumber();
assertTrue(retiringFederationCreationBlockNumber.isEmpty());
}

@Test
Expand Down Expand Up @@ -1689,7 +1689,7 @@ void getRetiringFederationCreationTime_withNewFederationActive_returnsOldFederat
@ParameterizedTest
@Tag("getRetiringFederationCreationBlockNumber")
@MethodSource("newFederationNotActiveActivationArgs")
void getRetiringFederationCreationBlockNumber_withNewFederationNotActive_returnsRetiringFederationNonExistentResponseCode(
void getRetiringFederationCreationBlockNumber_withNewFederationNotActive_returnsEmpty(
long currentBlock,
ActivationConfig.ForBlock activations) {

Expand All @@ -1703,8 +1703,8 @@ void getRetiringFederationCreationBlockNumber_withNewFederationNotActive_returns
.withActivations(activations)
.build();

long retiringFederationCreationBlockNumber = federationSupport.getRetiringFederationCreationBlockNumber();
assertThat(retiringFederationCreationBlockNumber, is((long) FederationChangeResponseCode.FEDERATION_NON_EXISTENT.getCode()));
Optional<Long> retiringFederationCreationBlockNumber = federationSupport.getRetiringFederationCreationBlockNumber();
assertTrue(retiringFederationCreationBlockNumber.isEmpty());
}

@ParameterizedTest
Expand All @@ -1724,8 +1724,9 @@ void getRetiringFederationCreationBlockNumber_withNewFederationActive_returnsOld
.withActivations(activations)
.build();

long retiringFederationCreationBlockNumber = federationSupport.getRetiringFederationCreationBlockNumber();
assertThat(retiringFederationCreationBlockNumber, is(oldFederation.getCreationBlockNumber()));
Optional<Long> retiringFederationCreationBlockNumber = federationSupport.getRetiringFederationCreationBlockNumber();
assertTrue(retiringFederationCreationBlockNumber.isPresent());
assertThat(retiringFederationCreationBlockNumber.get(), is(oldFederation.getCreationBlockNumber()));
}

@ParameterizedTest
Expand Down